The attached testcase crashes on ionmonkey revision 9cf3ea112635 (run with --ion -n -m --ion-eager).

Stack: ==17287== Invalid read of size 1 ==17287== at 0x5FFEB9: js::gc::MarkThingOrValueRoot(JSTracer*, unsigned long*, char const*) (Marking.cpp:296) ==17287== by 0x6D03D2: js::ion::MarkIonActivations(JSRuntime*, JSTracer*) (IonFrames.cpp:461) ==17287== by 0x46B3B7: _ZN2jsL11MarkRuntimeEP8JSTracerb.clone.0 (jsgc.cpp:2449) ==17287== by 0x46CA2E: IncrementalMarkSlice(JSRuntime*, long, js::gcreason::Reason, bool*) (jsgc.cpp:3142) ==17287== by 0x46F8F4: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3832) ==17287== by 0x470E94: Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3945) ==17287== by 0x51C108: js_NewStringCopyN(JSContext*, char const*, unsigned long) (jsgcinlines.h:411) ==17287== by 0x4AC8A4: js_NumberToStringWithBase(JSContext*, double, int) (jsnum.cpp:1189) ==17287== by 0x511355: js::ToStringSlow(JSContext*, JS::Value const&) (jsstr.cpp:3254) ==17287== by 0x512F7B: js_String(JSContext*, unsigned int, JS::Value*) (jsstr.h:128) ==17287== by 0x4045571: ??? ==17287== by 0xC71C1FF: ??? ==17287== Address 0x18 is not stack'd, malloc'd or (recently) free'd

Christian, this is the output I get: SyntaxError: missing } in compound statement ReferenceError: PST_JAN_1_2005 is not defined BUGNUMBER: 334807 ReferenceError: printStatus is not defined ReferenceError: RECORDLOOP is not defined testfunc : FAILED: expected number ( Infinity ) != actual testfunc : FAILED: expected number ( 1.7976931348623157e+308 ) != actual testfunc : FAILED: expected number ( 5e-324 ) != actual testfunc : FAILED: expected number ( 1.7976931348623157e+308 ) != actual testfunc : FAILED: expected number ( 5e-324 ) != actual testfunc : FAILED: expected number ( 1 ) != actual testfunc : FAILED: expected number ( 1.5707963267948966 ) != actual testfunc : FAILED: expected number ( 0.0001 ) != actual testfunc : FAILED: expected number ( NaN ) != actual Following the instructions in README, on both tip and the given changeset. Can you still reproduce this? If so I might have to ask for a core dump.

Oops, the bot cannot yet "cope with archived testcases yet."

Okay, the approach was working after all. Gary, could you re-test this just to make sure the crash goes away? (It was difficult to reproduce the segfault so I want to make sure.) This patch instruments the code generation for LStackArg, to track which stack slots get pushed. Every time we visit an instruction that has a safepoint, we add the current pending LStackArg slots to its safepoint. I feel kind of gross about this, but there aren't many other alternatives. Ideally we'd be able to just scan a delimited range in MarkIonJSFrame. To do that we'd have to push parameters in reverse order (that's not a change we should make right now), or delay LStackArgs until we reach the call, which would introduce longer live ranges. I'd be happy if there was a clearer way to fill the safepoint but I don't see that either.

Comment on attachment 642123 [details] an fix Review of attachment 642123 [details]: ----------------------------------------------------------------- Add something to discard pushedArgs at the call-site and r+. You might also consider the fact that arguments are pre-allocated vector of a predefined length which is initialized contiguously. This imply that you don't need to store the n arguments of each functions and that you might only push a tuple of the base StackOffset ( framePushed() ) with the number of initialized arguments ( n ). [ locals | (n - 3) undefined | 3 defined | (n - 1) undefined | 1 defined ] This can be translated by pushing the StackOffset to pushedArgumentSlots at the MPrepareCall and increment the number of argument initialized — of the last pushedArgumentSlots — in visitStackArg. Then a visitCall* will just pop the last entry. ::: js/src/ion/CodeGenerator.cpp @@ +402,5 @@ > uint32 argslot = lir->argslot(); > int32 stack_offset = StackOffsetOfPassedArg(argslot); > > masm.storeValue(val, Address(StackPointer, stack_offset)); > + return pushedArgumentSlots_.append(StackOffsetToSlot(stack_offset)); Where do you clear the pushedArgumentsSlots_? I think you can use ShrinkBy at the visitCall* which are expecting StackArg. @@ +2604,5 @@ > { > JSContext *cx = gen->cx; > > + unsigned slots = graph.localSlotCount() + > + (graph.argumentSlotCount() * sizeof(Value) / STACK_SLOT_SIZE); Either there is a naming issue or this feel inconsistent. ::: js/src/ion/shared/CodeGenerator-shared.h @@ +135,4 @@ > } > > inline int32 SlotToStackOffset(int32 slot) const { > JS_ASSERT(slot > 0 && slot <= int32(graph.localSlotCount())); Does this assertion hold without argumentSlotCount? @@ +143,5 @@ > + inline int32 StackOffsetToSlot(int32 offset) const { > + // offset = framePushed - (slot * STACK_SLOT_SIZE) > + // offset + (slot * STACK_SLOT_SIZE) = framePushed > + // slot * STACK_SLOT_SIZE = framePushed - offset > + // slot = (framePushed - offset) / STACK_SLOT_SIZE nit: // see SlotToStackOffset.

I forgot to mention that a good test case might be something which looks like: var i = 0; var j = 0; var k = 0; function g (a,b,c,d) { try {} catch(x) {}; if (k == 100 && i++ == j) gc(); } function f () { g( g( g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ) ), g( g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ) ), g( g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ) ), g( g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ), g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ) ) ) } for (; j <= 85; j++) { i = 0; for (k = 0; k <= 100; k++) f(); }

Comment on attachment 642123 [details] an fix Review of attachment 642123 [details]: ----------------------------------------------------------------- This is OK. It's a little sad that walking over the initialized contiguous slots in the argument vector by index didn't work out. That would have let us keep the distinction between local slots and argument vector slots, which is marginally less complicated. ::: js/src/ion/CodeGenerator.cpp @@ +1141,5 @@ > current = graph.getBlock(i); > for (LInstructionIterator iter = current->begin(); iter != current->end(); iter++) { > IonSpew(IonSpew_Codegen, "instruction %s", iter->opName()); > + if (iter->safepoint() && pushedArgumentSlots_.length()) { > + if (!markArgumentSlots(iter->safepoint())) As pierron pointed out, pushedArgumentSlots_ needs to be reset by the LCall* instructions. Those instructions must therefore also take care to call markArgumentSlots() themselves. ::: js/src/ion/IonCode.h @@ +210,5 @@ > > // Local slot count and frame size. Frame size is the value that can > // be added to the StackPointer along with the frame prefix to get a > // valid IonJSFrameLayout. > + uint32 frameSlots_; Comment doesn't mention that it includes maximum length of nested argument vectors. ::: js/src/ion/Safepoints.cpp @@ +269,5 @@ > > SafepointReader::SafepointReader(IonScript *script, const SafepointIndex *si) > : stream_(script->safepoints() + si->safepointOffset(), > script->safepoints() + script->safepointsSize()), > + localSlotCount_(script->frameSlots()) Does localSlotCount_ also require renaming now? ::: js/src/ion/shared/CodeGenerator-shared.h @@ +94,5 @@ > > // Vector of all patchable write pre-barrier offsets. > js::Vector<CodeOffsetLabel, 0, SystemAllocPolicy> barrierOffsets_; > > + // List of pushed argument slots. nit: // List of pushed slots for MCall arguments. (as opposed to MParameters).

Comment on attachment 642123 [details] an fix And of course, somehow, I posted this in the completely wrong security bug. I've fixed the nits and will post a final corrected patch where it belongs in bug 771398.

Bisect shows this stopped reproducing on a seemingly irrelevant changeset - debugging the original cset, this is definitely bug 779390.

A testcase for this bug was already added in the original bug (bug 779390).