923724 - Inspect Ion stack frames in the Debugger, very carefully

Status: RESOLVED DUPLICATE of bug 716647

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This is the first step toward fixing bug 716647.

The specific test case we want to enable in this bug will be like this:

  - build up some stack frames, in Ion, baseline, whatever

  - then create a special Debugger object (using a new and explicitly unsafe
    API,  new UnsafeOnStackDebugger)

  - use it to examine the stack, including finding out which frames are
    optimized to what degree (new API, frame.mode ∈ {"interpreter", "baseline",
    "ion", "ion-inlined"})

  - then disable that special Debugger and continue, without crashing

and none of the above should invalidate any on-stack JIT frames.

(There are many possible test cases for this because the first step can be done in many ways -- short/tall stacks, more and less optimized, recursion, calling in and out of C++, self-hosted code, different kinds of closures...)

Only frame.script, .offset, .callee, and .older (and the new .mode) have to work. Everything else can throw.

Comment 1

[Jason Orendorff [:jorendorff]]

Most of the work here is changing what Debugger.Frames have in them, and the hash table that stores them, to be able to cope with Ion frames.

Comment 2

[Shu-yu Guo [:shu]]

Jim, what's your time frame for this? I wouldn't mind taking a crack at it if you haven't started already.

Comment 3

[Jim Blandy :jimb]

Shu and I spoke about this on IRC, and he's going to take a shot at it.

Comment 4

[Shu-yu Guo [:shu]]

I have not been able to find a way to preserve frame identity for Ion-inlined frames without incurring a perf regression.

Jim and I had talked about hijacking CheckOverRecursed for also handling syncing Debugger frames, but I didn't realize until too late that inlined frames don't have such a check (as expected, not like we can recur from inlined frames). I then played with having a toggled jmp (between |cmp eax L| and |jmp L|) for the initial goto for the inlined call, but many cases of inlining result in no jmp at all, as the inlined block is often the immediate next block, and adding a |cmp eax L| where there was nothing before hurts (5% on DeltaBlue!).

We can still hijack CheckOverRecursed for non-inlined frames, but it doesn't seem possible to preserve identity of inlined frames. I suggest we reflect the fact that inline frames are rematerialized by adding a new |.rematerialized| property for them and sealing them, so that the user does not add new properties to them.

Thoughts?

Comment 5

[Shu-yu Guo [:shu]]

A second API option is to make Ion-inlined frames invisible in the Debugger parlance, but have a |rematerializeInlineFrames()| method on optimized frames. This method will return a newly constructed array of |Debugger.InlineFrame|s, or instances of some other appropriately named new type.

I don't like this option as much as what I first suggested, as the first option would be more amenable to a "deopt-and-copy-on-write" style for working with Ion frames, by which I mean writing to certain accessors or assigning certain hooks would deopt the Ion scripts and copy the data written to a corresponding baseline frame.

A third option would be to do what Jan did in bug 716647: bail out immediately and recompile baseline for debug mode, so you only get to observe these rematerialized frames once. This is a nonstarter for what I want, as I'd like to be able to expand these frames in the future to have things like onDeoptimization hooks.

Comment 6

[Shu-yu Guo [:shu]]

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

Attaching some prelim patches for r? while still polishing the bigger patches.

Comment 7

[Shu-yu Guo [:shu]]

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

This is in preparation to handle the onExceptionUnwind hook. Jan, could you
double check my logic about bailing out to baseline, especially the interaction
with stack depth part + iterators?

Comment 8

[Shu-yu Guo [:shu]]

Ignore the comments around the if in HandleExceptionIon, I had the if commented out to test for correctness.

Comment 9

[Jan de Mooij [:jandem]]

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

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

Looks good. Nice to see that we can reuse most of the bailout-for-try-catch code.

It'd be good to start writing tests for the new debugging code ASAP though even if it temporarily requires some testing-only flag. It ensures we don't completely break this code and once everything works we can remove the flag and we still have good tests.

::: js/src/jit/IonFrames.cpp
@@ +335,5 @@
>      RootedScript script(cx, frame.script());
>      jsbytecode *pc = frame.pc();
>  
> +    bool bailedOutForDebugMode = false;
> +/*

Don't forget to remove the comments.

@@ +403,1 @@
>                      return;

Nit: MOZ_ASSERT(!cx->isExceptionPending()); after the return.

Comment 10

[Jan de Mooij [:jandem]]

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

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

Forgot a comment.

::: js/src/jit/BaselineBailouts.cpp
@@ +1518,5 @@
>          break;
> +      case Bailout_IonExceptionDebugMode:
> +        // Return false to resume in HandleException with reconstructed
> +        // baseline frame.
> +        return false;

JS_ASSERT(cx->isExceptionPending()); here would be good.

::: js/src/jit/IonFrames.cpp
@@ +335,5 @@
>      RootedScript script(cx, frame.script());
>      jsbytecode *pc = frame.pc();
>  
> +    bool bailedOutForDebugMode = false;
> +/*

Don't forget to remove the comments.

@@ +403,1 @@
>                      return;

Nit: MOZ_ASSERT(!cx->isExceptionPending()); after the return.

Comment 11

[Shu-yu Guo [:shu]]

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

Comment 12

[Shu-yu Guo [:shu]]

Attachment: wip Part 4: Recompile on-stack baseline scripts when toggling debug mode. (r=?)

Comment 13

[Shu-yu Guo [:shu]]

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

Adapted from parts your patch in bug 716647. Adds capability to rematerialize live Ion frames. 

Note that the unit of rematerialization is an uninlined frame + its inlined frames all at once, to plan for the future 'jit transparency' mode for the Debugger where we don't automatically invalidate scripts, but can synchronize rematerialized frames by hijacking CheckOverRecursed, which only exists on uninlined frame entry.

This part of the API should be mostly stable, so asking for r?.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 823159 [details] [diff] [review]
wip Part 4: Recompile on-stack baseline scripts when toggling debug mode. (r=?)

Also adapted from your patch in bug 716647. This is the parts that adds the ability to recompile live baseline frames on the stack for debug mode.

One additional snag that your previous patch didn't address is the marking of stub pointers in BaselineStub frames and protecting against relocation of stub pointers on the C stack. For the former problem, we just patch the BaselineStub frames. For the latter, I introduce a |DebuggerVolatile<T>| that keeps a list of 'rooters' in |IonRuntime|. I'll add the MOZ_STACK_CLASS temp protection magic incantations later.

I'm not that familiar with baseline so asking for f?

Comment 15

[Shu-yu Guo [:shu]]

Things are in enough working order that I will write tests soon.

Comment 16

[Jan de Mooij [:jandem]]

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

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

Awesome, much nicer than the prototype! :)

::: js/src/jit/IonFrames.h
@@ +308,5 @@
> +//
> +// An optimized frame that has been rematerialized with values read out of
> +// Snapshots.
> +//
> +class RematerializedFrame

How about moving this class to a new file, maybe something we can also use for other debugging-only stuff?

::: js/src/jsobj.cpp
@@ +5430,5 @@
>  JS_FRIEND_API(void)
>  js_DumpValue(const Value &val)
>  {
>      dumpValue(val);
> +    if (!val.isString())

Is this change intentional?

::: js/src/vm/Debugger.cpp
@@ +3400,5 @@
>      return observesGlobal(&script->global()) && !script->selfHosted;
>  }
>  
>  /* static */ bool
> +Debugger::replaceFrameGuts(JSContext *cx, AbstractFramePtr from, AbstractFramePtr to,

Nice that we can reuse this code.

::: js/src/vm/Stack.cpp
@@ +1287,5 @@
>  {
>      if (active_) {
>          cx_->mainThread().ionTop = prevIonTop_;
>          cx_->mainThread().ionJSContext = prevIonJSContext_;
> +        clearRematerializedFrames();

Can we either move this out of the active_ |if| or assert the table is empty in the |else| case? Just to make sure we don't leak anything if !active_.

@@ +1378,5 @@
> +            if (!frame)
> +                return nullptr;
> +            p->value[frame->frameNo()] = frame;
> +
> +            frame->dump();

Nit: remove this.

::: js/src/vm/Stack.h
@@ +6,5 @@
>  
>  #ifndef vm_Stack_h
>  #define vm_Stack_h
>  
> +#include "mozilla/LinkedList.h"

Nit: unused #include?

@@ +128,5 @@
>          JS_ASSERT(res);
>          return res;
>      }
>      bool isBaselineFrame() const {
> +        return ptr_ && !isStackFrame() && !isRematerializedFrame();

Nit: return ptr_ && (ptr_ & 0x3) == 0x0; Also instead of 0x3 here and elsewhere we could have a

static const size_t TagMask = 0x3;

Or something similar.

@@ +1316,5 @@
>  
> +    // Rematerialized Ion frames which has info copied out of snapshots.
> +    typedef Vector<RematerializedFrame *, 1> RematerializedFrameVector;
> +    typedef HashMap<uint8_t *, RematerializedFrameVector> RematerializedFrameTable;
> +    RematerializedFrameTable rematerializedFrames_;

Nit: add a short comment here to explain this table is lazily initialized, in case somebody is worried about malloc overhead for every JitActivation.

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 823159 [details] [diff] [review]
wip Part 4: Recompile on-stack baseline scripts when toggling debug mode. (r=?)

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

Looks good but there are some problems with stub frames and stub pointers.

Kannan do you have any thoughts on this? Can you think of other places that might break when we recompile code for scripts on the stack?

::: js/src/jit/BaselineBailouts.cpp
@@ +12,1 @@
>  #include "vm/ArgumentsObject.h"

Nit: Debugger comes after ArgumentsObject.

::: js/src/jit/BaselineIC.cpp
@@ +5979,5 @@
>      if (!obj)
>          return false;
>  
> +    // We could invoke a getter that toggles debugging mode.
> +    DebuggerVolatile<ICGetProp_Fallback *> vstub(cx, stub);

It would be nice if DebuggerVolatile took a pointer-to-stub, so that you can set it to nullptr to make sure nobody will access it after this point without crashing.

More importantly, even something like x + y could toggle debug mode due to valueOf/toString calling arbitrary JS so we will just have to do this for all stubs I think. An alternative is to store a pointer to the original non-debug-mode BaselineScript in the debug-mode BaselineScript so that we don't have to handle stub pointers at all. What do you think?

::: js/src/jit/BaselineJIT.cpp
@@ +983,5 @@
> +            JSScript *script = iter.script();
> +            uint8_t *retAddr = iter.returnAddressToFp();
> +            ICEntry &entry = script->baselineScript()->icEntryFromReturnAddress(retAddr);
> +            MOZ_ASSERT(entry.isForOp());
> +            if (!entries.append(RecompileEntry(script, entry.pcOffset(), prevFrameStubPtr)))

There's not necessarily a BaselineStub frame for every BaselineJS frame: most stubs use tail calls to call into the VM. Just calling setReturnAddress on the previous frame should work I think.

@@ +1039,5 @@
> +            uint32_t pcOffset = entries[i].pcOffset;
> +            MOZ_ASSERT(script == iter.script());
> +            MOZ_ASSERT(pcOffset < script->length);
> +
> +            if (script->baselineScript()->debugMode()) {

We can turn this into an assert right?

@@ +1056,5 @@
> +
> +          case IonFrame_BaselineStub: {
> +            JSScript *script = entries[i].script;
> +
> +            if (script->baselineScript()->debugMode()) {

And here.

@@ +1081,5 @@
> +#else
> +                            "?",
> +#endif
> +                            newStub);
> +                    MoveVolatileForDebugMode(cx->runtime(), oldStub, newStub);

We can have a stub pointer on the C stack without having a stub frame. Most stubs use a tail call to call into the VM (see tailCallVM in BaselineIC.cpp) to avoid the need for a stub frame.

@@ +1117,5 @@
> +    IonSpew(IonSpew_BaselineScripts, "Recompiling (%s:%d) for debug mode",
> +            script->filename(), script->lineno);
> +
> +    RootedScript scriptRoot(cx, script);
> +    if (BaselineCompile(cx, scriptRoot) != Method_Compiled) {

Nit: change BaselineCompile to take a JSScript *.

@@ +1120,5 @@
> +    RootedScript scriptRoot(cx, script);
> +    if (BaselineCompile(cx, scriptRoot) != Method_Compiled) {
> +        // We will only fail to recompile for debug mode due to OOM, so
> +        // propagate the error instead of dropping back to interpreter for
> +        // this frame.

Nit: MOZ_ASSERT(status == Method_Error);

This is a dangerous case though because it leaves a frame on the stack with a return address into memory we just freed. It's probably okay if everything nicely propagates OOM but if something doesn't we're into a lot of (security) trouble. We could save the BaselineScript in the RecompileEntry and Destroy() it once everything is done and restore it if there's an error somewhere. Or patch the return address to something invalid... What do you think?

Edit: see also my other comment about saving the original BaselineScript, it would also deal with this problem...

@@ +1131,5 @@
> +
> +bool
> +jit::RecompileActiveBaselineScriptsForDebugMode(JSContext *cx, JSCompartment *comp)
> +{
> +    MOZ_ASSERT(comp->debugMode());

I think we should have an AutoCompartment here to enter comp, because it's not necessarily the current compartment.

@@ +1135,5 @@
> +    MOZ_ASSERT(comp->debugMode());
> +
> +    // We don't need to root these because BaselineCompile cannot GC.
> +    Vector<RecompileEntry> entries(cx);
> +    JSRuntime *rt = comp->runtimeFromMainThread();

Nit: cx->runtime()

::: js/src/jit/BaselineJIT.h
@@ +206,5 @@
>  
> +    void setDebugMode() {
> +        flags_ |= DEBUG_MODE;
> +    }
> +    bool debugMode() {

Micro-nit: bool debugMode() const

::: js/src/jit/Ion.cpp
@@ +183,5 @@
>      functionWrappers_(nullptr),
>      osrTempData_(nullptr),
>      flusher_(nullptr),
> +    ionCodeProtected_(false),
> +    debuggerVolatiles(nullptr)

Nit: debuggerVolatiles_ to match the other members.

::: js/src/jit/shared/IonFrames-x86-shared.h
@@ +509,5 @@
>          uint8_t *fp = reinterpret_cast<uint8_t *>(this);
>          return *reinterpret_cast<ICStub **>(fp + reverseOffsetOfStubPtr());
>      }
> +
> +    inline void setStubPtr(ICStub *stub) {

Nit: don't forget to update IonFrames-arm as well.

Comment 18

[Jan de Mooij [:jandem]]

For anybody following along: note that this bug does a lot more than what's in comment 0 and is more or less morphing into bug 716647.

Comment 19

[Jim Blandy :jimb]

GO SHU GO