The following testcase asserts on ionmonkey revision 1fd6c40d3852 (run with --ion -n -m --ion-eager): var ary = ["\u001Cfoo", "\u001Dfoo", "\u001Efoo", "\u001Ffoo"]; for (var i in ary) { ary[Number(i)].search("var MYVAR='077';++MYVAR") }

This is a bug with our cx->enumerators handling, which is broken anyway, so I'll do a full fix here.

This patch splits Register containers out into a separate file. This work is to make sure we can iterate values in Ion frames with a minimal include set.

Splits Snapshots.h into SnapshotReader.h and SnapshotWriter.h

A light refactoring of SnapshotReader and SnapshotIterator. The latter now inherits, instead of embeds, the former. The steps needed to iterate are also more clear and it's easier to skip over values that can't be read. This patch also removes IonBailoutIterator.

Comment on attachment 608201 [details] [diff] [review] part 1: split Register/FloatRegister into new header Review of attachment 608201 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/RegisterSets.h @@ +47,5 @@ > + > +namespace js { > +namespace ion { > + > +struct AnyRegister { suggestion: This kind of structure does not support any kind of iteration. Just for cleaning up a bit more to match the file name, it sounds better to move AnyRegister, ValueOperand, TypedOrValueRegister, ConstantOrRegister and Int32Key to another header named MacroAssemblerOperands.h which would be included by this one.

Comment on attachment 608203 [details] [diff] [review] part 3: refactor SnapshotIterator/SnapshotReader Review of attachment 608203 [details] [diff] [review]: ----------------------------------------------------------------- Apply nits and r=me. ::: js/src/ion/Bailouts.cpp @@ +79,2 @@ > } else { > + scopeChainv = iter.read(); nit: using iter.read() is dangerous because it is not copy&paste safe. I would prefer to keep iter.read() in a loop. One option is to make the scope chain part of the variable cached by the snapshot reader. ::: js/src/ion/SnapshotReader.h @@ +255,5 @@ > + return UndefinedValue(); > + } > + > + bool moreSlots() const { > + return slotsRead_ < slotCount_; nit: You should test against 0 instead of reading slotCount_. In addition, this would be easier to follow when debugging because both remainingFrames_ and slotCounts_ would be decreasing. @@ +271,3 @@ > static Value FromTypedPayload(JSValueType type, uintptr_t payload); > > + Value slotValue(const Slot &slot); nit: We cannot read everything, because some values, which are not mapped to stack slots, may still leave in registers before leaving IonMonkey. We need this function to be public, such as one can get the slot, ask if it is readable, and then load it with slotValue. MachineRegs &mr = … … while (si.moreSlots()) { SnapshotIterator::Slot s = si.readSlot(); AnyRegister reg; if (!s.liveInRegister(&reg) || mr.readableRegister(reg)) dump.push(slotValue(s)); else dump.push(OptimizedValue()) } ::: js/src/ion/Snapshots.cpp @@ +292,2 @@ > { > JS_ASSERT(in.snapshotOffset() < in.ionScript()->snapshotsSize()); nit: You may want to move this assert to the CompactBufferReader. JS_ASSERT(start < end);

This patch mostly obsoletes FrameRecovery in favor of simpler initialization aided by IonFrameIterator. It also moves some other classes around. All this refactoring is so we can iterate frame values with a minimal include set, which will let us refactor InlineFrameIterator, which will let us fix this bug a little easier.

This patch combines InlineFrameIterator and InlineFrameReverseIterator, which is now possible since SnapshotIterator has minimal includes. The goal of this patch is to let users observe values in each inlined frame, which is needed to cleanly solve this bug. (The word "cleanly" here is pretty generous but it's the best we can do for now.)

This patch fixes the actual bug, replacing our savedEnumerators hack with actual logic to unwind iterators, using the new InlineFrameIterator. Unfortunately this doesn't work yet because in the original test case for this bug, the iterator is in a register, and we don't support recovering registers in this path yet.

Comment on attachment 610795 [details] [diff] [review] part 5: remove most uses of FrameRecovery Review of attachment 610795 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonFrameIterator.h @@ +223,5 @@ > > class SnapshotIterator : public SnapshotReader > { > + IonJSFrameLayout *fp_; > + const MachineState &machine_; This is fixed in the following patch. @@ +235,5 @@ > > public: > + SnapshotIterator(IonScript *ionScript, SnapshotOffset snapshotOffset, > + IonJSFrameLayout *fp, const MachineState &machine); > + SnapshotIterator(const IonFrameIterator &iter, const MachineState &machine); It is not necessary to transfert the MachineState here since we should recover it from the ion::FrameIterator. ::: js/src/ion/IonFrames.cpp @@ +164,5 @@ > return ionScript_ ? ionScript_ : script_->ion; > } > > InlineFrameReverseIterator::InlineFrameReverseIterator(const IonFrameIterator &bottom) > + : si_(bottom, MachineState()), It is not necessary to transfert the MachineState here since we should recover it from the ion::FrameIterator. ::: js/src/ion/Registers.h @@ +121,5 @@ > +// Information needed to recover machine register state. > +class MachineState > +{ > + uintptr_t *regs_; > + double *fpregs_; This design will not work with OOL paths, because registers are not stored linearly and StackIter iterations need to be allocation free. In addition, moving GC may need to store and OOL path are not saving all registers but only a subset containing GC things and volatile registers. It would be easier to have an indirection map (double*[FloatRegisters::TotalAllocatable]) instead of a table of value, because we can store a NULL pointer to mark unreadbale/unwritable registers saved by the callee of the callVM.

Comment on attachment 610796 [details] [diff] [review] part 6: refactor InlineFrameIterator Review of attachment 610796 [details] [diff] [review]: ----------------------------------------------------------------- I like the merge of the SnapshotIterator with the InlineReverseIterator. 3 aspects which need to be fixed before accepting this patch are: 1/ InlineFrameIterator should not have a MachineState argument, because the only provider of the MachineState is the ion::FrameIterator given as first argument. 2/ InlineFrameIterator is duplicating 2 fields which are already stored in the SnapshotIterator, it would be best to reuse these fields instead of duplicating them. 3/ Rename current "extractFrameBits" & "advance" functions looks more like a "next" & "prev" functions. ::: js/src/ion/Bailouts.cpp @@ +183,5 @@ > } while (fiter.type() != IonFrame_JS && fiter.type() != IonFrame_Entry); > > if (fiter.type() == IonFrame_JS) { > // In the case of a JS frame, look up the pc from the snapshot. > + InlineFrameIterator ifi(&fiter, MachineState()); Remove the second argument. (see below) ::: js/src/ion/IonFrameIterator.h @@ +175,4 @@ > class SnapshotIterator : public SnapshotReader > { > IonJSFrameLayout *fp_; > + MachineState machine_; If the machine state contains 2 big indirection arrys, then you may switch to a MachineState *machine_; @@ +202,5 @@ > + const IonFrameIterator *frame_; > + MachineState machine_; > + SnapshotIterator si_; > + unsigned framesRead_; > + unsigned frameCount_; both fields (frameRead_, frameCount_) are stored in the SnapshotReader (remainingFrames_, frameCount_), you don't need to duplicate them here. @@ +215,5 @@ > + public: > + InlineFrameIterator(const IonFrameIterator *iter, const MachineState &machine); > + > + bool more() const { > + return framesRead_ < frameCount_; return si_.moreFrames(); ::: js/src/ion/IonFrames.cpp @@ +409,5 @@ > > // Recover the innermost inlined frame. > IonFrameIterator it(cx->runtime->ionTop); > ++it; > + InlineFrameIterator ifi(&it, MachineState()); Remove the machine state. @@ +560,5 @@ > } > > +InlineFrameIterator::InlineFrameIterator(const IonFrameIterator *iter, const MachineState &machine) > + : frame_(iter), > + machine_(machine), It would be best if the ion::FrameIterator can return the MachineState based on the safepoint instead of giving it away as an argument of the InlineFrameIterator. In addition this seems to leak to the StackIter. @@ +571,5 @@ > + advance(); > +} > + > +void > +InlineFrameIterator::extractFrameBits() extractFrameBits -> stepOneFrameForward @@ +594,5 @@ > + pc_ = script_->code + si_.pcOffset(); > +} > + > +void > +InlineFrameIterator::advance() advance -> stepOneFrameBackward @@ +606,5 @@ > + pc_ = script_->code + si_.pcOffset(); > + > + // This unfortunately is O(n*m), because we must skip over outer frames > + // before reading inner ones. > + unsigned remaining = frameCount_ - framesRead_ - 1; Use the previous snapshot iterator remaining count instead of storing frameCount_ and framesRead_. ::: js/src/vm/Stack.cpp @@ +1186,5 @@ > continue; > } > > state_ = ION; > + ionInlineFrames_ = ion::InlineFrameIterator(&ionFrames_, ion::MachineState()); Remove the machine state. @@ +1267,5 @@ > savedOption_(savedOption) > #ifdef JS_ION > , ionActivations_(cx), > ionFrames_((uint8_t *)NULL), > + ionInlineFrames_(NULL, ion::MachineState()) idem. @@ +1298,5 @@ > while (ionFrames_.more() && !ionFrames_.isScripted()) > ++ionFrames_; > > if (ionFrames_.more()) { > + ionInlineFrames_ = ion::InlineFrameIterator(&ionFrames_, ion::MachineState()); idem.

Comment on attachment 610816 [details] [diff] [review] part 8: fix actual bug Review of attachment 610816 [details] [diff] [review]: ----------------------------------------------------------------- Use ion::FrameIterator interface, instead of type() function and fix the frameRead iterator issue. ::: js/src/ion/IonFrames.cpp @@ +265,5 @@ > + UnwindIteratorForException(cx, obj); > +} > + > +static void > +UnwindFrame(JSContext *cx, const IonFrameIterator &iter, const InlineFrameIterator &frame) UnwindInlinedFrame @@ +291,5 @@ > + } > +} > + > +static void > +UnwindFrames(JSContext *cx, const IonFrameIterator &iter) UnwindInlineFrames or UnwindFrame @@ +293,5 @@ > + > +static void > +UnwindFrames(JSContext *cx, const IonFrameIterator &iter) > +{ > + for (InlineFrameIterator frames(&iter, ion::MachineState()); frames.more(); ++frames) Remove the MachineState argument. @@ +306,5 @@ > IonSpew(IonSpew_Invalidate, "handling exception"); > > IonFrameIterator iter(cx->runtime->ionTop); > while (iter.type() != IonFrame_Entry) { > if (iter.type() == IonFrame_JS) { Do not use iter.type(). while (iter.more()) { if (iter.isScripted()) { @@ +647,5 @@ > } > > void > InlineFrameIterator::advance() > { Add: JS_ASSERT(more()); @@ +653,5 @@ > frameCount_ = si_.frameCount(); > > + if (framesRead_ == frameCount_) { > + // The iterator is done; perform one final increment so more() makes > + // sense. frameRead_ is starting at 0, so unless frameCount_ is lacking one frame, I don't see any reason to hack the more function and to add this here because this will let you iterate one more time on the same thing at the end of your loop.

> ::: js/src/ion/IonFrameIterator.h > @@ +175,4 @@ > > class SnapshotIterator : public SnapshotReader > > { > > IonJSFrameLayout *fp_; > > + MachineState machine_; > > If the machine state contains 2 big indirection arrys, then you may switch > to a > MachineState *machine_; MachineState needs a separate refactoring that will come in a different patch. > both fields (frameRead_, frameCount_) are stored in the SnapshotReader > (remainingFrames_, frameCount_), you don't need to duplicate them here. > > + bool more() const { > > + return framesRead_ < frameCount_; > > return si_.moreFrames(); The iterators are going in different directions so this doesn't work. When IonFrameIterator is at its last frame, SnapshotIterator is at its first.

> It would be easier to have an indirection map > (double*[FloatRegisters::TotalAllocatable]) instead of a table of value, > because we can store a NULL pointer to mark unreadbale/unwritable registers > saved by the callee of the callVM. That's a good idea.

> @@ +653,5 @@ > > frameCount_ = si_.frameCount(); > > > > + if (framesRead_ == frameCount_) { > > + // The iterator is done; perform one final increment so more() makes > > + // sense. > > frameRead_ is starting at 0, so unless frameCount_ is lacking one frame, I > don't see any reason to hack the more function and to add this here because > this will let you iterate one more time on the same thing at the end of your > loop. frameRead_ starts at 1, since one frame is read at the start.

(In reply to Nicolas B. Pierron [:pierron] from comment #6) > nit: using iter.read() is dangerous because it is not copy&paste safe. I > would prefer to keep iter.read() in a loop. One option is to make the scope > chain part of the variable cached by the snapshot reader. Hrm, this pattern was pre-existing and used for reading |thisv| as well. I don't have any particular opinion but I'd like to wait until this series of refactoring is done to see what further improvements can be made. > nit: You should test against 0 instead of reading slotCount_. In addition, > this would be easier to follow when debugging because both remainingFrames_ > and slotCounts_ would be decreasing. Okay, actually I'd prefer to just make them both increasing, the decreasing thing is kind of mind bending. > > @@ +271,3 @@ > > static Value FromTypedPayload(JSValueType type, uintptr_t payload); > > > > + Value slotValue(const Slot &slot); > > nit: We cannot read everything, because some values, which are not mapped to > stack slots, may still leave in registers before leaving IonMonkey. We need > this function to be public, such as one can get the slot, ask if it is > readable, and then load it with slotValue. A use case hasn't come up for this yet in this patch, since all cases where we need to read a slot, it would be an error to not have registers for that slot (and in fact, that error does come up, necessitating the upcoming MachineRegs overhaul). In other cases, we can use skip(). part 3 - http://hg.mozilla.org/projects/ionmonkey/rev/711b6feb3f9a

part 4 - http://hg.mozilla.org/projects/ionmonkey/rev/19c9e03f06e8 part 5 - http://hg.mozilla.org/projects/ionmonkey/rev/8e8af92a2b45 (elided MachineState changes since this will be a separate overhaul)

(In reply to Nicolas B. Pierron [:pierron] from comment #13) > both fields (frameRead_, frameCount_) are stored in the SnapshotReader > (remainingFrames_, frameCount_), you don't need to duplicate them here. Okay, I was able to remove frameCount_ by caching and extra copy of the initial SnapshotIterator (which also avoids re-fetching the OsiIndex). framesRead_ doesn't nicely correlate to si_.framesRead() though, since they go in reverse directions. > > +void > > +InlineFrameIterator::extractFrameBits() > > extractFrameBits -> stepOneFrameForward I was actually able to just inline this function, so I renamed the outer one to findNextFrame(). Asking for review again - keep in mind, I'll get rid of the bogus MachineState stuff in the next patch.

I had to backout and reland 5 due to a horrible typo in ReadFrameDoubleSlot. part 6 landed too: http://hg.mozilla.org/projects/ionmonkey/rev/fe0d335f6685 http://hg.mozilla.org/projects/ionmonkey/rev/e8fe2c18bcba

last patch before MachineState refactoring

err, qref'd version

Comment on attachment 612438 [details] [diff] [review] part 7: ensure that iterators are actually closed Review of attachment 612438 [details] [diff] [review]: ----------------------------------------------------------------- Apply nits, then r=me. ::: js/src/ion/IonFrames.cpp @@ +243,5 @@ > } > > +static void > +UnwindIterator(JSContext *cx, const IonFrameIterator &iter, const InlineFrameIterator &frame, > + uint32 stackSlot) nit: iter is not used. @@ +265,5 @@ > + UnwindIteratorForException(cx, obj); > +} > + > +static void > +UnwindInlineFrame(JSContext *cx, const IonFrameIterator &iter, const InlineFrameIterator &frame) nit: iter is not used except as argument of a function which does not use it. @@ +291,5 @@ > + } > +} > + > +static void > +UnwindInlineFrames(JSContext *cx, const IonFrameIterator &iter) It took me some time to figure that out, but the name UnwindIteratorFor…(cx, obj) is named after the content of the object which is a JS Iterator. I found confusing to have "UnwindInlineFrames" here because I am thinking of bailouts when reading this function name. "UnwindIteratorFromIonFrame", "UnwindIteratorFromInlinedFrame", "UnwindIteratorFromSnapshot" would be easier to understand. @@ +294,5 @@ > +static void > +UnwindInlineFrames(JSContext *cx, const IonFrameIterator &iter) > +{ > + InlineFrameIterator frames(&iter, ion::MachineState()); > + for (;;) { for (; !frames.done(); ++frames) UnwindInlineFrame(cx, iter, frames); @@ +310,5 @@ > > IonSpew(IonSpew_Invalidate, "handling exception"); > > IonFrameIterator iter(cx->runtime->ionTop); > + while (!iter.isEntry()) { nit: while (!iter.done()) { @@ +312,5 @@ > > IonFrameIterator iter(cx->runtime->ionTop); > + while (!iter.isEntry()) { > + if (iter.isScripted()) { > + UnwindInlineFrames(cx, iter); It looks like you can almost get rid of some these functions by using StackIter here to fold the 2 outer-loops.

This patch introduces a new MachineState that is provided by IonFrameIterator, and works in out-of-line paths. This patch also fixes a bug in the previous patch.

Comment on attachment 613224 [details] [diff] [review] part 8: new MachineState Review of attachment 613224 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good, a few improvment possible on MachineState creation and safepoint reader. The Machine state is created at last when creating the Snapshot Iterator, previously it was created the outer-most location. The machine state is not necessary for all Snapshot Iterator and should be made optional, such as InlineFrameIterator do not need to read the Machine State to iterate over the inline frames. The MachineState class can be renamed because it no longer represents a State but an Index of register state. ::: js/src/ion/IonFrameIterator.h @@ +70,5 @@ > uint8 *current_; > FrameType type_; > uint8 *returnAddressToFp_; > size_t frameSize_; > + mutable const SafepointIndex *cachedSafepointIndex_; hum … That's ok, but weird. ::: js/src/ion/IonFrames.cpp @@ +250,5 @@ > + SafepointReader reader(ionScript(), safepoint()); > + > + GeneralRegisterSet actual, spilled; > + reader.getOsiReturnPointOffset(); > + reader.getGcRegs(&actual, &spilled); I really don't like the idea that the reader is not caching the data read once and that each function as to be knowledgeable about the underlying structure of a safepoint. nit: Get rid of the function which seems useless here. (getOsiReturnPointOffset) @@ +255,5 @@ > + > + // Get the base address to where safepoint registers are spilled. > + // Out-of-line calls do not unwind the extra padding space used to > + // aggregate bailout tables, so we use frameSize instead of frameLocals, > + // which would only account for local stack slots. Question: Do we care about saving this space and file a follow-up bug ? @@ +260,5 @@ > + uintptr_t *spillBase = reinterpret_cast<uintptr_t *>(fp()) + ionScript()->frameSize(); > + > + MachineState machine; > + memset(&machine, 0, sizeof(machine)); > + for (GeneralRegisterIterator iter(spilled); iter.more(); iter++, spillBase++) { nit: MachineState MachineState::FromGcRegs(GeneralRegisterSet actual, GeneralRegisterSet spilled); or FromSafepoint @@ +545,4 @@ > : SnapshotReader(iter.ionScript()->snapshots() + iter.osiIndex()->snapshotOffset(), > iter.ionScript()->snapshots() + iter.ionScript()->snapshotsSize()), > fp_(iter.jsFrame()), > + machine_(iter.machineState()), nit: SnapshotIterator should have the option of not creating a machine state, or an empty one for cases such as InlineFrameIterator where the machine state creation is not necessary because we have the guarantee to find our fields on the stack or in the snapshot. ::: js/src/ion/arm/Bailouts-arm.cpp @@ +132,5 @@ > } > MachineState machine() { > + MachineState machine; > + for (unsigned i = 0; i < Registers::Total; i++) > + machine.setRegisterLocation(Register::FromCode(i), &regs_[i]); nit: This deserve a function. something like: MachineState MachineState::FromBailout(uintptr_t *regs[Registers::Total], double *fpregs[FloatRegisters::Total]);

Thanks for the quick review! (In reply to Nicolas B. Pierron [:pierron] from comment #27) > > + GeneralRegisterSet actual, spilled; > > + reader.getOsiReturnPointOffset(); > > + reader.getGcRegs(&actual, &spilled); > > I really don't like the idea that the reader is not caching the data read > once and that each function as to be knowledgeable about the underlying > structure of a safepoint. I agree, I'll do that tomorrow. > > + // Get the base address to where safepoint registers are spilled. > > + // Out-of-line calls do not unwind the extra padding space used to > > + // aggregate bailout tables, so we use frameSize instead of frameLocals, > > + // which would only account for local stack slots. > > Question: Do we care about saving this space and file a follow-up bug ? I doubt it. It's a very small amount of space (maybe 128 bytes or so). > MachineState > MachineState::FromGcRegs(GeneralRegisterSet actual, GeneralRegisterSet > spilled); > > or FromSafepoint (The memset was an accident) ; is there a need to separate out this short block of code? it is not duplicated anywhere. > nit: SnapshotIterator should have the option of not creating a machine > state, or an empty one for cases such as InlineFrameIterator where the > machine state creation is not necessary because we have the guarantee to > find our fields on the stack or in the snapshot. It should be easy to add that constructor when a use case comes up. For now InlineFrameIterators are always constructed from IonFrameIterator. > > MachineState machine() { > > + MachineState machine; > > + for (unsigned i = 0; i < Registers::Total; i++) > > + machine.setRegisterLocation(Register::FromCode(i), &regs_[i]); > > nit: This deserve a function. something like: > > MachineState > MachineState::FromBailout(uintptr_t *regs[Registers::Total], > double *fpregs[FloatRegisters::Total]); Okay.

(In reply to David Anderson [:dvander] from comment #28) > Thanks for the quick review! No problem. > (In reply to Nicolas B. Pierron [:pierron] from comment #27) > > MachineState > > MachineState::FromGcRegs(GeneralRegisterSet actual, GeneralRegisterSet > > spilled); > > > > or FromSafepoint > > (The memset was an accident) ; is there a need to separate out this short > block of code? it is not duplicated anywhere. If you add FromBailout, then I would prefer to keep all initializations method similar.

https://hg.mozilla.org/projects/ionmonkey/rev/35144214826e https://hg.mozilla.org/projects/ionmonkey/rev/830e73261538

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