The attached testcase asserts on ionmonkey revision 4ce3983a43f4 (run with --ion -n -m --ion-eager).

There is some memory corruption involved here too. Valgrind shows this in a release shell, followed by SIGILL: ==13124== Invalid read of size 4 ==13124== at 0x826287B: js::gc::PushMarkStack(js::GCMarker*, js::BaseShape*) (Heap.h:678) ==13124== by 0x8262C32: js::gc::ScanShape(js::GCMarker*, js::Shape*) (Marking.cpp:602) ==13124== by 0x8264E21: js::GCMarker::drainMarkStack(js::SliceBudget&) (Marking.cpp:573) ==13124== by 0x80C1A8A: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind) (jsgc.cpp:3310) ==13124== by 0x80C2F01: Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3769) ==13124== by 0x80C3248: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:3793) ==13124== by 0x831C490: js::ion::IonCode::New(JSContext*, unsigned char*, unsigned int, JSC::ExecutablePool*) (jsgcinlines.h:413) ==13124== by 0x83E1E4D: js::ion::CodeGenerator::generate() (IonLinker.h:86) ==13124== by 0x831EE61: IonCompile(JSContext*, JSScript*, js::StackFrame*, unsigned char*) (Ion.cpp:749) ==13124== Address 0x8dfcbc8 is 17,728 bytes inside a block of size 32,768 free'd ==13124== at 0x77A020B: free (vg_replace_malloc.c:366) ==13124== by 0x824A75C: js::LifoAlloc::freeAll() (Utility.h:169) ==13124== by 0x80B8E36: PurgeRuntime(JSTracer*) (jsgc.cpp:2918) ==13124== by 0x80BF1D4: BeginMarkPhase(JSRuntime*) (jsgc.cpp:2991) ==13124== by 0x80C1A29: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind) (jsgc.cpp:3306) ==13124== by 0x80C2F01: Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3769) ==13124== by 0x80C3248: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:3793) ==13124== by 0xFECD3287: ??? GDB just crashes in markIfUnmarked.

This bug is me forgetting to implement torn value tracing. This patch: * Changes LSRA to filter which torn values get added to safe points. * Encodes torn nunboxes in safepoints. Compaction is similar to snapshots, ideally in the future this could be shared. * Removed the LSafepoint::spillRegs optimization since this caused a bug that took hours to track down: nunbox regs that were in WrapperMask were elided. Can be brought back if it ends up mattering but reg spilling isn't the fast path.

This patch bloats encoded safe points by at least one byte. Eyeballing the test case here, about 75% of x86 safepoints have no torn nunboxes, and the majority of ones that do, have only one entry. Also out-of-line calls may bloat by a few bytes to push/pop registers that are non-volatile, live, and not holding gc things.

Comment on attachment 628202 [details] [diff] [review] fix Review of attachment 628202 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=me with comments addressed. ::: js/src/ion/IonFrames.cpp @@ +415,5 @@ > + return *frame.jsFrame()->slotRef(slot); > + } > + uint32 index = a->toArgument()->index(); > + uint8 *argv = reinterpret_cast<uint8 *>(frame.jsFrame()->argv()); > + return *reinterpret_cast<uintptr_t *>(argv + index); Don't we want "Value *" or something here? I think argv + index is wrong since argv points to Value-sized data, not uint8. ::: js/src/ion/Safepoints.cpp @@ +162,5 @@ > +// If ppp = Reg, payload is reg YYYYY > +// > +// If ttt != Reg, type is: > +// XXXXX if not 11111, otherwise followed by [vwu] > +// If ppp != Reg, payloa is: Micro nit: payload @@ +180,5 @@ > +static const uint32 MAX_INFO_VALUE = (1 << PART_INFO_BITS) - 1; > +static const uint32 TYPE_KIND_SHIFT = 16 - PART_KIND_BITS; > +static const uint32 PAYLOAD_KIND_SHIFT = TYPE_KIND_SHIFT - PART_KIND_BITS; > +static const uint32 TYPE_INFO_SHIFT = PAYLOAD_KIND_SHIFT - PART_INFO_BITS; > +static const uint32 PAYLOAD_INFO_SHIFT = TYPE_INFO_SHIFT - PART_INFO_BITS; A JS_STATIC_ASSERT(PAYLOAD_INFO_SHIFT == 0); would be good as an extra sanity check. @@ +206,5 @@ > + *out = a.toStackSlot()->slot(); > + else > + *out = a.toArgument()->index(); > + > + return *out != MAX_INFO_VALUE; return *out < MAX_INFO_VALUE; @@ +238,5 @@ > + > + uint32 typeVal; > + bool typeExtra = !CanEncodeInfoInHeader(entry.type, &typeVal); > + if (!typeExtra) > + header |= (typeVal << TYPE_INFO_SHIFT); Don't we have to encode MAX_INFO_VALUE (11111) if typeExtra/payloadExtra? It may be good to (temporarily) test with MAX_INFO_VALUE == 0 or 1. Maybe we should have some (configure-) flag to set all kinds of unusual limits so that we can exercise these paths better. @@ +375,5 @@ > + if (info == MAX_INFO_VALUE) > + info = stream.readUnsigned(); > + > + if (kind == Part_Stack) > + return LStackSlot(info); JS_ASSERT(kind == Part_Arg); to complain when we end up with an invalid or new |kind|.

> > + uint32 index = a->toArgument()->index(); > > + uint8 *argv = reinterpret_cast<uint8 *>(frame.jsFrame()->argv()); > > + return *reinterpret_cast<uintptr_t *>(argv + index); > > Don't we want "Value *" or something here? I think argv + index is wrong > since argv points to Value-sized data, not uint8. LArgument's "index" is a byte offset, confusingly. Someday we should change either the name or the way the offset is computed. Thanks for the careful review, I'm testing with MAX_INFO_VALUE == 1 right now.

w/ nits picked: http://hg.mozilla.org/projects/ionmonkey/rev/06a664ebc3cc

JSBugMon: This bug has been automatically verified fixed.

Automatically extracted testcase for this bug was committed: https://hg.mozilla.org/mozilla-central/rev/2e891e0db397