Last Comment Bug 810525 - unregress DecompileValueGenerator change to handle object literal reference bases
: unregress DecompileValueGenerator change to handle object literal reference b...
Status: RESOLVED FIXED
[js:p2:fx22]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: Brendan Eich [:brendan]
:
:
Mentors:
Depends on: 831205 831846 850949
Blocks: 787283
  Show dependency treegraph
 
Reported: 2012-11-09 15:27 PST by Brendan Eich [:brendan]
Modified: 2013-03-20 15:54 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (3.70 KB, patch)
2012-11-09 15:35 PST, Brendan Eich [:brendan]
luke: review+
Details | Diff | Splinter Review
proposed fix (5.45 KB, patch)
2012-12-14 11:50 PST, Brendan Eich [:brendan]
luke: review+
Details | Diff | Splinter Review
proposed fix (10.70 KB, patch)
2013-01-04 18:52 PST, Brendan Eich [:brendan]
jdemooij: review+
Details | Diff | Splinter Review
better fix (11.26 KB, patch)
2013-01-11 17:29 PST, Brendan Eich [:brendan]
jdemooij: review+
Details | Diff | Splinter Review

Description Brendan Eich [:brendan] 2012-11-09 15:27:36 PST
These show regressions:

js> ({p:1, q:2}).m()
typein:1:1 TypeError: (intermediate value).m is not a function
js> [].m()
typein:2:0 TypeError: (intermediate value).m is not a function
js> [1,2,3].m()
typein:3:0 TypeError: (intermediate value).m is not a function
js> /hi/.m()
typein:4:0 TypeError: (intermediate value).m is not a function

With the patch attached once I have the bug#, you get this:

js> ({p:1, q:2}).m()
typein:1:1 TypeError: ({p:1, q:2}).m is not a function
js> [].m()
typein:2:0 TypeError: [].m is not a function
js> [1,2,3].m()
typein:3:0 TypeError: [1, 2, 3].m is not a function
js> /hi/.m()
typein:4:0 TypeError: /hi/.m is not a function

/be
Comment 1 Brendan Eich [:brendan] 2012-11-09 15:35:19 PST
Created attachment 680266 [details] [diff] [review]
proposed fix

Bite-sized, with test chaser!

/be
Comment 2 Luke Wagner [:luke] 2012-11-09 15:51:21 PST
Nice
Comment 3 Luke Wagner [:luke] 2012-11-12 08:31:19 PST
Comment on attachment 680266 [details] [diff] [review]
proposed fix

Stealing at Brendan's request (I reviewed the initial ExpressionDecompiler patch).
Comment 4 Brendan Eich [:brendan] 2012-11-14 10:30:51 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/f242101b1c00

/be
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-11-14 10:34:24 PST
Sorry to reopen, but the practice is to resolve the bug when it's merged over to m-c.
Comment 7 Brendan Eich [:brendan] 2012-12-14 11:42:34 PST
Sorry, I ass-u-med that no one would hardcode "(intermediate value)" into expected error message strings in the testsuite. The effort to do that was on the order of the effort to write the fix part of this bug's patch.

New patch coming up, and "I will run the testsuite" x 100 on the blackboard.

/be
Comment 8 Brendan Eich [:brendan] 2012-12-14 11:50:22 PST
Created attachment 692411 [details] [diff] [review]
proposed fix
Comment 9 Luke Wagner [:luke] 2012-12-14 14:04:06 PST
Comment on attachment 692411 [details] [diff] [review]
proposed fix

Hehe
Comment 12 Brendan Eich [:brendan] 2012-12-28 22:32:48 PST
My jit-tests passed. Hrm.

Any ideas? I'm on Mac, not Linux, but that should not matter.

Benjamin, can you reproduce the failure?

/be
Comment 13 Brendan Eich [:brendan] 2012-12-28 23:12:39 PST
Is the --no-ion option in the failing log significant? How do I enable it? Naive attempts to run jit_test.py with it get

Usage: jit_test.py [options] JS_SHELL [TESTS]

jit_test.py: error: no such option: --no-ion

/be
Comment 14 :Benjamin Peterson 2012-12-29 07:38:33 PST
 $ jit-test/jit_test.py _OPT.OBJ/js getProtot --args=-a -o
/home/benjamin/dev/mozilla/inbound/js/src/jit-test/tests/basic/testErrorReportIn_getPrototypeOf.js:9:0 Error: Assertion failed: got "TypeError: [\"kittens\", 4, 3] is not an object", expected "TypeError: \"kittens\" is not an object"
Exit code: 3
FAIL - /home/benjamin/dev/mozilla/inbound/js/src/jit-test/tests/basic/testErrorReportIn_getPrototypeOf.js
[0|1|0|0] 100% ==========================================================>|   0.0s
FAILURES:
    /home/benjamin/dev/mozilla/inbound/js/src/jit-test/tests/basic/testErrorReportIn_getPrototypeOf.js
TIMEOUTS:
Comment 15 Brendan Eich [:brendan] 2013-01-02 14:20:04 PST
Thanks.

Who do I grumble at about build logs not including exact command lines?

/be
Comment 16 Brendan Eich [:brendan] 2013-01-02 22:12:50 PST
My patch is exposing a latent bug, covered up by this bug and exposed by its fix.

When run interpreter-first (no -a option to js), this code

FindStartPC(JSContext *cx, ScriptFrameIter &iter, int spindex, int skipStackHits, Value v,
            jsbytecode **valuepc)
{
    jsbytecode *current = *valuepc;

    if (spindex == JSDVG_IGNORE_STACK)
        return true;

    *valuepc = NULL;

    PCStack pcstack;
    if (!pcstack.init(cx, iter.script(), current))
        return false;

    if (spindex == JSDVG_SEARCH_STACK) {
        size_t index = iter.numFrameSlots();

gets a pcstack.depth_ = 4 and index = iter.numFrameSlots() of 4, which agree. The stack is from this line of the test:

    Object.getPrototypeOf.apply(null, ["kittens",4,3])

and so has a pcstack of [callprop<'apply'>, getprop<'getPrototypeOf'>, null, object] (these are JSOP_* names, lowercased). The value wanted, the non-callable "kittens", does not match any operand stack value, so the DVG attempt falls back and the test passes.

With -a, pcstack.depth_ is 4 but index is 6 -- the operand stack and pcstack are not the same size (which is a bad bug, suggesting an assertion in this code that they match).

How does skipping the interpreter result in a difference in iter.numFrameSlots()? dvander, bhackett: little help?

/be
Comment 17 Jan de Mooij [:jandem] 2013-01-03 01:03:23 PST
JM optimizes Function.prototype.{apply, call} to call the target function directly, I'm pretty sure that's what's going on here. See SplatApplyArgs in MonoIC.cpp.

Now that we have IonMonkey it's probably fine to disable/remove this optimization from JM (v8-raytrace is the main benchmark that depends on fast fun.apply).
Comment 18 Jan de Mooij [:jandem] 2013-01-03 01:17:37 PST
(To verify this is indeed the problem, you can just make IsLowerableFunCallOrApply always return false).
Comment 19 Brendan Eich [:brendan] 2013-01-03 10:46:24 PST
Jan: thanks. What I'd like to do is flag the stack frame or something (is there a frame?) so the expression decompiler can fall back. Thoughts?

/be
Comment 20 Jan de Mooij [:jandem] 2013-01-04 01:23:34 PST
(In reply to Brendan Eich [:brendan] from comment #19)
> Jan: thanks. What I'd like to do is flag the stack frame or something (is
> there a frame?) so the expression decompiler can fall back. Thoughts?

There is a StackFrame. I'm not sure if it's worth fixing this though, especially considering our plans to remove JM. Note that IonMonkey also breaks DecompileValueGenerator in many cases, because we can't always fully reconstruct the interpreter stack. I think iter.numFrameSlots() may be != pcstack.depth, too (iter.numFrameSlots() looks at snapshots and these may encode the stack state at another pc).
Comment 21 Jan de Mooij [:jandem] 2013-01-04 01:40:04 PST
If you just want to detect the optimized call/apply case, you can probably use StackFrame::loweredCallOrApply():

if (!iter.isIon() && iter.interpFrame()->loweredCallOrApply())
    ...
Comment 22 Brendan Eich [:brendan] 2013-01-04 08:08:34 PST
(In reply to Jan de Mooij [:jandem] from comment #20)
> (In reply to Brendan Eich [:brendan] from comment #19)
> > Jan: thanks. What I'd like to do is flag the stack frame or something (is
> > there a frame?) so the expression decompiler can fall back. Thoughts?
> 
> There is a StackFrame. I'm not sure if it's worth fixing this though,
> especially considering our plans to remove JM.

The latent bug is a bug, it needs fixing now.

JIT work should not regress error message quality, separate point (the point of this bug report, whose patch is blocked by the latent bug).

> Note that IonMonkey also
> breaks DecompileValueGenerator in many cases,

With what results? I trust not memory-unsafe crashes.

Sub-optimal error message quality (fallback, in DVG terms) might be ok. If the interpreter always runs first and the error is deterministic and inevitable. But runtime errors generally aren't.

BTW, what are the JS_MORE_DETERMINISTIC #ifdefs about? I can't see that defined anywhere.

Someone needs to own high-quality diagnostics, not treat it as some cruft from the past to incrementally degrade, break, leave latently-broken in memory-unsafe ways. It's part of the job, along with making awesome JITs.

> because we can't always fully
> reconstruct the interpreter stack. I think iter.numFrameSlots() may be !=
> pcstack.depth, too (iter.numFrameSlots() looks at snapshots and these may
> encode the stack state at another pc).

Bugs on file?

(In reply to Jan de Mooij [:jandem] from comment #21)
> If you just want to detect the optimized call/apply case, you can probably
> use StackFrame::loweredCallOrApply():
> 
> if (!iter.isIon() && iter.interpFrame()->loweredCallOrApply())

Will try it -- thanks very much for the tip.

/be
Comment 23 Jan de Mooij [:jandem] 2013-01-04 11:23:42 PST
(In reply to Brendan Eich [:brendan] from comment #22)
> > Note that IonMonkey also
> > breaks DecompileValueGenerator in many cases,
> 
> With what results? I trust not memory-unsafe crashes.

It shouldn't crash, iter.numFrameSlots() may be different, but the DVG code does not depend on that (last I looked at it, correct me if I'm wrong!).

> BTW, what are the JS_MORE_DETERMINISTIC #ifdefs about? I can't see that
> defined anywhere.

The correctness fuzzers compare the output of two runs (for instance JIT vs. interpreter) to find bugs. These builds are configured with --enable-more-deterministic and JS_MORE_DETERMINISTIC is used in various places to ensure consistent output (e.g. the result of the gc() function is deterministic with more-deterministic builds).

> Someone needs to own high-quality diagnostics, not treat it as some cruft
> from the past to incrementally degrade, break, leave latently-broken in
> memory-unsafe ways. It's part of the job, along with making awesome JITs.

OK, point taken.

> Bugs on file?

See bug 755813, I don't think there are other bugs on file.
Comment 24 Brendan Eich [:brendan] 2013-01-04 16:23:15 PST
(In reply to Jan de Mooij [:jandem] from comment #23)
> (In reply to Brendan Eich [:brendan] from comment #22)
> > > Note that IonMonkey also
> > > breaks DecompileValueGenerator in many cases,
> > 
> > With what results? I trust not memory-unsafe crashes.
> 
> It shouldn't crash, iter.numFrameSlots() may be different, but the DVG code
> does not depend on that (last I looked at it, correct me if I'm wrong!).

With my fix to handle more JSOps in ExpressionDecompiler, you get wrong results. I hope there's no crash bug, but I'm not convinced yet.

> > BTW, what are the JS_MORE_DETERMINISTIC #ifdefs about? I can't see that
> > defined anywhere.
> 
> The correctness fuzzers compare the output of two runs (for instance JIT vs.
> interpreter) to find bugs. These builds are configured with
> --enable-more-deterministic and JS_MORE_DETERMINISTIC is used in various
> places to ensure consistent output (e.g. the result of the gc() function is
> deterministic with more-deterministic builds).

Right, but why the #ifdefs in jsopcode.cpp? Maybe cuz of the error message quality shifting depending on interp vs. jit.

> > Bugs on file?
> 
> See bug 755813, I don't think there are other bugs on file.

Thanks.

/be
Comment 25 Brendan Eich [:brendan] 2013-01-04 18:52:14 PST
Created attachment 698198 [details] [diff] [review]
proposed fix

Had to add a StackFrame::Flags bit, BUMPED_STACK, to propagate the fact that SplatApplyArgs farbled sp from methodjit-time to runtime.

This passes `./jit_test.py --args='-a' ../Darwin_DBG.OBJ/js`.

/be
Comment 26 Brendan Eich [:brendan] 2013-01-05 22:14:29 PST
I should've use FARBLED_STACK ;-). Perhaps the plainest and clearest name would be JIT_REVISED_STACK to indicate that bytecode analysis cannot map the stack any longer. Comments welcome.

/be
Comment 27 Jan de Mooij [:jandem] 2013-01-07 02:47:40 PST
Comment on attachment 698198 [details] [diff] [review]
proposed fix

Review of attachment 698198 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, r=me with comments below addressed.

::: js/src/jsopcode.cpp
@@ +6177,5 @@
> +     * the methodjit has "splatted" the arguments array, bumping the caller's
> +     * stack pointer and skewing it from what static analysis in pcstack.init
> +     * would compute.
> +     */
> +    if (iter.interpFrame()->bumpedStack())

interpFrame() asserts !isIon, so this should be

if (!iter.isIon() && iter.interpFrame()->bumpedStack())

@@ +6188,5 @@
>          return false;
>  
>      if (spindex == JSDVG_SEARCH_STACK) {
>          size_t index = iter.numFrameSlots();
> +        JS_ASSERT(pcstack.depth() == index);

This doesn't hold with Ion (new failures with jit_test.py --ion $shell). Either remove the assert or turn it into JS_ASSERT_IF(!iter.isIon(), ..);

::: js/src/vm/Stack.h
@@ +277,5 @@
>          /* Ion frame state */
>          RUNNING_IN_ION       = 0x400000,  /* frame is running in Ion */
> +        CALLING_INTO_ION     = 0x800000,  /* frame is calling into Ion */
> +
> +        BUMPED_STACK        = 0x1000000,  /* sp bumped for lowered apply */

Either JIT_BUMPED_STACK or JIT_REVISED_STACK is fine with me.
Comment 28 Brendan Eich [:brendan] 2013-01-07 11:08:58 PST
(In reply to Jan de Mooij [:jandem] from comment #27)
> @@ +6177,5 @@
> > +     * the methodjit has "splatted" the arguments array, bumping the caller's
> > +     * stack pointer and skewing it from what static analysis in pcstack.init
> > +     * would compute.
> > +     */
> > +    if (iter.interpFrame()->bumpedStack())
> 
> interpFrame() asserts !isIon, so this should be
> 
> if (!iter.isIon() && iter.interpFrame()->bumpedStack())

Aha, thanks -- but see below.

> 
> @@ +6188,5 @@
> >          return false;
> >  
> >      if (spindex == JSDVG_SEARCH_STACK) {
> >          size_t index = iter.numFrameSlots();
> > +        JS_ASSERT(pcstack.depth() == index);
> 
> This doesn't hold with Ion (new failures with jit_test.py --ion $shell).
> Either remove the assert or turn it into JS_ASSERT_IF(!iter.isIon(), ..);

Rather, I think the if you corrected above should be

if (iter.isIon() || iter.interpFrame()->bumpedStack())

to be safe. This may degrade error messages from runtime in ion-jitted code, though. It may be that there's no memory safety issue with diverged operand value vs. operand generating pc stacks. Could you say more about how ion revises the stack?

/be
Comment 29 Jan de Mooij [:jandem] 2013-01-08 04:18:25 PST
(In reply to Brendan Eich [:brendan] from comment #28)
> Rather, I think the if you corrected above should be
> 
> if (iter.isIon() || iter.interpFrame()->bumpedStack())

That's fine with me.

> to be safe. This may degrade error messages from runtime in ion-jitted code,
> though. It may be that there's no memory safety issue with diverged operand
> value vs. operand generating pc stacks. Could you say more about how ion
> revises the stack?

The problem is that IonMonkey uses a snapshot that may encode the stack state at a previous pc, so that's why the stack depth doesn't necessarily match the current pc's depth. I don't think there are safety issues here, but we may get bogus error messages, so maybe using the fallback path for Ion frames is indeed the right thing to do.
Comment 30 Brendan Eich [:brendan] 2013-01-08 18:31:19 PST
(In reply to Jan de Mooij [:jandem] from comment #29)
> (In reply to Brendan Eich [:brendan] from comment #28)
> > Rather, I think the if you corrected above should be
> > 
> > if (iter.isIon() || iter.interpFrame()->bumpedStack())
> 
> That's fine with me.
> 
> > to be safe. This may degrade error messages from runtime in ion-jitted code,
> > though. It may be that there's no memory safety issue with diverged operand
> > value vs. operand generating pc stacks. Could you say more about how ion
> > revises the stack?
> 
> The problem is that IonMonkey uses a snapshot that may encode the stack
> state at a previous pc,

Any breadcrumbs we could leave to fix this, if not use a snapshot for the current pc?

/be
Comment 31 Brendan Eich [:brendan] 2013-01-09 02:48:40 PST
Jan: I get two failures still, even with the change mentioned in comment 28, both on jit-test/tests/for-of/non-iterable.js. The two options blamed are --no-jm and --ion-eager, and iter.isIon() is false for both cases. pcstack.depth_ is 1. index = iter.numFrameSlots() is 2. This seems unsafe because we could index out of bounds in pcstack.

Any suggestions? If IonMonkey is minimizing snapshots for good Ion reasons, but there's a way to tell from jsopcode.cpp code that cares, that would suffice. Checking equality of index and pcstack.depth() is possible but seems not sufficient in general.

/be
Comment 32 Jan de Mooij [:jandem] 2013-01-09 04:08:51 PST
(In reply to Brendan Eich [:brendan] from comment #31)
> Jan: I get two failures still, even with the change mentioned in comment 28,
> both on jit-test/tests/for-of/non-iterable.js. The two options blamed are
> --no-jm and --ion-eager, and iter.isIon() is false for both cases.

The test does not fail without these flags? I just checked, IonMonkey doesn't compile anything on that test with --no-jm (it runs in the interpreter).

> Any suggestions? If IonMonkey is minimizing snapshots for good Ion reasons,
> but there's a way to tell from jsopcode.cpp code that cares, that would
> suffice. Checking equality of index and pcstack.depth() is possible but
> seems not sufficient in general.

Yeah, there are good reasons to minimize snapshots (they keep values alive so more regalloc pressure, etc). It's hard to tell from jsopcode.cpp because many ops are compiled differently, based on type information. I think your proposal to check for iter.isIon() is the right thing to do.
Comment 33 Brendan Eich [:brendan] 2013-01-09 06:58:10 PST
(In reply to Jan de Mooij [:jandem] from comment #32)
> (In reply to Brendan Eich [:brendan] from comment #31)
> > Jan: I get two failures still, even with the change mentioned in comment 28,
> > both on jit-test/tests/for-of/non-iterable.js. The two options blamed are
> > --no-jm and --ion-eager, and iter.isIon() is false for both cases.
> 
> The test does not fail without these flags? I just checked, IonMonkey
> doesn't compile anything on that test with --no-jm (it runs in the
> interpreter).

Weird -- the bug is for code of this form

    NAME           //+1 =1
    ITER           //+0 =1
    GOTO L         //+0 =1
    LOOPHEAD       //+0 =1
    ITERNEXT       //+1 =2
    SETLOCAL       //+0 =2
    POP            //-1 =1
    JSOP_STRING    //+1 =2
    JSOP_THROW     //-1 =1
L:  JSOP_MOREITER  //+1 =2

pcstack.depth_ is 1 as expected at label L from the bytecode. But iter.numFrameSlots() is 2, and I stepped into it to verify it was the SCRIPTED case.

It seems Ion is not running. Not sure what's going wrong just yet, leaving this here in case it triggers a memory.

/be
Comment 34 Brendan Eich [:brendan] 2013-01-11 05:36:57 PST
Duh, the reason the stack depth is 2 when the error is being reported is super-simple: JSOP_MOREITER does a PUSH_NULL() right away, so any error from its implementation will find the post-op stack depth, not the pre-op that is expected by bytecode analysis up to but not including that op.

I will weaken the assertion to

        JS_ASSERT(pcstack.depth() <= index);

/be
Comment 35 Brendan Eich [:brendan] 2013-01-11 16:08:34 PST
Whew, no safety problem because of the check

        if (index < size_t(pcstack.depth()))
            *valuepc = pcstack[index];

after index has been decremented in the JSDVG_SEARCH_STACK case. However, there's no else clause, which means *valuepc is null on true return, which foils decompiling from the current pc.

I dug into the CVS history to find out how this used to work. Old code used to check more carefully, e.g., see http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=jsopcode.c&branch=&root=/cvsroot&subdir=mozilla/js/src&command=DIFF_FRAMESET&rev1=3.296&rev2=3.297, the comment "This happens when the value comes from a temporary slot that the interpreter uses for GC roots. Assume that it is fp->pc that caused the exception."

That comment misses that the slot may not be temporary (JOF_TMPSLOT), it may be an "output" operand stack of the opcode, but it does the right thing: blames the current pc.

Over time this code changed and safety was lost. I didn't track down the exact revision, I'm out of time right now.

New patch anon.

/be
Comment 36 Brendan Eich [:brendan] 2013-01-11 16:14:05 PST
(In reply to Brendan Eich [:brendan] from comment #35)
> Over time this code changed and safety was lost.

Bleargh, jetlag. s/safety/current-pc decompilation instead of fallback/

/be
Comment 37 Brendan Eich [:brendan] 2013-01-11 17:29:51 PST
Created attachment 701379 [details] [diff] [review]
better fix

I'll fix the FIXME to cite a new bug before committing.

If Ion's snapshot is for a previous pc, is there ever a bail-out to the interpreter at that snapshot? I guess not, otherwise you might get double effects.

If only the decompiler can suffer from a snapshot for a previous pc, what are snapshots for?

If there's an effect-free guarantee that the previous bytecode has no effects, then perhaps the decompiler can interpret its way forward to the current pc.

Jan, can you school me on Ion's snapshot uses? Thanks.

/be
Comment 38 Brendan Eich [:brendan] 2013-01-11 17:30:36 PST
Pushed to try, I hope I used the right options to conserve build farm machines but cover the necessary test surface:

https://tbpl.mozilla.org/?tree=Try&rev=1a3aa108e25e

/be
Comment 39 Jan de Mooij [:jandem] 2013-01-14 01:57:24 PST
Comment on attachment 701379 [details] [diff] [review]
better fix

Review of attachment 701379 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brendan Eich [:brendan] from comment #37)
> If Ion's snapshot is for a previous pc, is there ever a bail-out to the
> interpreter at that snapshot? I guess not, otherwise you might get double
> effects.

An LCallGeneric instruction (JSOP_CALL) can have a snapshot for a previous pc:

pc0 JSOP_X
pc1 JSOP_Y
pc2 JSOP_Z
pc3 JSOP_CALL (snapshot for pc0)

This snapshot will be used when we bailout to the interpreter because the callee is not a function or something similar, but only before we have done anything effectful. It would be wrong to bailout and use the snapshot after we have done something effectful, because that would be observable from JS.

Effectful operations can also have a post-snapshot that's used if the call triggers invalidation of it's caller. Using that snapshot here is wrong though (the register allocator may insert register/memory moves between the effectful instruction and the point where we capture this snapshot), and it will be very hard to reconstruct the stack state from it.

> If there's an effect-free guarantee that the previous bytecode has no
> effects, then perhaps the decompiler can interpret its way forward to the
> current pc.

See the CALL example above: the instructions before the call have no side-effects, but when we end up in DVG we may have done something effectful as part of the JSOP_CALL.
Comment 40 Brendan Eich [:brendan] 2013-01-14 16:20:37 PST
(In reply to Jan de Mooij [:jandem] from comment #39)
> This snapshot will be used when we bailout to the interpreter because the
> callee is not a function or something similar, but only before we have done
> anything effectful. It would be wrong to bailout and use the snapshot after
> we have done something effectful, because that would be observable from JS.

You bet!

> Effectful operations can also have a post-snapshot that's used if the call
> triggers invalidation of it's caller. Using that snapshot here is wrong
> though (the register allocator may insert register/memory moves between the
> effectful instruction and the point where we capture this snapshot), and it
> will be very hard to reconstruct the stack state from it.

DVG won't come into play with the call (callee) expression as the base of an object reference until after the call returns, so no worries.

> > If there's an effect-free guarantee that the previous bytecode has no
> > effects, then perhaps the decompiler can interpret its way forward to the
> > current pc.
> 
> See the CALL example above: the instructions before the call have no
> side-effects, but when we end up in DVG we may have done something effectful
> as part of the JSOP_CALL.

That's ok, DVG doesn't analyze into calls, just callee expressions that result in non-callables (so the error is being thrown under JSOP_CALL), or post-call expressions for foo.bar().baz where the return value is null or undefined.

This iter.isIon() fallback bug seems fixable, I'll file it and note it in the FIXME. Thanks!

/be
Comment 42 Brendan Eich [:brendan] 2013-01-15 19:23:51 PST
Filed followup bug 831120.

/be
Comment 43 Ed Morley [:emorley] 2013-01-16 12:31:08 PST
https://hg.mozilla.org/mozilla-central/rev/39885ae3a597

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