Last Comment Bug 732852 - IonMonkey: Assertion failure: throwing, at jscntxt.h:1178
: IonMonkey: Assertion failure: throwing, at jscntxt.h:1178
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
: 738537 741204 743138 (view as bug list)
Depends on:
Blocks: jsfunfuzz langfuzz IonFuzz 740212
  Show dependency treegraph
 
Reported: 2012-03-04 17:55 PST by Christian Holler (:decoder)
Modified: 2013-02-07 05:18 PST (History)
6 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: split Register/FloatRegister into new header (40.26 KB, patch)
2012-03-21 19:43 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review
part 2: split Snapshots.h (14.68 KB, patch)
2012-03-21 19:44 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review
part 3: refactor SnapshotIterator/SnapshotReader (21.89 KB, patch)
2012-03-21 19:46 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review
part 4: move SnapshotIterator to IonFrameIterator.h (3.48 KB, patch)
2012-03-29 19:32 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review
part 5: remove most uses of FrameRecovery (18.32 KB, patch)
2012-03-29 19:36 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review
part 6: refactor InlineFrameIterator (15.61 KB, patch)
2012-03-29 19:40 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
part 8: fix actual bug (10.07 KB, patch)
2012-03-30 00:11 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
part 6: new InlineFrameIterator (15.48 KB, patch)
2012-04-02 14:32 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review
part 7: ensure that iterators are actually closed (10.95 KB, patch)
2012-04-04 20:12 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
part 7: ensure that iterators are actually closed (11.01 KB, patch)
2012-04-04 20:15 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review
part 8: new MachineState (26.32 KB, patch)
2012-04-08 21:48 PDT, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-03-04 17:55:50 PST
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")
}
Comment 1 David Anderson [:dvander] 2012-03-12 18:14:10 PDT
This is a bug with our cx->enumerators handling, which is broken anyway, so I'll do a full fix here.
Comment 2 David Anderson [:dvander] 2012-03-21 19:43:59 PDT
Created attachment 608201 [details] [diff] [review]
part 1: split Register/FloatRegister into new header

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.
Comment 3 David Anderson [:dvander] 2012-03-21 19:44:49 PDT
Created attachment 608202 [details] [diff] [review]
part 2: split Snapshots.h

Splits Snapshots.h into SnapshotReader.h and SnapshotWriter.h
Comment 4 David Anderson [:dvander] 2012-03-21 19:46:43 PDT
Created attachment 608203 [details] [diff] [review]
part 3: refactor SnapshotIterator/SnapshotReader

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 5 Nicolas B. Pierron [:nbp] 2012-03-22 13:35:30 PDT
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 6 Nicolas B. Pierron [:nbp] 2012-03-22 14:43:32 PDT
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);
Comment 7 David Anderson [:dvander] 2012-03-22 21:01:46 PDT
*** Bug 738537 has been marked as a duplicate of this bug. ***
Comment 8 David Anderson [:dvander] 2012-03-29 19:32:12 PDT
Created attachment 610792 [details] [diff] [review]
part 4: move SnapshotIterator to IonFrameIterator.h
Comment 9 David Anderson [:dvander] 2012-03-29 19:36:14 PDT
Created attachment 610795 [details] [diff] [review]
part 5: remove most uses of FrameRecovery

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.
Comment 10 David Anderson [:dvander] 2012-03-29 19:40:04 PDT
Created attachment 610796 [details] [diff] [review]
part 6: refactor InlineFrameIterator

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.)
Comment 11 David Anderson [:dvander] 2012-03-30 00:11:35 PDT
Created attachment 610816 [details] [diff] [review]
part 8: fix actual bug

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 12 Nicolas B. Pierron [:nbp] 2012-03-30 14:04:54 PDT
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 13 Nicolas B. Pierron [:nbp] 2012-03-30 14:33:52 PDT
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 14 Nicolas B. Pierron [:nbp] 2012-03-30 14:57:20 PDT
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.
Comment 15 David Anderson [:dvander] 2012-03-30 15:08:30 PDT
> ::: 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.
Comment 16 David Anderson [:dvander] 2012-03-30 15:10:11 PDT
> 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.
Comment 17 David Anderson [:dvander] 2012-03-30 15:12:24 PDT
> @@ +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.
Comment 18 David Anderson [:dvander] 2012-04-02 13:30:13 PDT
(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
Comment 19 David Anderson [:dvander] 2012-04-02 13:47:51 PDT
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)
Comment 20 David Anderson [:dvander] 2012-04-02 14:32:15 PDT
Created attachment 611595 [details] [diff] [review]
part 6: new InlineFrameIterator

(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.
Comment 21 David Anderson [:dvander] 2012-04-02 18:14:42 PDT
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
Comment 22 David Anderson [:dvander] 2012-04-04 20:12:19 PDT
Created attachment 612437 [details] [diff] [review]
part 7: ensure that iterators are actually closed

last patch before MachineState refactoring
Comment 23 David Anderson [:dvander] 2012-04-04 20:15:24 PDT
Created attachment 612438 [details] [diff] [review]
part 7: ensure that iterators are actually closed

err, qref'd version
Comment 24 Nicolas B. Pierron [:nbp] 2012-04-05 10:38:42 PDT
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.
Comment 25 David Anderson [:dvander] 2012-04-05 20:49:34 PDT
*** Bug 743138 has been marked as a duplicate of this bug. ***
Comment 26 David Anderson [:dvander] 2012-04-08 21:48:49 PDT
Created attachment 613224 [details] [diff] [review]
part 8: new MachineState

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 27 Nicolas B. Pierron [:nbp] 2012-04-08 22:59:34 PDT
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]);
Comment 28 David Anderson [:dvander] 2012-04-09 00:15:08 PDT
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.
Comment 29 Nicolas B. Pierron [:nbp] 2012-04-09 00:29:59 PDT
(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.
Comment 31 Christian Holler (:decoder) 2012-04-18 06:17:53 PDT
*** Bug 741204 has been marked as a duplicate of this bug. ***
Comment 32 Christian Holler (:decoder) 2013-02-07 05:18:52 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397

Note You need to log in before you can comment on or make changes to this bug.