716647 - [jsdbg2] allow Debugger to be enabled with debuggee frames on the stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This is a huge usability hurdle.

Per bhackett, we just have to do two things:

1. When enabling debug mode, purge all jitcode (if we don't do it already)

2. Search the stack for torn values (junk left in a variable by JIT code because the variable is never used) and set them to undefined.

Sounds easy, right?

Comment 1

[Jason Orendorff [:jorendorff]]

We already purge jitcode when entering debug mode. The path is:

Debugger::addDebuggeeGlobal
 -> JSCompartment::addDebuggee
  -> JSCompartment::updateForDebugMode
     which loops over all scripts calling mjit::ReleaseScriptCode

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

I haven't written the code to cope with torn values yet. But here's the thing. I can't get a test to fail.

Now the test in this patch, debug-mode-04.js, does partly produce this situation; the debugger finds that `unused === undefined` if inference is enabled, and
`unused === 42` otherwise. But I want a test that will fail or crash with this patch, but pass once I actually implement torn value fixup.

Maybe the fuzzers could help with this.

Comment 3

[Brian Hackett [Laid off!]]

I think for debug-mode-04.js you might want to call g.j several times, with -m -n -a.  If it is only called once the interpreter will I think push the stack frame and set the local variables to undefined first.  On later calls where both the caller and callee are JIT'ed, no initialization should be done for the callee's locals.

Comment 4

[Christian Holler (:decoder)]

Fuzzing this on external host right now (with all tests). I'll check the results tomorrow and if we can't find anything that way, we can still specialize on the jsdbg2 tests.

Comment 5

[Jim Blandy :jimb]

Perhaps, for testing, the shell should offer an option to fill dead values with a distinctive value. Then, the debugger tests could check for sure that dead variables got that value. It's certainly testing too much, but it seems like a good fit for the implementation we've actually chosen.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: WIP 2

decoder, could you try this one?

Comment 7

[Christian Holler (:decoder)]

Okay :) Testing the new patch on m-c revision 7edc6c4cf83d now.

Comment 8

[Christian Holler (:decoder)]

I've been running this for quite a while now and haven't found anything more specific to this patch.

Comment 9

[Jim Blandy :jimb]

Bug 846985 is yet another application of Debugger foiled by this restriction.

Comment 10

[Jan de Mooij [:jandem]]

Here's what we have to do when we turn on debug mode with scripts on the stack:

------
(1) Recompile all Baseline scripts on the stack and patch the return address into these scripts. This is necessary for breakpoints to work. Note that this will be much simpler than the bailout mechanism in Ion (or the one JM had) since we're not modifying the frame itself (or kicking it into the interpreter).

All Baseline scripts that are not on the stack can be discarded.

(2) Invalidate all Ion scripts. This will ensure no Ion code is executed after we return to the Ion-frames that are currently on the stack.

(3) For every Ion frame on the stack, allocate a new structure (IonDebuggerFrame or something) that contains a copy of the frame's locals, arguments etc. See also (4).

We can add a third state to AbstractFramePtr so that an AbstractFramePtr can point to an IonDebuggerFrame. We also need a way to map from a (possibly inlined) Ion frame to its IonDebuggerFrame.

After we return to the Ion frame, it will bailout immediately, because of (2). The bailout process can use the IonDebuggerFrame for any locals/arguments that were modified by the debugger.

(4) To allow recovering a frame's locals/arguments from within a VM call, we have to guarantee the register allocator won't insert any move groups between the instruction itself and the LOsiPoint that follows it. We also have to audit all places where we perform a VM call (especially OOL VM calls) to make sure they don't mess with any registers other than temps/defs after performing the VM call.

We also need (4) for try-catch support. Recovering the expression stack from VM calls is harder, but locals/arguments should work I think. Locals/arguments can be optimized away but I don't expect that to be a huge problem when turning on debug mode.
------

I don't know when/if I'll have time for this, but I can probably prototype something in a few days and see if the approach works...

Comment 11

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> Locals/arguments can be optimized away but I don't expect that to be a huge
> problem when turning on debug mode.

Just as a quick note: Chrome doesn't always preserve all locals in the debugger, either. In practice, that isn't too bad.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: WIP -10

Here's a very hackish prototype to implement what I described in comment 10.

It's now possible to turn on the debugger with Baseline and Ion frames on the stack. The debugger can do everything it wants with these frames: inspect and modify locals/arguments, use eval-in-frame, set breakpoints etc.

For Ion frames, the debugger uses a copy of the frame's locals/arguments (the IonDebuggerFrame I described in comment 10) and when we bailout from Ion to Baseline we copy these values to the BaselineFrame, so that the script will resume with these modified values.

This needs a lot of cleanup, fixes, comments and tests, but it's promising so far.

Comment 13

[Jan de Mooij [:jandem]]

What will be most complicated I think is the debugger triggering a "forced return" of an inlined Ion-frame from an onUnwindException hook. In this case we basically want to trigger a bailout from the exception handling code; I'm not sure if adding all that is really worth the complexity.

OTOH, there's some overlap with try-catch here - try-catch will need a similar mechanism (though try-catch is a bit simpler because we can forbid inlining of functions containing try statements). Anyway, I should probably attack try-catch first before finishing this.

Comment 14

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #13)
> What will be most complicated I think is the debugger triggering a "forced
> return" of an inlined Ion-frame from an onUnwindException hook.

If supporting certain onUnwindException resumption values would block fixing this bug, it's completely fine to file that as a follow-up.

Comment 15

[Jim Blandy :jimb]

Would it be possible to put off recompiling scripts or invalidating them until we actually touch them with the debugger, either by creating a Debugger.Frame visiting them, or by setting breakpoints on them?

In general, most debugging is just inspection / observation. The faster we can make that, the more useful Debugger will be.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Jim Blandy :jimb from comment #15)
> Would it be possible to put off recompiling scripts or invalidating them
> until we actually touch them with the debugger, either by creating a
> Debugger.Frame visiting them, or by setting breakpoints on them?

Yeah that should be possible. I think we could run Ion code in debug mode as long as you don't add breakpoints or enable step mode etc. And actually, starting with that may be a good way to use the existing jit-tests for testing Ion interaction with the debugger...

Comment 17

[Shu-yu Guo [:shu]]

Attachment: Part 1: Refactor readFrameArgs in Ion frame iterators. (r=?)

Moving patches to this bug as patches on bug 923724 has morphed into this one.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: Part 2: Bailout in place instead of directly to catch on Ion exception when Debugger is on. (r=?)

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Part 3: Support rematerializing Ion frames on the stack. (r=?)

Comment 20

[Shu-yu Guo [:shu]]

Attachment: Part XX: Return JS_ION_OPTIMIZED_OUT magic values for optimized out values from JIT frames. (r=?)

The Ion changes are trivial and the rest is Debugger changes.

Comment 21

[Shu-yu Guo [:shu]]

Comment on attachment 825076 [details] [diff] [review]
Part 1: Refactor readFrameArgs in Ion frame iterators. (r=?)

Carrying r=jandem.

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 825078 [details] [diff] [review]
Part 2: Bailout in place instead of directly to catch on Ion exception when Debugger is on. (r=?)

Carrying r=jandem.

Comment 23

[Shu-yu Guo [:shu]]

Comment on attachment 825079 [details] [diff] [review]
Part 3: Support rematerializing Ion frames on the stack. (r=?)

Carrying r=jandem.

Comment 25

[Shu-yu Guo [:shu]]

Preliminary working test:

>8--------------------------------------------------------------------------------------------
shu@pikashu ~/moz/inbound/js/src/Debug32
% cat ../jit-test/tests/debug/Frame-04.js 
// The debugger can inspect a global's frames that were on the stack before a
// it was added as a debuggee.

Debugger.Magic.prototype.toString = function() { return this.why; }

var g = newGlobal();
g.dbg = new Debugger;

// Spin up an empty loop to force jit.
g.eval("function f(x,d) { for (var i = 0; i < 5000; i++); g(43,d); }");
g.eval("function g(x,d) { for (var i = 0; i < 5000; i++); h(44,d); }");
g.eval("" + function h(x, d) {
  if (!d) return;

  dbg.addDebuggee(this);
  var frame = dbg.getNewestFrame();

  print(frame.mode);
  assertEq(frame.live, true);
  print(frame.arguments[0]);

  frame = frame.older;
  print(frame.mode);
  assertEq(frame.live, true);
  print(frame.script.startLine)
  print(frame.arguments[0]);

  frame = frame.older;
  print(frame.mode);
  assertEq(frame.live, true);
  print(frame.script.startLine)
  print(frame.arguments[0]);
});
g.eval("f(42); f(42, true);");
shu@pikashu ~/moz/inbound/js/src/Debug32
% ./js ../jit-test/tests/debug/Frame-04.js
[Scripts] Analyzing script ../jit-test/tests/debug/Frame-04.js:10 (0xf6841300) (usecount=1100)
[Scripts] Analyzing script ../jit-test/tests/debug/Frame-04.js:11 (0xf6841380) (usecount=1100)
[Scripts] Analyzing script ../jit-test/tests/debug/Frame-04.js:10 (0xf6841300) (usecount=1100)
[Scripts] Analyzing script ../jit-test/tests/debug/Frame-04.js:11 (0xf6841380) (usecount=1100)
interpreter
44
optimized
11
<optimized out>
optimized
10
<optimized out>

>8--------------------------------------------------------------------------------------------

Interesting thing here is that since the argument x in both f and g is unused, it gets optimized away. The debugger is reading Snapshots using exactly the same functions as Ion uses to bailout to Baseline. So, x *isn't recoverable in the general case*, period.

I hope this isn't too big of a usability problem. It will only happen when inspecting live Ion frames, but will unfortunately happen in both the semantics preserving mode and the JIT inspection mode.

Comment 26

[Shu-yu Guo [:shu]]

FWIW, there will invariably be dead things that are uninspectable. Chrome can't inspect free variables from closures that have no uses.

Comment 27

[Shu-yu Guo [:shu]]

Attachment: Part XX: Return JS_ION_OPTIMIZED_OUT magic values for optimized out values from JIT frames. (r=?)

Accidentally left some cruft in the last version.

Comment 28

[Jim Blandy :jimb]

I'm not sure we want to expose magic values this way. I think it would be better to throw an exception - but adding new exception types brings its own complications. Let's talk about this on #jsapi with jorendorff.

Comment 29

[Jim Blandy :jimb]

For example, when we start allowing Debugger.Frame.prototype.eval, won't those magic values end up being touched directly by JS?

Comment 30

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #29)
> For example, when we start allowing Debugger.Frame.prototype.eval, won't
> those magic values end up being touched directly by JS?

The magic value reflections are readonly, and so far only ION_OPTIMIZED_OUT can be reflected. Why is it a problem if they're touched?

As for whether the change I made in Ion is okay, it's fine, or even preferred. Before, any optimized out values were just encoded as undefined and decoded as undefined as well, when we bail out from Ion to Baseline or the interpreter. So, if we accidentally used an optimized out value (which isn't supposed to be possible since the only way we can optimize something out is because it's unused), there's no indication that we did something wrong, since the provenance of that particular undefined is lost. Now, if we accidentally use an optimized out value, hopefully we'll get an assert on leaking a magic value.

Comment 31

[Jan de Mooij [:jandem]]

I was just thinking, once this bug is fixed it will be possible to turn on the debugger with asm.js/Odin code on the stack. I don't know how we want to handle that, but we should at least add tests and make sure it doesn't crash :)

Comment 32

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #31)
> I was just thinking, once this bug is fixed it will be possible to turn on
> the debugger with asm.js/Odin code on the stack. I don't know how we want to
> handle that, but we should at least add tests and make sure it doesn't crash

In what way is the resulting code strange? OdinMonkey just produces valid IonMonkey compiled scripts, right?

Comment 33

[Jim Blandy :jimb]

(I certainly agree we should have some test coverage.)

Comment 34

[Luke Wagner [:luke]]

(In reply to Jim Blandy :jimb from comment #32)
Nope, Odin compilation produces an AsmJSModule (one for the whole outer asm.js module function).  Currently, asm.js calls are totally absent from the call stack (they don't show up in ScriptFrameIter or anywhere else).  The top TODO on the list once caching is done is to fix this so that asm.js shows up in Error.stack.

Anyhow, Odin is explicitly disabled by debugMode (see EstablishPreconditions in AsmJS.cpp); it'll take a lot more work to make it work with the debugger since so many of the supporting data structures (snapshots, safe points, etc) just aren't created at all.

Comment 35

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #28)
> I'm not sure we want to expose magic values this way. I think it would be
> better to throw an exception - but adding new exception types brings its own
> complications. Let's talk about this on #jsapi with jorendorff.

In IRC conversation, shu and I agreed:

- It makes sense for eval-in-debuggee functions to throw when they refer to a variable that the JIT can't fetch; but it does not make sense for getVariable to do so, because then one would need to wrap every use of getVariable in a try clause, which is less convenient than returning a distinguished value.

- While we will want to have Debugger APIs that expose SpiderMonkey-specific details (like optimization information, xray wrappers, and so on), and while it is Debugger's job to "draw back the curtain" on the engine, there should be a core API that behaves as one would expect from simply reading the ECMAScript spec. That core API would include getVariable, so getVariable should not return something SpiderMonkey-specific like MagicWhy-based values.

- Because all object debuggee values are represented by Debugger.Object instances, it would be unambiguous for getVariable to return an object like { optimizedOut: true } when asked for an unavailable variable value. This requires no 'catch' clause, avoids having more special types and classes.

Comment 36

[Jim Blandy :jimb]

Draft debugger API doc change:

JavaScript engines often omit variables from environments, to save space and reduce execution time. If the given variable should be in scope, but <code>getVariable</code> is unable to produce its value, it returns an ordinary JavaScript object (not a <code>Debugger.Object</code> instance) whose <code>optimizedOut</code> property is <code>true</code>.

Comment 37

[Jim Blandy :jimb]

(In reply to Luke Wagner [:luke] from comment #34)
> Anyhow, Odin is explicitly disabled by debugMode (see EstablishPreconditions
> in AsmJS.cpp); it'll take a lot more work to make it work with the debugger
> since so many of the supporting data structures (snapshots, safe points,
> etc) just aren't created at all.

What happens to a compartment's existing AsmJSModules when the compartment becomes a debuggee?

Comment 38

[Luke Wagner [:luke]]

Nothing.  All asm.js-compiled functions (both the asm.js module functions and the inner functions exported from it) look like fun->isNative() to the JS engine so the debugger mostly stays away.

Comment 39

[Jim Blandy :jimb]

Okay. But if I reload with the debugger enabled, then it'll all run as ordinary JS, and we can debug things at the JS level. Sounds good.

Comment 40

[Jim Blandy :jimb]

Comment on attachment 825092 [details] [diff] [review]
Part XX: Return JS_ION_OPTIMIZED_OUT magic values for optimized out values from JIT frames. (r=?)

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

Clearing review, as we decided to use a different way to report optimized-out variable values.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +198,5 @@
>            }
>            case MIRType_Magic:
>            {
>              uint32_t index;
> +            JSWhyMagic why = mir->isUnused() ? JS_ION_OPTIMIZED_OUT : JS_OPTIMIZED_ARGUMENTS;

If I follow correctly, this is saying that whenever mir->isUnused(), it represents a value that was optimized out, correct?

Comment 41

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #40)
> Comment on attachment 825092 [details] [diff] [review]
> Part XX: Return JS_ION_OPTIMIZED_OUT magic values for optimized out values
> from JIT frames. (r=?)
> 
> Review of attachment 825092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I follow correctly, this is saying that whenever mir->isUnused(), it
> represents a value that was optimized out, correct?

That's right. It says its value was not synched to a bailout Snapshot because it has no uses.

Comment 42

[Shu-yu Guo [:shu]]

Attachment: WIP Part 4: Recompile on-stack baseline scripts when toggling debug mode

Needs rebasing.

Comment 43

[Shu-yu Guo [:shu]]

Attachment: WIP Part 5: Relax Debugger

Need rebasing.

Comment 44

[Jim Blandy :jimb]

Shu says these patches apply cleanly to inbound commit f5c23a81cc85.

Comment 45

[Shu-yu Guo [:shu]]

Attachment: Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

Comment 46

[Shu-yu Guo [:shu]]

Attachment: Part 2: Refactor readFrameArgs in Ion frame iterators. (

Comment 47

[Shu-yu Guo [:shu]]

Attachment: Part 2: Bailout in place instead of directly to catch on Ion exception when Debugger is on.

Comment 48

[Shu-yu Guo [:shu]]

Attachment: Part 3: Support rematerializing Ion frames on the stack.

Comment 49

[Shu-yu Guo [:shu]]

Attachment: Part 5: Recompile on-stack baseline scripts when toggling debug mode.

Comment 50

[Shu-yu Guo [:shu]]

Attachment: Part 5: Relax the no on-stack scripts restriction for addDebuggee.

Comment 51

[Shu-yu Guo [:shu]]

Comment on attachment 8378696 [details] [diff] [review]
Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

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

I might've asked you for review of this already, but this incarnation looks a bit different.

Comment 52

[Shu-yu Guo [:shu]]

Comment on attachment 8378698 [details] [diff] [review]
Part 2: Refactor readFrameArgs in Ion frame iterators. (

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

Carrying r+ from jandem.

Comment 53

[Shu-yu Guo [:shu]]

Comment on attachment 8378699 [details] [diff] [review]
Part 2: Bailout in place instead of directly to catch on Ion exception when Debugger is on.

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

Carrying r+ from jandem.

Comment 54

[Shu-yu Guo [:shu]]

Comment on attachment 8378700 [details] [diff] [review]
Part 3: Support rematerializing Ion frames on the stack.

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

Carrying r+ from jandem.

Comment 55

[Shu-yu Guo [:shu]]

This will need a bunch of testing; I'll try to write some non-stupid ones tomorrow.

Comment 56

[Jan de Mooij [:jandem]]

Comment on attachment 8378696 [details] [diff] [review]
Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

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

Nice, using a MagicValue in the compiler also feels a bit cleaner than abusing UndefinedValue.

::: js/src/jit/IonAnalysis.cpp
@@ +705,5 @@
> +        return false;
> +
> +    JSWhyMagic why = phi->getOperand(0)->toConstant()->value().whyMagic();
> +    for (size_t i = 1; i < phi->numOperands(); i++) {
> +        if (why != phi->getOperand(i)->toConstant()->value().whyMagic())

Are all operands guaranteed to be constants?

::: js/src/vm/Debugger.cpp
@@ +753,5 @@
> +        if (!optObj)
> +            return false;
> +
> +        RootedValue trueVal(cx, BooleanValue(true));
> +        if (!JSObject::defineProperty(cx, optObj, cx->names().optimizedOut, trueVal))

This could use tests, but you mentioned that already.

Comment 57

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8378696 [details] [diff] [review]
Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

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

This is Great :)

The only detail, is that will increase the size of snapshots and the constant pool, see Bug 968296.

Comment 58

[Jan de Mooij [:jandem]]

Comment on attachment 8378701 [details] [diff] [review]
Part 5: Recompile on-stack baseline scripts when toggling debug mode.

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

Looks good, but some things I'd like to understand better before I review:

::: js/src/jit/BaselineBailouts.cpp
@@ +1492,5 @@
>      RootedScript outerScript(cx, nullptr);
>  
>      JS_ASSERT(cx->currentlyRunningInJit());
>      IonFrameIterator iter(cx);
> +    uint8_t *outerFp = NULL;

Nit: s/NULL/nullptr (please grep the other patches for NULL too)

::: js/src/jit/BaselineIC.cpp
@@ +761,5 @@
> +        wasEnabled_(frame->script()->baselineScript()->debugMode())
> +    { }
> +
> +    bool invalid() const {
> +        return !wasEnabled_ && frame_->script()->baselineScript()->debugMode();

If you make this |return false;|, do you get jit-test failures? Please make sure we have tests for this.

::: js/src/jit/BaselineJIT.cpp
@@ +964,5 @@
> +    JSScript *script;
> +    uint32_t pcOffset;
> +
> +#ifdef DEBUG
> +    // Used for sanity asserts in debug mode.

Nit: "debug mode" is a bit ambiguous here. "debug builds"?

@@ +1122,5 @@
> +
> +    DebugOnly<bool> expectedDebugMode = cx->compartment()->debugMode();
> +    IonSpew(IonSpew_BaselineScripts, "Recompiling (%s:%d) for debug mode %s",
> +            script->filename(), script->lineno(), expectedDebugMode ? "ON" : "OFF");
> +    MOZ_ASSERT(oldBaselineScript->debugMode() != expectedDebugMode);

I think this will fail if a script is on the stack more than once. You can check

BaselineScript *oldBaselineScript = script->baselineScript();
bool expectedDebugMode = cx->compartment()->debugMode();

if (expectedDebugMode == oldBaselineScript->debugMode()) {
    // ... explain when this happens ...
    return true;
}

script->setBaselineScript(cx, nullptr);
...

@@ +1161,5 @@
> +    DebugOnly<bool> expectedDebugMode = comp->debugMode();
> +    for (size_t i = 0; i < entries.length(); i++) {
> +        JSScript *script = entries[i].script;
> +        if (!RecompileBaselineScriptForDebugMode(cx, script))
> +            return false;

We should call resetActive() for all entries in the vector before returning false. If we don't do that we may get GC asserts later on, or keep scripts alive longer than necessary if GC thinks they are active on the stack.

@@ +1166,5 @@
> +
> +        // Active here means 'was recompiled'. We use it to only patch
> +        // frames of recompiled scripts in PatchBaselineFramesForDebugMode
> +        // below.
> +        MOZ_ASSERT(!script->baselineScript()->active());

I think this assert will fail if a script is on the stack multiple times.

Actually, why do we need the active flag here? Aren't we recompiling all scripts in the Vector anyway, so they will all get the flag? Not setting the flag would simplify things.

@@ +1175,5 @@
> +    for (JitActivationIterator iter(cx->runtime()); !iter.done(); ++iter) {
> +        if (iter.activation()->compartment() == comp)
> +            processed = PatchBaselineFramesForDebugMode(cx, iter, entries, processed);
> +    }
> +    MOZ_ASSERT(processed = entries.length());

Nit: s/=/==/

::: js/src/jscompartment.h
@@ +376,5 @@
>      }
>  
> +    /* True if the compartment has live Ion code and is in debug mode. */
> +    bool debugModeAndHasLiveIonCode() const {
> +        return !!(debugModeBits & (DebugModeFromMask | DebugHasLiveIonCode));

(Is this function used? Maybe in other patches?) This function returns true if we're in debug mode OR there's live Ion code. I think we want something like this:

const unsigned mask = DebugModeFromMask | DebugHasLiveIonCode;
return (debugModeBits & mask) == mask;

Or maybe simpler:

return debugMode() && (debugModeBits & DebugHasLiveIonCode);

Comment 59

[Jan de Mooij [:jandem]]

Also, shouldn't we call PatchBaselineFramesForDebugMode if RecompileBaselineScriptForDebugMode fails?

It's unfortunate that RecompileBaselineScriptForDebugMode is fallible, but we have to handle this without leaving the stack in a bogus state.

Comment 60

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #56)
> Comment on attachment 8378696 [details] [diff] [review]
> Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach
> Debugger about them.
> 
> Review of attachment 8378696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, using a MagicValue in the compiler also feels a bit cleaner than
> abusing UndefinedValue.
> 
> ::: js/src/jit/IonAnalysis.cpp
> @@ +705,5 @@
> > +        return false;
> > +
> > +    JSWhyMagic why = phi->getOperand(0)->toConstant()->value().whyMagic();
> > +    for (size_t i = 1; i < phi->numOperands(); i++) {
> > +        if (why != phi->getOperand(i)->toConstant()->value().whyMagic())
> 
> Are all operands guaranteed to be constants?
> 

Good catch. It seems possible that we can get a phi(a, phi(OPTIMIZED_ARGUMENTS, OPTIMIZED_OUT)) or something.

Comment 61

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #58)
> Comment on attachment 8378701 [details] [diff] [review]
> Part 5: Recompile on-stack baseline scripts when toggling debug mode.
> 
> Review of attachment 8378701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but some things I'd like to understand better before I review:
> 

All good points below, especially about the active flag.

> 
> @@ +1122,5 @@
> > +
> > +    DebugOnly<bool> expectedDebugMode = cx->compartment()->debugMode();
> > +    IonSpew(IonSpew_BaselineScripts, "Recompiling (%s:%d) for debug mode %s",
> > +            script->filename(), script->lineno(), expectedDebugMode ? "ON" : "OFF");
> > +    MOZ_ASSERT(oldBaselineScript->debugMode() != expectedDebugMode);
> 
> I think this will fail if a script is on the stack more than once. You can
> check
> 
> BaselineScript *oldBaselineScript = script->baselineScript();
> bool expectedDebugMode = cx->compartment()->debugMode();
> 
> if (expectedDebugMode == oldBaselineScript->debugMode()) {
>     // ... explain when this happens ...
>     return true;
> }
> 
> script->setBaselineScript(cx, nullptr);
> ...
> 
> @@ +1161,5 @@
> > +    DebugOnly<bool> expectedDebugMode = comp->debugMode();
> > +    for (size_t i = 0; i < entries.length(); i++) {
> > +        JSScript *script = entries[i].script;
> > +        if (!RecompileBaselineScriptForDebugMode(cx, script))
> > +            return false;
> 
> We should call resetActive() for all entries in the vector before returning
> false. If we don't do that we may get GC asserts later on, or keep scripts
> alive longer than necessary if GC thinks they are active on the stack.
> 
> @@ +1166,5 @@
> > +
> > +        // Active here means 'was recompiled'. We use it to only patch
> > +        // frames of recompiled scripts in PatchBaselineFramesForDebugMode
> > +        // below.
> > +        MOZ_ASSERT(!script->baselineScript()->active());
> 
> I think this assert will fail if a script is on the stack multiple times.
> 
> Actually, why do we need the active flag here? Aren't we recompiling all
> scripts in the Vector anyway, so they will all get the flag? Not setting the
> flag would simplify things.
> 

The logic got messed up when reviving the patches. I think you're right that the active flag isn't needed at all.

> 
> ::: js/src/jscompartment.h
> @@ +376,5 @@
> >      }
> >  
> > +    /* True if the compartment has live Ion code and is in debug mode. */
> > +    bool debugModeAndHasLiveIonCode() const {
> > +        return !!(debugModeBits & (DebugModeFromMask | DebugHasLiveIonCode));
> 
> (Is this function used? Maybe in other patches?) This function returns true
> if we're in debug mode OR there's live Ion code. I think we want something
> like this:
> 
> const unsigned mask = DebugModeFromMask | DebugHasLiveIonCode;
> return (debugModeBits & mask) == mask;
> 
> Or maybe simpler:
> 
> return debugMode() && (debugModeBits & DebugHasLiveIonCode);

This function is dead; slipped past me when rebasing.

Comment 62

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #59)
> Also, shouldn't we call PatchBaselineFramesForDebugMode if
> RecompileBaselineScriptForDebugMode fails?
> 
> It's unfortunate that RecompileBaselineScriptForDebugMode is fallible, but
> we have to handle this without leaving the stack in a bogus state.

Yeah, good catch. The cleanest thing here to do is do the recompiles transaction style -- don't swap in any of the newly recompiled BaselineScripts until all the compilations have succeeded.

Comment 63

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #62)
> Yeah, good catch. The cleanest thing here to do is do the recompiles
> transaction style -- don't swap in any of the newly recompiled
> BaselineScripts until all the compilations have succeeded.

Yes, that would be great and avoid a lot of trouble. Then we just have to make sure the caller handles this correctly.

Comment 64

[Shu-yu Guo [:shu]]

Attachment: Part 5: Recompile on-stack baseline scripts when toggling debug mode.

WIP.

New fixes:
- Fixes for patching return addresses for stub frames.
- Trampolines for off->on toggles.

We need a trampoline for off->on toggles because when we recompile a baseline
script from for debug mode to for non-debug mode, there are VM calls in the
debug mode script for which we have no corresponding location in the non-debug
mode script (e.g., the debug mode prologue). For these cases we generate a fake
isForOp ICEntry with an accompanying trampoline which fixes up a return from a
baseline VM call before returning to the "real" address in the non-debug mode
script.

Comment 65

[Jim Blandy :jimb]

Comment on attachment 8378696 [details] [diff] [review]
Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

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

Needs tests, but otherwise looks fine to me.

::: js/src/vm/Debugger.cpp
@@ +753,5 @@
> +        if (!optObj)
> +            return false;
> +
> +        RootedValue trueVal(cx, BooleanValue(true));
> +        if (!JSObject::defineProperty(cx, optObj, cx->names().optimizedOut, trueVal))

Yeah, tests needed.

Comment 66

[Shu-yu Guo [:shu]]

I split out part 2 into bug 979056 so I can land some of this without it getting bitrotten over and over.

Comment 67

[Shu-yu Guo [:shu]]

Attachment: Part 4: Recompile on-stack baseline scripts when toggling debug mode.

This is a pretty gnarly patch, unfortunately.

Jan, look for the "Recompile Patching Overview" block comment that explains in
detail how the patching's done for all the cases.

Comment 68

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #67)
> Created attachment 8387312 [details] [diff] [review]
> Part 4: Recompile on-stack baseline scripts when toggling debug mode.
> 
> This is a pretty gnarly patch, unfortunately.

Most of the extra code/complexity compared to the patch I reviewed before is to handle the on -> off case, right? What if, when we do on -> off, we still recompile the script but in a special mode:

(1) Still add an ICEntry for the debug prologue and epilogue, so that we can resume there like any other VM call. If it's simpler, we could even emit the callVM in this case and have an unconditional jump to skip it in the common case. Or emit something like this:

jmp skip
pop frame reg <- ICEntry points here
skip:
...

(2) For the debug trap handler, we could do the same, emit:

jmp skip
pop R0 and/or R1 if necessary <- ICEntry points here
skip:
...

If you don't like the unconditional jumps, we could emit this after the main code and jump back to the start of the op after popping R0/R1.

What do you think, could this work? It's some extra code but perf should be the same, and we'll only do this for scripts that are on the stack when we turn off debug mode.

Comment 69

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #68)
> (In reply to Shu-yu Guo [:shu] from comment #67)
> > Created attachment 8387312 [details] [diff] [review]
> > Part 4: Recompile on-stack baseline scripts when toggling debug mode.
> > 
> > This is a pretty gnarly patch, unfortunately.
> 
> Most of the extra code/complexity compared to the patch I reviewed before is
> to handle the on -> off case, right? What if, when we do on -> off, we still
> recompile the script but in a special mode:
> 
> (1) Still add an ICEntry for the debug prologue and epilogue, so that we can
> resume there like any other VM call. If it's simpler, we could even emit the
> callVM in this case and have an unconditional jump to skip it in the common
> case. Or emit something like this:
> 
> jmp skip
> pop frame reg <- ICEntry points here
> skip:
> ...
> 
> (2) For the debug trap handler, we could do the same, emit:
> 
> jmp skip
> pop R0 and/or R1 if necessary <- ICEntry points here
> skip:
> ...
> 
> If you don't like the unconditional jumps, we could emit this after the main
> code and jump back to the start of the op after popping R0/R1.
> 
> What do you think, could this work? It's some extra code but perf should be
> the same, and we'll only do this for scripts that are on the stack when we
> turn off debug mode.

I don't feel like emitting such "continuation fixers" inline make the situation any less complicated. I kind of feel the opposite, because:

  1. The proposed approach feels really ad hoc. This will be sprinkled everywhere in the code for *every single op*.
  2. Complicates the compiler instead of having the logic kept separately.
  3. We can toggle off->on from the interrupt handler, and emitting inline fixers at loopheads is definitely a no go IMO.

All in all I think like the current patch with the unified continuation fixer, albeit complicated, is the cleaner and more maintainable approach.

Comment 70

[Jan de Mooij [:jandem]]

Comment on attachment 8387312 [details] [diff] [review]
Part 4: Recompile on-stack baseline scripts when toggling debug mode.

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

It turned out to be less complicated than I thought, great. I'd like to take another look before I r+ though, it's easy to miss something.

::: js/src/jit/BaselineIC.h
@@ +235,5 @@
> +  public:
> +    ICEntry(uint32_t pcOffset, IsFor isFor)
> +      : firstStub_(nullptr), returnOffset_(), pcOffset_(pcOffset), isFor_(isFor)
> +    {
> +        // The offset must fit in at least 29 bits, since we shave off 3 for

Because we use bitfields, it's nice to add:

MOZ_ASSERT(pcOffset_ == pcOffset);
MOZ_ASSERT(isFor_ == isFor);

To help catch cases where the bitfield becomes too small (and MSVC has some weird bitfield+enum quirks).

::: js/src/jit/BaselineOnStackRecompile.cpp
@@ +43,5 @@
> +        stub(nullptr)
> +    {
> +#ifdef DEBUG
> +        // Assert that if we have a NotForOp ICEntry, that there are no
> +        // unsynched slots, since such a recompile could have only been

Nit: unsynced

@@ +122,5 @@
> +    return reinterpret_cast<uint8_t *>(addr);
> +}
> +
> +static const char *
> +IsForToString(ICEntry::IsFor isFor)

IsForToString sounds like it should return a bool. Can we rename ICEntry::IsFor to ICEntry::Kind/CallKind or something?

@@ +138,5 @@
> +        return "debug prologue";
> +      case ICEntry::IsForDebugEpilogue:
> +        return "debug epilogue";
> +      default:
> +        return "?";

I'd prefer MOZ_ASSUME_UNREACHABLE. When somebody adds a new enum value, it's good to force them to handle it everywhere :)

@@ +178,5 @@
> +    // script.
> +    //
> +    // Off to On:
> +    //  A. From a "can call" stub.
> +    //  B. From the interrupt handler.

What's special about the interrupt handler? It's just a VM call and most/all of these can toggle debug mode right? Also, what about non-call stubs like BinaryArith? Those can also toggle debug mode, would be good to mention them all.

@@ +193,5 @@
> +    // state). Specifics on what need to be done are documented below.
> +    //
> +
> +    IonCommonFrameLayout *prev = nullptr;
> +    size_t i = *start;

We should rename this to something that describes what it's referring to (entryIndex or so).

@@ +231,5 @@
> +            bool popFrameReg;
> +            BaselineOnStackRecompileInfo *recompInfo =
> +                cx->new_<BaselineOnStackRecompileInfo>(pc, isFor);
> +            if (!recompInfo)
> +                return false;

What happens exactly in this case? It'd be nice if we could make this method infallible, maybe we could allocate upfront?

@@ +374,5 @@
> +        return false;
> +    }
> +
> +    // Destroy the old baseline script only if compilation succeeded.
> +    BaselineScript::Destroy(cx->runtime()->defaultFreeOp(), oldBaselineScript);

If we destroy the old script here, won't UndoRecompile* restore garbage?

@@ +425,5 @@
> +    for (JitActivationIterator iter(cx->runtime()); !iter.done(); ++iter) {
> +        if (iter.activation()->compartment() == comp &&
> +            !PatchBaselineFramesForDebugMode(cx, iter, entries, &processed))
> +        {
> +            return false;

Don't we want to undo in this case too?

@@ +457,5 @@
> +SyncOnStackBaselineRecompileInfo(BaselineFrame *frame, Value *vp, bool rv)
> +{
> +    BaselineOnStackRecompileInfo *info = frame->onStackRecompileInfo();
> +    MOZ_ASSERT(info);
> +    MOZ_ASSERT(info->resumeAddr);

It'd be good to assert here that resumeAddr is inside frame->script()->baseline()

@@ +471,5 @@
> +    // need to deal with it here.
> +    ICEntry::IsFor isFor = info->frameIsFor;
> +    if (isFor == ICEntry::IsForDebugEpilogue ||
> +        ((isFor == ICEntry::IsForDebugPrologue ||
> +          (isFor == ICEntry::IsForCallVM && JSOp(*info->pc) == JSOP_DEBUGGER)) && rv))

This condition is pretty hard to read. We should rewrite it as a number of separate ifs (maybe in a helper function that returns a bool).

@@ +515,5 @@
> +        AutoCompartment ac(cx, cx->runtime()->atomsCompartment());
> +        uint32_t offset;
> +        JitCode *code = generateOnStackBaselineRecompileHandler(cx, &offset);
> +        baselineOnStackRecompileHandler_ = code;
> +        baselineOnStackRecompileHandlerNoFrameRegPopAddr_ = code->raw() + offset;

generateOnStackBaselineRecompileHandler (linker.newCode) is fallible, so we should make sure code != nullptr.

@@ +524,5 @@
> +
> +void *
> +JitRuntime::getOnStackBaselineRecompileHandlerAddress(JSContext *cx, bool popFrameReg)
> +{
> +    getOnStackBaselineRecompileHandler(cx);

Same here.

@@ +574,5 @@
> +    masm.pushValue(Address(temp, offsetof(BaselineOnStackRecompileInfo, valueR1)));
> +    masm.push(BaselineFrameReg);
> +    masm.push(Address(temp, offsetof(BaselineOnStackRecompileInfo, resumeAddr)));
> +
> +    // Call a stub to free the allocated info.

This will leak if we're unwinding the stack for an exception, see HandleException and HandleExceptionBaseline.

Comment 71

[Shu-yu Guo [:shu]]

Attachment: Part 4: Recompile on-stack baseline scripts when toggling debug mode v3

Answering review questions in comment below.

Comment 72

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #70)
> Comment on attachment 8387312 [details] [diff] [review]
> Part 4: Recompile on-stack baseline scripts when toggling debug mode.
> 
> Review of attachment 8387312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It turned out to be less complicated than I thought, great. I'd like to take
> another look before I r+ though, it's easy to miss something.

For sure, thanks for reviewing.

> 
> ::: js/src/jit/BaselineIC.h
> @@ +235,5 @@
> > +  public:
> > +    ICEntry(uint32_t pcOffset, IsFor isFor)
> > +      : firstStub_(nullptr), returnOffset_(), pcOffset_(pcOffset), isFor_(isFor)
> > +    {
> > +        // The offset must fit in at least 29 bits, since we shave off 3 for
> 
> Because we use bitfields, it's nice to add:
> 
> MOZ_ASSERT(pcOffset_ == pcOffset);
> MOZ_ASSERT(isFor_ == isFor);
> 
> To help catch cases where the bitfield becomes too small (and MSVC has some
> weird bitfield+enum quirks).
> 

Good suggestion, added.

> ::: js/src/jit/BaselineOnStackRecompile.cpp
> @@ +43,5 @@
> > +        stub(nullptr)
> > +    {
> > +#ifdef DEBUG
> > +        // Assert that if we have a NotForOp ICEntry, that there are no
> > +        // unsynched slots, since such a recompile could have only been
> 
> Nit: unsynced
> 

Fixed.

> @@ +122,5 @@
> > +    return reinterpret_cast<uint8_t *>(addr);
> > +}
> > +
> > +static const char *
> > +IsForToString(ICEntry::IsFor isFor)
> 
> IsForToString sounds like it should return a bool. Can we rename
> ICEntry::IsFor to ICEntry::Kind/CallKind or something?
> 

Renamed ICEntry::Kind.

> @@ +138,5 @@
> > +        return "debug prologue";
> > +      case ICEntry::IsForDebugEpilogue:
> > +        return "debug epilogue";
> > +      default:
> > +        return "?";
> 
> I'd prefer MOZ_ASSUME_UNREACHABLE. When somebody adds a new enum value, it's
> good to force them to handle it everywhere :)
> 

Changed.

> @@ +178,5 @@
> > +    // script.
> > +    //
> > +    // Off to On:
> > +    //  A. From a "can call" stub.
> > +    //  B. From the interrupt handler.
> 
> What's special about the interrupt handler? It's just a VM call and most/all
> of these can toggle debug mode right? Also, what about non-call stubs like
> BinaryArith? Those can also toggle debug mode, would be good to mention them
> all.
> 

Combined the interrupt handler and the debugger statement handler cases into a single VM call case.

> @@ +193,5 @@
> > +    // state). Specifics on what need to be done are documented below.
> > +    //
> > +
> > +    IonCommonFrameLayout *prev = nullptr;
> > +    size_t i = *start;
> 
> We should rename this to something that describes what it's referring to
> (entryIndex or so).
> 

Renamed.

> @@ +231,5 @@
> > +            bool popFrameReg;
> > +            BaselineOnStackRecompileInfo *recompInfo =
> > +                cx->new_<BaselineOnStackRecompileInfo>(pc, isFor);
> > +            if (!recompInfo)
> > +                return false;
> 
> What happens exactly in this case? It'd be nice if we could make this method
> infallible, maybe we could allocate upfront?
> 

Good catch on this one, the error handling in the caller of this function was a mess. The new version allocates RecompileInfos up front. If alloc fails sometime later, we delete all RecompileInfos when erroring out.

As a result the patching function is now infallible.

> @@ +374,5 @@
> > +        return false;
> > +    }
> > +
> > +    // Destroy the old baseline script only if compilation succeeded.
> > +    BaselineScript::Destroy(cx->runtime()->defaultFreeOp(), oldBaselineScript);
> 
> If we destroy the old script here, won't UndoRecompile* restore garbage?
> 

Another great catch. Destroying the old baseline scripts now only happens after all the entries/RecompileInfos have been successfully allocated.

> @@ +425,5 @@
> > +    for (JitActivationIterator iter(cx->runtime()); !iter.done(); ++iter) {
> > +        if (iter.activation()->compartment() == comp &&
> > +            !PatchBaselineFramesForDebugMode(cx, iter, entries, &processed))
> > +        {
> > +            return false;
> 
> Don't we want to undo in this case too?
> 

The patching function is now infallible.

> @@ +457,5 @@
> > +SyncOnStackBaselineRecompileInfo(BaselineFrame *frame, Value *vp, bool rv)
> > +{
> > +    BaselineOnStackRecompileInfo *info = frame->onStackRecompileInfo();
> > +    MOZ_ASSERT(info);
> > +    MOZ_ASSERT(info->resumeAddr);
> 
> It'd be good to assert here that resumeAddr is inside
> frame->script()->baseline()
> 

Added.

> @@ +471,5 @@
> > +    // need to deal with it here.
> > +    ICEntry::IsFor isFor = info->frameIsFor;
> > +    if (isFor == ICEntry::IsForDebugEpilogue ||
> > +        ((isFor == ICEntry::IsForDebugPrologue ||
> > +          (isFor == ICEntry::IsForCallVM && JSOp(*info->pc) == JSOP_DEBUGGER)) && rv))
> 
> This condition is pretty hard to read. We should rewrite it as a number of
> separate ifs (maybe in a helper function that returns a bool).
> 

Refactored out into a helper function.

> @@ +515,5 @@
> > +        AutoCompartment ac(cx, cx->runtime()->atomsCompartment());
> > +        uint32_t offset;
> > +        JitCode *code = generateOnStackBaselineRecompileHandler(cx, &offset);
> > +        baselineOnStackRecompileHandler_ = code;
> > +        baselineOnStackRecompileHandlerNoFrameRegPopAddr_ = code->raw() + offset;
> 
> generateOnStackBaselineRecompileHandler (linker.newCode) is fallible, so we
> should make sure code != nullptr.
> 

I now make sure it's available if needed in the phase where I allocate all the RecompileInfos up front.

> @@ +524,5 @@
> > +
> > +void *
> > +JitRuntime::getOnStackBaselineRecompileHandlerAddress(JSContext *cx, bool popFrameReg)
> > +{
> > +    getOnStackBaselineRecompileHandler(cx);
> 
> Same here.
> 

Ditto.

> @@ +574,5 @@
> > +    masm.pushValue(Address(temp, offsetof(BaselineOnStackRecompileInfo, valueR1)));
> > +    masm.push(BaselineFrameReg);
> > +    masm.push(Address(temp, offsetof(BaselineOnStackRecompileInfo, resumeAddr)));
> > +
> > +    // Call a stub to free the allocated info.
> 
> This will leak if we're unwinding the stack for an exception, see
> HandleException and HandleExceptionBaseline.

Ooh, good point. I now delete it right after HandleExceptionBaseline returns. HandleExceptionBaseline actually needs to grab the "real" return address out of the RecompileInfo to get the right pc.

Comment 73

[Jim Blandy :jimb]

Comment on attachment 8378702 [details] [diff] [review]
Part 5: Relax the no on-stack scripts restriction for addDebuggee.

Cancelled per IRC discussion; patch under revision.

Comment 74

[Shu-yu Guo [:shu]]

Attachment: Part 5: Relax the no on-stack scripts restriction for addDebuggee v2

Changes:

 - renamed .repr to .implementation

 - {add,remove}Debuggee are now fallible due to recompilation (but can operate
   infallibly when toggling on->off as needed for sweeping)

 - assert that the pc never needs updating for remat frames

Comment 75

[Jan de Mooij [:jandem]]

Comment on attachment 8391034 [details] [diff] [review]
Part 4: Recompile on-stack baseline scripts when toggling debug mode v3

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

Looks great! r=me with the comments below addressed.

Please make sure we have a ton of good tests for this, also ones that are easy to modify by the fuzzers (use normal JS functions instead of eval("long string") etc).

::: js/src/jit/BaselineIC.h
@@ +240,5 @@
> +        // the Kind enum.
> +        MOZ_ASSERT(pcOffset_ == pcOffset);
> +        MOZ_ASSERT(kind_ == kind);
> +        JS_STATIC_ASSERT(BaselineScript::MAX_JSSCRIPT_LENGTH < 0x1fffffff);
> +        JS_ASSERT(pcOffset <= BaselineScript::MAX_JSSCRIPT_LENGTH);

Nit: s/JS/MOZ to match the other asserts.

::: js/src/jit/BaselineOnStackRecompile.cpp
@@ +44,5 @@
> +        pcOffset(icEntry.pcOffset()),
> +        frameKind(icEntry.kind()),
> +        stub(nullptr)
> +    {
> +#ifdef DEBUG

Nit: just to be sure:

MOZ_ASSERT(pcOffset == icENtry.pcOffset());
MOZ_ASSERT(frameKind == icEntry.kind());

(JM used to have weird bugs caused by bit fields in IC structures that were too small...)

@@ +152,5 @@
> +    // Initialize the on-stack recompile handler, which may fail, so that
> +    // patching the stack is infallible.
> +    if (needsRecompileHandler) {
> +        JitRuntime *rt = cx->runtime()->jitRuntime();
> +        if (!rt->getOnStackBaselineRecompileHandlerAddress(cx, true))

Needs a DeleteRecompileInfosOnRecompileEntries. To avoid such leaks, we should give DebugModeRecompileEntry a destructor that js_delete's recompInfo. Then when we store it in the frame, we can null the pointer in the RecompileEntry so that it's not freed in that case. We can also remove DeleteRecompileInfosOnRecompileEntries then.

@@ +483,5 @@
> +
> +    // If all recompiles succeeded, destroy the old baseline scripts and patch
> +    // the live frames.
> +    //
> +    // After this point the function must be infallible.

Nice!

@@ +529,5 @@
> +        return true;
> +
> +    // |rv| is the value in ReturnReg. If true, in the case of the prologue,
> +    // debug trap, and debugger statement handler, it means a forced return.
> +    if ((kind == ICEntry::Kind_DebugPrologue ||

Nit: I think you can remove one '(' (and the corresponding ')' at the end of the condition).

@@ +675,5 @@
> +    Linker linker(masm);
> +    JitCode *code = linker.newCode<NoGC>(cx, JSC::OTHER_CODE);
> +
> +    noFrameRegPopOffset.fixup(&masm);
> +    *noFrameRegPopOffsetOut = noFrameRegPopOffset.offset();

I'm not sure how this behaves if linker.newCode fails. Best to add

if (!code)
    return nullptr;

immediately after JitCode *code = ...

::: js/src/jit/BaselineOnStackRecompile.h
@@ +11,5 @@
> +
> +#include "jit/BaselineIC.h"
> +#include "jit/BaselineJIT.h"
> +
> +namespace js {

Nit: add a comment either here or at the top of the cpp file to briefly explain what it's for (recompiling on-stack Baseline scripts when toggling debug mode), in case people confuse it with Interpreter -> Baseline OSR, Baseline -> Ion OSR, Ion bailouts or something.

Thinking about this more, it is a bit confusing with the other OSR. Maybe BaselineDebug(Mode)Recompile.h/cpp or BaselineDebugOSR?

::: js/src/jit/IonFrameIterator.h
@@ +373,1 @@
>      struct Nop {

Nit: is this change still necessary? I don't see "Nop" anywhere else in this patch, but maybe I missed it.

Comment 77

[Shu-yu Guo [:shu]]

Attachment: Part 4: Recompile on-stack baseline scripts when toggling debug mode

Nits addressed and renamed to BaselineDebugModeOSR.

Comment 78

[Shu-yu Guo [:shu]]

Comment on attachment 8395148 [details] [diff] [review]
Part 4: Recompile on-stack baseline scripts when toggling debug mode

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

Carrying r=jandem

Comment 79

[Shu-yu Guo [:shu]]

Attachment: Part 4: Recompile on-stack baseline scripts when toggling debug mode

Forgot to add a move constructor

Comment 80

[Shu-yu Guo [:shu]]

Attachment: Part 5: Relax the no on-stack scripts restriction for addDebuggee

Rebased on top of 986767 to use the UnderGC variants of removeDebuggee.

Comment 81

[Shu-yu Guo [:shu]]

Attachment: Part 6: Tests

Feel free to add more!

Comment 82

[Shu-yu Guo [:shu]]

Comment on attachment 8395151 [details] [diff] [review]
Part 4: Recompile on-stack baseline scripts when toggling debug mode

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

Carrying r=jandem

Comment 83

[Jim Blandy :jimb]

Comment on attachment 8395153 [details] [diff] [review]
Part 6: Tests

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

Is using 'timeout' in the unit tests definitely better than simply having the debuggee code call a function from the main global? It's good to have it in there for at least one test, since we definitely want to exercise things from the interrupt callback. But it seems slow (we spin the CPU for a second), and slightly unpredictable (although I guess it's always going to hit the while loop's poll, in practice).

Actually, in the simpler cases, I suppose there's no reason not to simply say "g.dbg = dbg" and let g's code manipulate itself. Naturally, giving g an onEnterFrame handler in g would cause unbounded recursion, but the simpler cases would be okay.

It might be cool to have a shell primitive that continually re-requests interrupts from every interrupt handler, and then calls your handler after N interrupts. Then we could sweep with N, and exercise every interrupt poll site.

I'd like to see more tests:

- frame.eval in non-youngest frames should work.

- frame.eval in non-youngest frames executing within a let block should work.

- frame.eval("var d = 'bleah'") where the new d shadows a 'var d' in an enclosing scope that is captured by some inner function --- that is, the new declaration "captures" references.

- frame.eval("d = 'bleah'") where IonMonkey had previously determined that d was an integer.

- enabling debugging from within getters; setters; implicitly invoked toString and valueOf functions; proxy handlers; and other non-obvious  call sites should work.

- Enabling debugging in some compartments while the stack winds through several other compartments that are not debuggees, and then walking the stack, should yield only frames for the new debuggee compartments.

- Walking a stack that goes from debuggee -> non-debuggee -> debuggee, then making the non-debuggee into a debuggee, and walking the stack again, should make the stack frames appear.

You've got your head in the midst of all this machinery; surely you can come up with more torture than this. :) Think of all the edge cases you had to consider in constructing the patch, and then write a test for each one of them.




I'd like to see tests that call eval in non-top frames, too.

I'd like to see tests that eval in frames where control is within a 'let' block.

I'd like to see tests that try to say frame.eval("var d = 'bleah'") where d has previously been noted by Ion as an integer. And where d is captured by nested functions.

::: js/src/jit-test/tests/debug/Debugger-debuggees-22.js
@@ +10,5 @@
> +
> +  dbg.addDebuggee(this);
> +});
> +
> +g.eval("(" + function test() { f(false); f(false); f(true); f(true); } + ")();");

If the 'false' runs are supposed to "warm things up" and get a particular frame implementation, then it would be nice to assertEq the implementation. Say, have the debuggee call a function in the main global which checks dbg.getNewestFrame().implementation.

::: js/src/jit-test/tests/debug/Frame-eval-19.js
@@ +4,5 @@
> +
> +if (!jitTogglesMatch(Opts_Ion2NoParallelCompilation))
> +  quit(0);
> +
> +withJitOptions(Opts_Ion2NoParallelCompilation, function () {

Doesn't the "if (!...)" check that those jit options are already in force? Or does the withJitOptions change the ion.usecount.trigger?

::: js/src/jit-test/tests/debug/resumption-08.js
@@ +1,2 @@
> +// Check whether we respect resumption values when toggling debug mode on->off
> +// from various points with live scripts on the stack.

This one is really nice.

Comment 84

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #83)
> Comment on attachment 8395153 [details] [diff] [review]
> Part 6: Tests
> 
> Review of attachment 8395153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is using 'timeout' in the unit tests definitely better than simply having
> the debuggee code call a function from the main global? It's good to have it
> in there for at least one test, since we definitely want to exercise things
> from the interrupt callback. But it seems slow (we spin the CPU for a
> second), and slightly unpredictable (although I guess it's always going to
> hit the while loop's poll, in practice).
> 
> Actually, in the simpler cases, I suppose there's no reason not to simply
> say "g.dbg = dbg" and let g's code manipulate itself. Naturally, giving g an
> onEnterFrame handler in g would cause unbounded recursion, but the simpler
> cases would be okay.
> 

The reason I use the timeout over having code that manipulates itself is that self-manipulating code tend to mess with Ion state. Since we don't do any of the debugger calls until some particular index, we end up bailing out of the top frame, and if that frame happens to be inlined, it screws up some N number of inlined frames as well. At the end of the day it was much more unreliable in practice when I tried to write self-manipulating code. You have to do stuff that you know would inhibit inline heuristics. I can give it another try to see if I come up with anything.

> It might be cool to have a shell primitive that continually re-requests
> interrupts from every interrupt handler, and then calls your handler after N
> interrupts. Then we could sweep with N, and exercise every interrupt poll
> site.
> 

I don't understand what's being suggested here.

> I'd like to see more tests:
> 
> - frame.eval in non-youngest frames should work.
> 
> - frame.eval in non-youngest frames executing within a let block should work.
> 
> - frame.eval("var d = 'bleah'") where the new d shadows a 'var d' in an
> enclosing scope that is captured by some inner function --- that is, the new
> declaration "captures" references.
> 
> - frame.eval("d = 'bleah'") where IonMonkey had previously determined that d
> was an integer.
> 
> - enabling debugging from within getters; setters; implicitly invoked
> toString and valueOf functions; proxy handlers; and other non-obvious  call
> sites should work.
> 
> - Enabling debugging in some compartments while the stack winds through
> several other compartments that are not debuggees, and then walking the
> stack, should yield only frames for the new debuggee compartments.
> 
> - Walking a stack that goes from debuggee -> non-debuggee -> debuggee, then
> making the non-debuggee into a debuggee, and walking the stack again, should
> make the stack frames appear.
> 
> You've got your head in the midst of all this machinery; surely you can come
> up with more torture than this. :) Think of all the edge cases you had to
> consider in constructing the patch, and then write a test for each one of
> them.
> 
> 
> 
> 
> I'd like to see tests that call eval in non-top frames, too.
> 
> I'd like to see tests that eval in frames where control is within a 'let'
> block.
> 
> I'd like to see tests that try to say frame.eval("var d = 'bleah'") where d
> has previously been noted by Ion as an integer. And where d is captured by
> nested functions.
> 

The thing is, a lot of the suggested test cases don't exercise any new code paths. They seem like they might, like the different types on a local for a eval-in-frame, but they don't. I'll incorporate some of these, but theoretically *every single existing test* should be adapted somehow to an on-stack version.

> ::: js/src/jit-test/tests/debug/Debugger-debuggees-22.js
> @@ +10,5 @@
> > +
> > +  dbg.addDebuggee(this);
> > +});
> > +
> > +g.eval("(" + function test() { f(false); f(false); f(true); f(true); } + ")();");
> 
> If the 'false' runs are supposed to "warm things up" and get a particular
> frame implementation, then it would be nice to assertEq the implementation.
> Say, have the debuggee call a function in the main global which checks
> dbg.getNewestFrame().implementation.

Okay.

> 
> ::: js/src/jit-test/tests/debug/Frame-eval-19.js
> @@ +4,5 @@
> > +
> > +if (!jitTogglesMatch(Opts_Ion2NoParallelCompilation))
> > +  quit(0);
> > +
> > +withJitOptions(Opts_Ion2NoParallelCompilation, function () {
> 
> Doesn't the "if (!...)" check that those jit options are already in force?
> Or does the withJitOptions change the ion.usecount.trigger?

It does. The |if (!...)" check only checks the boolean toggles and just not bother running the tests if the toggles are different. The |withJitOptions| mess with the heuristics provided all the JIT options are on/off accordingly.

Comment 85

[Shu-yu Guo [:shu]]

Adding dependency because new tests intermittently fail without fix for bug 989509.

Comment 86

[Shu-yu Guo [:shu]]

Attachment: Part 5: Relax the no on-stack scripts restriction for addDebuggee.

Comment 87

[Shu-yu Guo [:shu]]

Attachment: Part 6: Tests.

Added more tests. Here's what each test is supposed to cover in terms of code
coverage:

Part 1: JS_OPTIMIZED_OUT magic
  - optimized-out-01.js

Part 2: Bailout in place
  - resumption-08.js ('throw' resumption values)

Part 3: Remat frames
  - Frame-eval-19.js
  - Frame-eval-20.js
  - optimized-out-01.js
  - resumption-08.js

Part 4: Debug mode OSR
  - all tests exercise this, but in particular:
  - Debugger-debugees-22.js (off->on for calls)
  - Debugger-debugees-23.js (off->on for corner cases)
  - Debugger-debugees-24.js (off->on global-at-a-time)
  - Debugger-debugees-25.js (on->off global-at-a-time)
  - resumption-08.js (on->off all possible cases)
  - Frame-eval-19.js (off->on in interrupt handler)

Part 5: Debugger API
  - Frame-implementation-01.js mainly

Frame-eval-21 tests let scoping, but doesn't really exercise any new code paths
except the debug mode OSR.

Comment 88

[Shu-yu Guo [:shu]]

Attachment: Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them.

Rebased on top of bug 996422. Mostly unchanged but requesting another look from Jan for safe measure.

Comment 89

[Jim Blandy :jimb]

Comment on attachment 8400361 [details] [diff] [review]
Part 6: Tests.

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

We need tests to establish that remat frames are properly shared when multiple Debuggers inspect an Ion frame. Something like this, modulo Ion warmup and such:

var g = newGlobal();
var dbg1 = new Debugger(g);
var dbg2 = new Debugger(g);

g.eval('function f(x) { g(); }');
g.eval('function g() { debugger; }');

dbg1.onDebuggerStatement = function (frame) {
  assertEq(frame.callee.name, 'g');
  frame.older.environment.setVariable('x', "not a number at all");
  assertEq(dbg2.getNewestFrame().older.getVariable('x'), "not a number at all");
}
g.f(42);

Comment 90

[Jim Blandy :jimb]

Comment on attachment 8400361 [details] [diff] [review]
Part 6: Tests.

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

Could we have a test that modifies an Ion frame (causing a remat frame to be created), and then removes that frame's global as a debuggee (thus taking the compartment out of debug mode), and verifies that the modifications captured in the remat frame still take effect? In other words, we should test that remat frames' changes are respected even when debug mode is off and the Debugger that created them is long gone.

Comment 91

[Shu-yu Guo [:shu]]

Attachment: More tests

Additional tests that Jim pointed out that should be added.

Comment 92

[Jim Blandy :jimb]

Comment on attachment 8400314 [details] [diff] [review]
Part 5: Relax the no on-stack scripts restriction for addDebuggee.

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

On IRC, you said you were going to update this patch; so I'll just publish some dumb comments I had on the original patch first.

::: js/src/vm/Debugger.cpp
@@ +1356,5 @@
>  
>  JSTrapStatus
>  Debugger::fireNewGlobalObject(JSContext *cx, Handle<GlobalObject *> global, MutableHandleValue vp)
>  {
> +    RootedObject hook(cx, getHook(OnNewGlobalObject));

It seems like the patch is inserting a carriage return here; let's not do that.

@@ +1659,5 @@
>               */
> +            for (GlobalObjectSet::Enum e(dbg->debuggees); !e.empty(); e.popFront()) {
> +                // We can't recompile on-stack scripts here, and we can only
> +                // toggle debug mode to off, so removeDebuggeeGlobal must
> +                // be infallible.

"so we use an infallible variant of removeDebuggeeGlobal."

Comment 93

[Shu-yu Guo [:shu]]

Attachment: Part 5: Relax the no on-stack scripts restriction for addDebuggee.

- Fix propagating remat frame mutations after debug mode is turned off
- Applied comments

Comment 94

[Shu-yu Guo [:shu]]

Attachment: Part 5 1/2: Support rematerialized frames in DebugScopes::updateLiveScope.

Luke, you wrote the DebugScope stuff so I'm flagging you for this part.

The gist of RematerializedFrames is this: they are structs with information copied out of Ion snapshots used by Debugger to interact with Ion frames. When Ion frames are rematerialized, they are also invalidated. When the invalidation thunk runs, any mutations done to the RematerializedFrame is copied to the newly constructed BaselineFrame. They are a new variant serviceable by AbstractFramePtr, so they should play in the whole DebugScopes business.

This whole patch set is pretty complicated, let me know if you have any questions about the details.

Comment 95

[Shu-yu Guo [:shu]]

Comment on attachment 8408703 [details] [diff] [review]
Part 5 1/2: Support rematerialized frames in DebugScopes::updateLiveScope.

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

::: js/src/vm/ScopeObject.cpp
@@ +2018,5 @@
>       */
>      for (AllFramesIter i(cx); !i.done(); ++i) {
> +        if (i.isIon()) {
> +
> +        }

Err, ignore this empty conditional. Had it in for debugging and forgot to take it out.

Comment 96

[Shu-yu Guo [:shu]]

Attachment: Part 6 1/2: Add shell function to deterministically request interrupt

Adds new functions to support deterministic triggering of interrupts to the
shell and replaces timeout() uses by them.

Comment 97

[Shu-yu Guo [:shu]]

Try push here for the whole patch set: https://tbpl.mozilla.org/?tree=Try&rev=0ada0b85b9c6

Comment 98

[Shu-yu Guo [:shu]]

So the tests are still failing on 10.6 and XP. Debugging some more, I think it's because when exactly an interrupt gets serviced when signal handlers are in place is not deterministic across systems, and behaves slightly differently on 10.6 and XP.

I am not in the mood or have the patience to fix signal handling to be consistent across platforms. I am going to rewrite the test to test for eventually succeeding instead of succeeding the first time. That is, we will keep interrupting until we get an Ion frame, and then break out of the infinite loop there.

Comment 99

[Shu-yu Guo [:shu]]

Much better with rewritten test: https://tbpl.mozilla.org/?tree=Try&rev=be33b96ad263

Comment 100

[Shu-yu Guo [:shu]]

Attachment: Part 6 1/2: Add shell function to deterministically request interrupt

Rewritten to have more interrupt points.

Comment 101

[Jim Blandy :jimb]

Comment on attachment 8409351 [details] [diff] [review]
Part 6 1/2: Add shell function to deterministically request interrupt

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

Looks great --- thanks for taking the time to make this deterministic.

::: js/src/jit-test/tests/debug/Frame-eval-19.js
@@ +25,5 @@
>      return true;
>    });
>  
> +  g.done = false;
> +  g.eval("(" + function () { for (i = 0; i < 5; i++) f(false); while (!done) f(true); } + ")();");

Couldn't this test now be tightened / simplified to just call f(false) five times, then f(true) once, and assert that done is true?

::: js/src/shell/js.cpp
@@ +4664,5 @@
> +"  Requests interrupt callback if cond is true. No-op otherwise."),
> +
> +    JS_FN_HELP("setInterruptCallback", SetInterruptCallback, 1, 0,
> +"setInterruptCallback(func)",
> +"  Sets func as the interrupt callback function."),

What is the meaning of the callback function's return value?

You might want to note, here and in the 'timeout' docstring, that this replaces the callback passed to timeout, and vice versa.

Comment 102

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #101)
> Couldn't this test now be tightened / simplified to just call f(false) five
> times, then f(true) once, and assert that done is true?

This comment doesn't need to be addressed. Shu explained on IRC that interrupts don't get noticed when running JIT code on OSX 10.6, since 10.6 enables execution on any page that permits any access at all. There's a patch under review to work around that, but for the time being, we need the 'while' in that test.

Comment 103

[Jim Blandy :jimb]

Comment on attachment 8378700 [details] [diff] [review]
Part 3: Support rematerializing Ion frames on the stack.

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

::: js/src/vm/Stack.cpp
@@ -734,5 @@
>      return *this;
>  }
>  
>  ScriptFrameIter::Data *
>  ScriptFrameIter::copyData() const

On IRC, Shu and I concluded that this function should be asserting that 'this' is not an AsmJS frame.

Comment 104

[Jim Blandy :jimb]

Comment on attachment 8408430 [details] [diff] [review]
Part 5: Relax the no on-stack scripts restriction for addDebuggee.

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

Okay, I think I actually understand the Debugger.cpp changes. Here are my comments there. Review still in progress.

::: js/src/jit/Ion.cpp
@@ +3036,5 @@
>      return true;
>  }
>  
> +bool
> +jit::ToggleDebugMode(JSContext *maybecx, JSCompartment *comp,

This function doesn't actually toggle anything. It should be called something like updateJITForDebugMode, after JSCompartment::updateForDebugMode.

::: js/src/vm/Debugger.cpp
@@ +2111,5 @@
>      /* Since there may be multiple contexts, use AllFramesIter. */
>      for (AllFramesIter i(cx); !i.done(); ++i) {
> +        if (dbg->observesFrame(i)) {
> +            if (i.isIon() && !i.ensureHasRematerializedFrame())
> +                return false;

It might be nice to comment that giving i a rematerialized frame here is necessary to allow the loop below to find it.

Also, hurrah.

@@ -2251,5 @@
> -    /* Refuse to enable debug mode for a compartment that has running scripts. */
> -    if (!debuggeeCompartment->debugMode() && debuggeeCompartment->hasScriptsOnStack()) {
> -        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_DEBUG_NOT_IDLE);
> -        return false;
> -    }

And hurrah.

@@ +3653,5 @@
> +Debugger::handleIonBailout(JSContext *cx, jit::RematerializedFrame *from, jit::BaselineFrame *to)
> +{
> +    ScriptFrameIter iter(cx);
> +    while (iter.abstractFramePtr() != to)
> +        ++iter;

I had to ask why we're iterating over frames here; it would be nice to have a comment saying:

When we return to a bailed-out Ion real frame, we must update all Debugger.Frames that refer to its inline frames. However, since we can't pop individual inline frames off the stack (we can only pop the real frame that contains them all, as a unit), we cannot assume that the frame we're dealing with is the top frame. Advance the iterator across any inlined frames younger than |from|.

::: js/src/vm/Debugger.h
@@ -303,4 @@
>      void markKeysInCompartment(JSTracer *tracer);
>  
>      static const Class jsclass;
> -

Let's leave this blank line here.

::: js/src/vm/Stack.cpp
@@ +1074,5 @@
> +{
> +    switch (data_.state_) {
> +    case DONE:
> +    case ASMJS:
> +        break;

Would it make sense to tighten this switch up a bit by adding a  default: MOZ_ASSUME_UNREACHABLE(...) branch? That is, we should either be omitting the 'false' cases, and taking advantage of the no-matching-case behavior; *or* we should try to be exhaustive.

@@ +1095,4 @@
>  AbstractFramePtr
>  FrameIter::abstractFramePtr() const
>  {
> +    MOZ_ASSERT(hasUsableAbstractFramePtr());

It was reassuring to see this here, and know that hasUsableAbstractFramePtr has no false negatives.

Comment 105

[Luke Wagner [:luke]]

Comment on attachment 8408703 [details] [diff] [review]
Part 5 1/2: Support rematerialized frames in DebugScopes::updateLiveScope.

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

I'm completely unfamiliar with RematerializedFrame, so probably Jan should review this bit.

Comment 106

[Jim Blandy :jimb]

I understand that we bail out all Ion frames and invalidate all scripts when we change debug mode. It seems to me we should have tests that verify that this has actually occurred. Invalidation is supposed to be unobservable, but we could observe if it fails to occur. So, two test suggestions:

- With Ion frames on the stack, add their global as a debuggee, set an onPop handler on the frames, and observe that they fire when expected.

- With Ion frames on the stack, add their global as a debuggee, set a breakpoint, and observe that it is hit. This proves that the frame has stopped running the Ion code, which can't contain breakpoints.

Comment 107

[Jim Blandy :jimb]

Comment on attachment 8408703 [details] [diff] [review]
Part 5 1/2: Support rematerialized frames in DebugScopes::updateLiveScope.

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

All this does is implement AbstractFramePtr::prevUpToDate for remat frames too, right?

Comment 108

[Jim Blandy :jimb]

Comment on attachment 8408430 [details] [diff] [review]
Part 5: Relax the no on-stack scripts restriction for addDebuggee.

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

Looks good to me, with many comments.

::: js/src/jit/Ion.cpp
@@ +3078,5 @@
>      jit::MarkActiveBaselineScripts(zone);
>  
>      for (JitActivationIterator iter(rt); !iter.done(); ++iter) {
>          JSCompartment *comp = iter.activation()->compartment();
> +        if (comp_ == comp || zone_ == comp->zone()) {

There's nothing going on here, and in the hunk below, beyond simplifying the expressions in light of the fact that comp and comp->zone() are known to be non-null, right?

::: js/src/jscompartment.cpp
@@ +769,1 @@
>      // We do allow disabling debug mode while scripts are on the stack.  In

Without that first paragraph, the contrastive 'do' here has lost its "contrastee".

@@ +826,5 @@
>          return false;
>      }
>      debugModeBits |= DebugFromJS;
> +    if (!wasEnabled && !updateJITForDebugMode(cx, invalidate)) {
> +        js_ReportOutOfMemory(cx);

Since updateJITForDebugMode is fallible iff it's passed a JSContext, it seems like it should be able to take care of calling js_ReportOutOfMemory itself. As I understand it, SpiderMonkey convention is to place these calls as close to the actual allocation as possible, rather than leaving it an undocumented requirement on callers to take care of reporting the failure.

And it seems like updateJITForDebugMode actually takes care of this anyway. So I don't understand why this call is needed at all.

@@ +849,5 @@
>                                js::GlobalObjectSet::Enum *debuggeesEnum)
>  {
>      bool wasEnabled = debugMode();
> +    removeDebuggeeUnderGC(cx->runtime()->defaultFreeOp(), global, invalidate, debuggeesEnum);
> +    if (debuggees.empty() && wasEnabled && !debugMode()) {

This condition is surprising. Doesn't !debugMode() imply debuggees.empty(), even at this mid-state-transition point?

@@ +851,5 @@
>      bool wasEnabled = debugMode();
> +    removeDebuggeeUnderGC(cx->runtime()->defaultFreeOp(), global, invalidate, debuggeesEnum);
> +    if (debuggees.empty() && wasEnabled && !debugMode()) {
> +        if (!updateJITForDebugMode(cx, invalidate)) {
> +            js_ReportOutOfMemory(cx);

Similarly.

::: js/src/vm/GlobalObject.cpp
@@ +761,5 @@
>  #endif
>      if (debuggers->empty() && !global->compartment()->addDebuggee(cx, global))
>          return false;
> +    if (!debuggers->append(dbg))
> +        return global->compartment()->removeDebuggee(cx, global);

This change seems wrong. If the append fails, there's no way addDebugger should ever return true.

If the removeDebuggee call fails, that's sort of a double fault situation; if we know it can only occur due to OOM, and that it leaves things in a coherent state, it's probably okay to ignore it.

::: js/src/vm/Stack.cpp
@@ -1073,5 @@
>          MOZ_ASSERT(data_.ionFrames_.isIonJS());
> -        jit::RematerializedFrame *frame =
> -            activation()->asJit()->lookupRematerializedFrame(data_.ionFrames_.fp(),
> -                                                             ionInlineFrames_.frameNo());
> -        MOZ_ASSERT(frame);

I don't understand why this assertion is going away.  lookupRematerializedFrame can return null, but returning a null AbstractFramePtr seems like bad news.

Comment 109

[Jim Blandy :jimb]

Comment on attachment 8400361 [details] [diff] [review]
Part 6: Tests.

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

The idea behind the .implementation assertions was that, if changes in the implementation caused the tests not to exercise what they're intended to, we'd find out immediately. However, this inescapably makes the tests sensitive to implementation changes, putting the test authors (and reviewers!!) on the hook to help sort out future breakage.

For what it's worth, as the person who suggested the .implementation checks, if they turn out to be a maintenance burden, I'm fine with deleting the assertions and letting the tests cover whatever they cover.

::: js/src/jit-test/tests/debug/Frame-eval-19.js
@@ +4,5 @@
> +
> +if (!jitTogglesMatch(Opts_Ion2NoParallelCompilation))
> +  quit(0);
> +
> +withJitOptions(Opts_Ion2NoParallelCompilation, function () {

Out of curiosity, why is it necessary to disable parallel compilation? Perhaps a comment in jitopts.js would help.

@@ +12,5 @@
> +  g.eval("" + function f(d) { g(d); });
> +  g.eval("" + function g(d) { h(d); });
> +  g.eval("" + function h(d) { while (d); });
> +
> +  timeout(1, function () {

I see that this is patched by part 6 1/2 to use 'interruptif'.

@@ +16,5 @@
> +  timeout(1, function () {
> +    dbg.addDebuggee(g);
> +    var frame = dbg.getNewestFrame();
> +    assertEq(frame.implementation == "ion" ||
> +             frame.implementation == "ion-inlined", true);

All of these tests treat "ion" and "ion-inlined" frames identically. Would it make sense to have D.F.p.implementation just call them both "ion", simplify these tests, and have a separate D.F.p.inlined accessor that reflects whether the frame is non-oldest in its real Ion frame?

::: js/src/jit-test/tests/debug/Frame-eval-20.js
@@ +20,5 @@
> +                 f.implementation == "ion-inlined", true);
> +        assertEq(f.environment.getVariable("foo"), 42);
> +
> +        // EIF of a different type too.
> +        f.eval((shadow ? "var " : "") + "foo = 'string of 42'");

It would be nice to have tests that actually *introduce* a shadowing binding, which the spec says should work. But that doesn't work at all right now; filed as bug 999858.

@@ +28,5 @@
> +
> +    g.eval("" + function f(d) { var foo = 42; g(d); return foo; });
> +    g.eval("" + function g(d) { h(d); });
> +
> +    g.eval("(" + function () { for (i = 0; i < 5; i++) f(false); assertEq(f(true), "string of 42"); } + ")();");

One of the benefits of using the implicit toString of a function was to let you indent the code normally without sprinkling quotes and concatenations everywhere. Can we spread this and other similar examples across the natural number of lines?

::: js/src/jit-test/tests/debug/Frame-implementation-01.js
@@ +37,5 @@
> +  function (f) { assertEq(f.implementation == "ion" ||
> +                          f.implementation == "ion-inlined", true); }],
> + [Opts_NoJits,
> +  function (f) { assertEq(f.implementation, "interpreter"); }]].forEach(function (t) {
> +    testFrameImpl(t[0], t[1]);

It would be spiffier to say:

.forEach(function ([opts, f]) { testFrameImpl(opts, f); })

Comment 110

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d34458e80bcb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b30afb9de404
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/061ebab47be3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a19a7c0a4b04
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c92f83e9a864
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0e61e88866ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a49c8cdf14c4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/079f4f0ed6a6

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

Comment 111

[Jan de Mooij [:jandem]]

AWFY shows a 31.6% regression on SS validate-input. Maybe we can bisect to see which part caused it?

Comment 112

[Shu-yu Guo [:shu]]

Investigating.

Comment 113

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d34458e80bcb
https://hg.mozilla.org/mozilla-central/rev/b30afb9de404
https://hg.mozilla.org/mozilla-central/rev/061ebab47be3
https://hg.mozilla.org/mozilla-central/rev/a19a7c0a4b04
https://hg.mozilla.org/mozilla-central/rev/c92f83e9a864
https://hg.mozilla.org/mozilla-central/rev/0e61e88866ce
https://hg.mozilla.org/mozilla-central/rev/a49c8cdf14c4
https://hg.mozilla.org/mozilla-central/rev/079f4f0ed6a6

Comment 114

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #111)
> AWFY shows a 31.6% regression on SS validate-input. Maybe we can bisect to
> see which part caused it?

The regression is caused by the following pattern:

OSR -> Bailout -> OSR

When we bail out back to baseline, one of the slots is optimized out and is thus MagicValue(JS_OPTIMIZED_OUT). When we recompile the script for OSR, we add a type barrier for a typeset that contains undefined, the type (incorrectly) given to optimized out values inside the BaselineFrameInspector. This causes an unnecessary bailout.

Comment 115

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/797c8dfa7c6d

Comment 116

[Till Schneidereit [:till]]

Bisecting bug 1001372, the crash is cause by part 5[1]. Build with --disable-threadsafe to reproduce.


[1]: http://hg.mozilla.org/mozilla-central/rev/c92f83e9a864

Comment 117

[Hannes Verschore [:h4writer]]

Fix tracelogger after part 4 changed script from HandleScript to JSScript * in baseline:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe7d0fd049c

Comment 118

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fbe7d0fd049c

Comment 119

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7818fa19c4c5