Closed Bug 633436 Opened 13 years ago Closed 13 years ago

Android (thumb) debug builds are broken

Categories

(Toolkit :: Crash Reporting, defect)

Product:
Toolkit
Component:
Crash Reporting
Platform:
ARM
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(2 files)

Fix register conflict
1.14 KB, patch
vlad
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
Updated comment to note gcc weirdness
1.53 KB, patch
cjones
: review+
Details | Diff | Splinter Review 
/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
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.
Assignee: nobody → jones.chris.g
Attachment #511638 - Flags: review?(vladimir)
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 on attachment 511638 [details] [diff] [review]
Fix register conflict

Nm, I do need to store r7 here.
Attachment #511638 - Attachment is obsolete: true
Attachment #511638 - Flags: review?(vladimir)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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.)
Attachment #511638 - Attachment is obsolete: false
Attachment #511638 - Flags: review?(vladimir)
(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 on attachment 511638 [details] [diff] [review]
Fix register conflict

I guess this is ok, but it's a little weird that it's broken!
Attachment #511638 - Flags: review?(vladimir) → review+
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 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.
Attachment #511638 - Flags: approval2.0?
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.
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/
Oh, ick, it's a Makefile patch. :-(  There's really no legitimate way to fix this, eh?
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.
Attachment #511638 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/08092fa7b8c8
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: