1019304 - PJS: Capture stack frames on bail out for debugging

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

To improve debugging bailouts in PJS, we should capture the Ion frames for the threads upon bailout for diagnostics, and possibly hooking up to the debugger front end.

For the most part the code necessary for this is in place. RematerializedFrame landed as part of bug 716647 and need to be patched to work with bailout stack frames, which do not have exit frames. Bailout sites also need to be manually instrumented with more detailed information for the reason of bailing out.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Part 1: Teach RematerializedFrame to rematerialize bailout frames.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Part 2: Overhaul PJS bailout mechanism to be like the normal bailout mechanism.

- Remove ad-hoc IR tracing and AbortPar machinery.
- PJS bailouts use a different handler that unwinds the entire ForkJoin
  stack, but otherwise shares the same code as sequential bailouts.
- Each thread's stack is snapshotted as a RematerializedFrame vector.

Sorry this is a pretty large patch. Let me know if you have questions.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8437224 [details] [diff] [review]
Part 2: Overhaul PJS bailout mechanism to be like the normal bailout mechanism.

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

::: js/src/vm/ForkJoin.cpp
@@ +1795,5 @@
>  // ParallelBailoutRecord
>  
> +ParallelBailoutRecord::~ParallelBailoutRecord()
> +{
> +    js_delete(frames_);

Oops, this should be something like

reset();
js_delete(frames_);

Comment 4

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8437224 [details] [diff] [review]
Part 2: Overhaul PJS bailout mechanism to be like the normal bailout mechanism.

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

This looks pretty good to me!

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8437223 [details] [diff] [review]
Part 1: Teach RematerializedFrame to rematerialize bailout frames.

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

Sorry for the delay!

::: js/src/jit/RematerializedFrame.cpp
@@ +45,5 @@
>  /* static */ RematerializedFrame *
>  RematerializedFrame::New(ThreadSafeContext *cx, uint8_t *top, InlineFrameIterator &iter)
>  {
>      unsigned numFormals = iter.isFunctionFrame() ? iter.callee()->nargs() : 0;
> +    unsigned numActualArgs = Max(numFormals, iter.numActualArgs());

Nit: s/numActualArgs/numArgs would avoid confusion about numActualArgs potentially being != iter.numActualArgs().

::: js/src/vm/Stack.cpp
@@ +1616,2 @@
>  jit::RematerializedFrame *
> +jit::JitActivation::getRematerializedFrame(ThreadSafeContext *cx, T &iter, size_t inlineDepth)

Pre-existing nit: can this be |const T &iter| or does it not compile?

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Part 3: Mark rematerialized frames inside ForkJoin during ForkJoin minor collection.

Snapshotted frames strongly hold on to GC things and need to be marked,
especially during evacuation.

Comment 7

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/45f24290b96e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5322e6721141
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/adc7e2d717a9

Comment 8

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

I had to back these three out in https://hg.mozilla.org/integration/mozilla-inbound/rev/724d46a1b00a for various spidermonkey test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42165671&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=42161984&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=42163846&tree=Mozilla-Inbound

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Part 4: Add MUnreachable to end basic blocks that have bails in them.

Turns out I couldn't easily make MBail a control instruction because it's used
in function inlining, which isn't really hooked up snoopControlFlow. Instead,
introduce MUnreachable for ending assumed-to-be-unreachable basic blocks.

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8443812 [details] [diff] [review]
Part 4: Add MUnreachable to end basic blocks that have bails in them.

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

Looks good. Thanks for making this change! Just one minor nit:

::: js/src/jit/LIR-Common.h
@@ +1198,5 @@
>    public:
>      LIR_HEADER(Bail)
>  };
>  
> +class LUnreachable : public LInstructionHelper<0, 0, 0>

This should inherit from LControlInstructionHelper instead of LInstructionHelper.

Comment 11

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/feaac6c10dc6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7125c33385
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bd4f36fc30
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0fca95e6e0f

Try looked pretty green: https://tbpl.mozilla.org/?tree=Try&rev=f94202f24d18

Comment 12

[Octoploid]

gcc warns:
js/src/vm/ForkJoin.h:284:1: warning: multi-line comment [-Wcomment]
 //           /       \

278 //
279 // The lattice of causes goes:
280 //
281 //       { everything else }
282 //               |
283 //           Interrupt
284 //           /       \
285 //   Unsupported   UnsupportedVM
286 //           \       /
287 //              None
288 //

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Octoploid from comment #12)
> gcc warns:
> js/src/vm/ForkJoin.h:284:1: warning: multi-line comment [-Wcomment]

Filed as bug 1029672.

Comment 14

[Daniel Holbert [:dholbert]]

Comment 11's landing was merged to m-c yesterday, so this should've been closed as FIXED. I assume this wasn't closed because comment 8's earlier backout was in the same push, and made it look like this was still backed out, or something.

In any case, this is now in m-c, with these commits:
https://hg.mozilla.org/mozilla-central/rev/feaac6c10dc6
https://hg.mozilla.org/mozilla-central/rev/cd7125c33385
https://hg.mozilla.org/mozilla-central/rev/c6bd4f36fc30
https://hg.mozilla.org/mozilla-central/rev/a0fca95e6e0f