698085 - VMPI_callWithRegistersSaved broken on macos-x64

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)

Description

[Edwin Smith]

Attachment: Fix #ifdefs on mac, and force jmp_buf to be ptr-aligned.

Callee-saved registers aren't being saved in a way that's visible to the ZCT reaper, for two reasons.

1. PosixPortUtils.cpp checks  #if defined linux || defined __GNUC__ before checking for AVMPLUS_MAC.  Mac uses gcc, so we get the generic GCC function that uses __builtin_unwind_init().

On 10.7, with gcc-4.2 and with gcc (default llvm-gcc), __builtin_unwind_init doesn't apparently save callee-saved registers.  (found by disassembling in gdb).

2. If you fix the ifdefs, we use setjmp:

   jmp_buf buf;
   _setjmp(buf);

jmp_buf is int[], and the compiler is free to store it at a 4-aligned offset on the stack.  Pointers inside it will be unaligned, thus invisible to ZCT.

Comment 1

[Edwin Smith]

After posting the patch, it occurred to me a union probably works too:

>    union { intptr_t force_align; jmp_buf buf; }; (void) force_align;
>    VMPI_setjmpNoUnwind(buf);                   // Save registers - not always reliable

Comment 2

[Edwin Smith]

More data points on reproduction:
 - reproduces on halfmoon, because strings stay in registers without spilling.  CodegenLIR generates less register pressure, so is less susceptible.
 - reproduces on llvm-gcc (macos 10.7 default gcc) but not gcc-4.2 (classic gcc), presumably because llvm-gcc takes more liberties with alignment of int[] arrays, which is what jmp_buf is.
 - reproduces on x64 but not x86, because __builtin_unwind_init() properly spills callee-saved registers on x86, and because sizeof(ptr) == sizeof(int) on 32-bit.

One of the failing tests is ecma3/Expressions/e11_4_8.as.  The critical function holds a string (n) in a register, but nowhere on the stack, while frequent string allocation occurs, forcing ZCT reaps:

function Not(n) {
    n = ToInt32(n);
    n = ToInt32BitString(n);
    var r = ""
    for( var l = 0; l < n.length; l++  )           // n is the finalized-while-used string
        r += ( n.charAt(l) == "0" ) ? "1" : "0";   // frequent string allocation
    n = ToInt32Decimal(r);
    return n;
}

Comment 3

[Lars T Hansen]

Comment on attachment 570345 [details] [diff] [review]
Fix #ifdefs on mac, and force jmp_buf to be ptr-aligned.

There needs to be a comment on the #ifdef for MAC that it needs to come before any check for __GNUC__.

What are the implications for the linux/gcc case if it's the case that __builtin_unwind_init does not save correctly?  Is this a 4.2 specific bug and "nobody but Mac uses that" or is it a mac-specific bug (mismatch between llvm-gcc and libraries) or is something else going on?

Comment 4

[Edwin Smith]

Yeah, will investigate.  Do you know of the back-story on getcontext()?  seems purpose-built for what we need.  Solaris uses it, mac probably could (portably), maybe others too.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Edwin Smith from comment #0)
> jmp_buf is int[], and the compiler is free to store it at a 4-aligned offset
> on the stack.  Pointers inside it will be unaligned, thus invisible to ZCT.

Quick thoughts: Should we add an assertion that the address of buf.buf is aligned?  Is there some way to express that as a static assert?  (We may not be able to static assert the whole package, but we could at least static assert that the offsetof .buf within the struct is a sane offset.)

Comment 6

[Edwin Smith]

(In reply to Lars T Hansen from comment #3)
> Comment on attachment 570345 [details] [diff] [review] [diff] [details] [review]
> Fix #ifdefs on mac, and force jmp_buf to be ptr-aligned.
> 
> There needs to be a comment on the #ifdef for MAC that it needs to come
> before any check for __GNUC__.

comment added.

> What are the implications for the linux/gcc case if it's the case that
> __builtin_unwind_init does not save correctly?  Is this a 4.2 specific bug
> and "nobody but Mac uses that" or is it a mac-specific bug (mismatch between
> llvm-gcc and libraries) or is something else going on?

This appears to be an llvm-gcc specific bug, where llvm aligns int[] more aggressively than classic gcc-4.2.  A cursory disassemble of classic gcc-4.2 __builtin_unwind_init() showed it not saving all callee-saved registers.

I spent a half hour parusing getcontext(), that seems like a perfect fit - ucontext_t contains pointer sized fields and thus should be aligned.  but, its also probably heavyweight, saving more than what the GC needs.

Comment 7

[Edwin Smith]

tamarin-redux:
changeset:   6697:b2eea1dadc64
user:        Edwin Smith <edwsmith>
date:        Fri Oct 28 13:36:00 2011 -0400
summary:     Bug 698085 - VMPI_callWithRegistersSaved broken on macos-x64 (r=treilly+ sr=lhansen+)

Comment 8

[Brent Baker]

Buildbot failure compiling mac 64bit release and release-debugger that appears to be caused by this change:

../VMPI/PosixPortUtils.cpp: In function ‘void VMPI_callWithRegistersSaved(void (*)(void*, void*), void*)’:
../VMPI/PosixPortUtils.cpp:214: error: ‘assert’ was not declared in this scope

Comment 9

[Tamarin Bot]

changeset: 6702:64a6c89cc39f
user:      Felix S Klock II <[e-mail]>
summary:   Bug 698085: Include VMAssert.h in attempt to placate buildbot (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/64a6c89cc39f

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Numerous assert fails atop mac64 buildbot, e.g.:

/Users/build/buildbot/tamarin-redux/mac64-intel/builds/6702-64a6c89cc39f/mac/avmshell_sd_64   abcasm/abs_helper.abc abcasm/bkpt.abc
   Assertion failed: ((intptr_t(&buf) & ~(sizeof(void*)-1)) == 0), function VMPI_callWithRegistersSaved, file ../VMPI/PosixPortUtils.cpp, line 215.
   Assertion failed: ((intptr_t(&buf) & ~(sizeof(void*)-1)) == 0), function VMPI_callWithRegistersSaved, file ../VMPI/PosixPortUtils.cpp, line 215.
   unexpected exit code expected:0 actual:-6 Signal Name: SIGABRT FAILED!
   captured output: Assertion failed: ((intptr_t(&buf) & ~(sizeof(void*)-1)) == 0), function VMPI_callWithRegistersSaved, file ../VMPI/PosixPortUtils.cpp, line 215.|
   FAILED  passes:0 fails:1 unexpected  passes: 0 expected  failures: 0

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: fix asserts to mask away the high bits, not the low ones

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 571596 [details] [diff] [review]
fix asserts to mask away the high bits, not the low ones

(going to push in attempt to fix the build, but figured I'd give Ed a heads-up about this.)

Comment 13

[Tamarin Bot]

changeset: 6703:9dda02d2847b
user:      Felix S Klock II <[e-mail]>
summary:   Bug 698085: fix asserts to mask away high bits not the low ones (r=fklockii, sr pending=edwsmith).

http://hg.mozilla.org/tamarin-redux/rev/9dda02d2847b

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

One mystery: why did these failures only come up on Mac64?

- I had assumed when I added the include of VMAssert.h (comment 9) that the reason we only saw failures on mac64 is that the other platforms were setting up the headers to introduce the assert function (and presumably enable it on debug builds).

- But after I added that include of VMAssert.h, the Mac64 builds started failing due to what appears to be a logic error in the assert.

- But no other build failed.

This indicates to me that it is likely the calls to assert were not happening on any platforms except Mac64.  This means that there is something wrong on our debug builds, at least with respect to calls to assert() within VMPI support code.

I don't know exactly what is wrong.  I strongly suspect it has something to do with the attempt to do away with our own assert routine and switching to using the host provided assert.h; see Bug 645878, comment 33, Bug 645878, comment 34, Bug 645878, comment 35, Bug 645878, comment 38, and so on.

It would be good to resolve this mystery.  But that is not a job for this ticket.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #14)
> It would be good to resolve this mystery.  But that is not a job for this
> ticket.

Filed as Bug 699363

Comment 16

[Edwin Smith]

Comment on attachment 571596 [details] [diff] [review]
fix asserts to mask away the high bits, not the low ones

ohboy, thanks for the save.

Comment 17

[Edwin Smith]

is there anything else for this bug? if not I can re-close it.  thanks again for the bailout.

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Edwin Smith from comment #17)
> is there anything else for this bug? if not I can re-close it.  thanks again
> for the bailout.

not to my knowledge.