633436 - Android (thumb) debug builds are broken

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

/home/cjones/mozilla/mozilla-central/toolkit/crashreporter/google-breakpad/src/client/linux/handler/../../../common/linux/linux_syscall_support.h: In function 'int sys_clone(int (*)(void*), void*, int, void*, int*, void*, int*)':
/home/cjones/mozilla/mozilla-central/toolkit/crashreporter/google-breakpad/src/client/linux/handler/../../../common/linux/linux_syscall_support.h:2202: error: r7 cannot be used in asm here

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix register conflict

r?->vlad on blassey's advice.

The comment in the patch describes the problem.  I'm not sure if I need to save r7 here or I can rely on this function being inlined and the caller's save of r7.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The code causing the error is

    LSS_INLINE int LSS_NAME(clone)(int (*fn)(void *), void *child_stack,
                                   int flags, void *arg, int *parent_tidptr,
                                   void *newtls, int *child_tidptr) {
      long __res;
      __asm__ __volatile__(/* if (fn == NULL)
                            *   return -EINVAL;
                            */
                           "movl   %3,%%ecx\n"
                           "jecxz  1f\n"

                           /* if (child_stack == NULL)
                            *   return -EINVAL;
                            */
                           "movl   %4,%%ecx\n"
                           "jecxz  1f\n"

                           /* Set up alignment of the child stack:
                            * child_stack = (child_stack & ~0xF) - 20;
                            */
                           "andl   $-16,%%ecx\n"
                           "subl   $20,%%ecx\n"

                           /* Push "arg" and "fn" onto the stack that will be
                            * used by the child.
                            */
                           "movl   %6,%%eax\n"
                           "movl   %%eax,4(%%ecx)\n"
                           "movl   %3,%%eax\n"
                           "movl   %%eax,(%%ecx)\n"

                           /* %eax = syscall(%eax = __NR_clone,
                            *                %ebx = flags,
                            *                %ecx = child_stack,
                            *                %edx = parent_tidptr,
                            *                %esi = newtls,
                            *                %edi = child_tidptr)
                            * Also, make sure that %ebx gets preserved as it is
                            * used in PIC mode.
                            */
                           "movl   %8,%%esi\n"
                           "movl   %7,%%edx\n"
                           "movl   %5,%%eax\n"
                           "movl   %9,%%edi\n"
                           "pushl  %%ebx\n"
                           "movl   %%eax,%%ebx\n"
                           "movl   %2,%%eax\n"
                           LSS_ENTRYPOINT

                           /* In the parent: restore %ebx
                            * In the child:  move "fn" into %ebx
                            */
                           "popl   %%ebx\n"

                           /* if (%eax != 0)
                            *   return %eax;
                            */
                           "test   %%eax,%%eax\n"
                           "jnz    1f\n"

                           /* In the child, now. Terminate frame pointer chain.
                            */
                           "movl   $0,%%ebp\n"

                           /* Call "fn". "arg" is already on the stack.
                            */
                           "call   *%%ebx\n"

                           /* Call _exit(%ebx). Unfortunately older versions
                            * of gcc restrict the number of arguments that can
                            * be passed to asm(). So, we need to hard-code the
                            * system call number.
                            */
                           "movl   %%eax,%%ebx\n"
                           "movl   $1,%%eax\n"
                           LSS_ENTRYPOINT

                           /* Return to parent.
                            */
                         "1:\n"
                           : "=a" (__res)
                           : "0"(-EINVAL), "i"(__NR_clone),
                             "m"(fn), "m"(child_stack), "m"(flags), "m"(arg),
                             "m"(parent_tidptr), "m"(newtls), "m"(child_tidptr)
                           : "esp", "memory", "ecx", "edx", "esi", "edi");
      LSS_RETURN(int, __res);
    }

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 511638 [details] [diff] [review]
Fix register conflict

Nm, I do need to store r7 here.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 511638 [details] [diff] [review]
Fix register conflict

The existing code is already marking r7 as a clobbered register, so it should be OK.  (God I hate inline assembly.)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #2)
> The code causing the error is

... and that's totally the x86 code.  Should read before I copypaste.  Here's what I meant to post.

    LSS_INLINE int LSS_NAME(clone)(int (*fn)(void *), void *child_stack,
                                   int flags, void *arg, int *parent_tidptr,
                                   void *newtls, int *child_tidptr) {
      long __res;
      {
        register int   __flags __asm__("r0") = flags;
        register void *__stack __asm__("r1") = child_stack;
        register void *__ptid  __asm__("r2") = parent_tidptr;
        register void *__tls   __asm__("r3") = newtls;
        register int  *__ctid  __asm__("r4") = child_tidptr;
        __asm__ __volatile__(/* if (fn == NULL || child_stack == NULL)
                              *   return -EINVAL;
                              */
                             "cmp   %2,#0\n"
                             "it    ne\n"
                             "cmpne %3,#0\n"
                             "it    eq\n"
                             "moveq %0,%1\n"
                             "beq   1f\n"

                             /* Push "arg" and "fn" onto the stack that will be
                              * used by the child.
                              */
                             "str   %5,[%3,#-4]!\n"
                             "str   %2,[%3,#-4]!\n"

                             /* %r0 = syscall(%r0 = flags,
                              *               %r1 = child_stack,
                              *               %r2 = parent_tidptr,
                              *               %r3 = newtls,
                              *               %r4 = child_tidptr)
                              */
                             "mov r7, %9\n"
                             "swi 0x0\n"

                             /* if (%r0 != 0)
                              *   return %r0;
                              */
                             "movs  %0,r0\n"
                             "bne   1f\n"

                             /* In the child, now. Call "fn(arg)".
                              */
                             "ldr   r0,[sp, #4]\n"
                             "mov   lr,pc\n"
                             "ldr   pc,[sp]\n"

                             /* Call _exit(%r0).
                              */
                             "mov r7, %10\n"
                             "swi 0x0\n"
                           "1:\n"
                             : "=r" (__res)
                             : "i"(-EINVAL),
                               "r"(fn), "r"(__stack), "r"(__flags), "r"(arg),
                               "r"(__ptid), "r"(__tls), "r"(__ctid),
                               "i"(__NR_clone), "i"(__NR_exit)
                             : "cc", "r7", "lr", "memory");
      }
      LSS_RETURN(int, __res);
    }

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 511638 [details] [diff] [review]
Fix register conflict

I guess this is ok, but it's a little weird that it's broken!

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

FWIW, I think this is a gcc bug.  The generic syscall template, LSS_BODY, writes to r7 but pushes/pops it, and gcc doesn't complain about that.  Seems like it treats clobbered registers differently, or maybe it's being smarter than we're giving it credit for.  I dunno.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 511638 [details] [diff] [review]
Fix register conflict

Would like to land this to unbork android debug builds.  We don't really care so much about breakpad in debug builds, so if it's wonky it's not on anyone's critical path to fix it.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, exception_handler.s sez

sys_clone:
.LFB1131:
	.file 8 "/home/cjones/mozilla/mozilla-central/toolkit/crashreporter/google-breakpad/src/client/linux/handler/../../../common/linux/linux_syscall_support.h"
	.loc 8 2144 0
	@ args = 12, pretend = 0, frame = 24
	@ frame_needed = 0, uses_anonymous_args = 0
	push	{r4, r5, r7, lr}
[snip]
	pop	{r4, r5, r7, pc}
.LFE1131:
	.size	sys_clone, .-sys_clone

so the generated assembly is doing the right thing.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Updated comment to note gcc weirdness

Comment 13

[(not currently active) Ted Mielczarek]

That file has its own SVN repo, if you can give me a patch against that repo I'll ferry this upstream for you:
http://code.google.com/p/linux-syscall-support/

Comment 14

[(not currently active) Ted Mielczarek]

Oh, ick, it's a Makefile patch. :-(  There's really no legitimate way to fix this, eh?

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Well, there's no bug to "fix" on our side, AFAICT.  I put what I know in the Makefile comment

+# The syscall number is passed through r7 in the linux ARM ABI, but r7
+# is also the THUMB frame pointer.  (Unfortunate, but ah well.)  gcc
+# complains if we store to r7, not unreasonably, but complains
+# inconsistently.  The generic syscall template pushes/stores to/pops
+# r7 with no complaint from gcc, but the sys_clone() function marks r7
+# as a clobbered register yet gcc error's.  The generated assembly for
+# sys_clone() looks OK, so we chalk this up to a gcc/gas quirk and
+# work around it by telling gcc that the THUMB frame pointer is a
+# vanilla callee-save register.

I'm about 90% sure gcc is either being weird or being very smart.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/08092fa7b8c8