Closed Bug 1019304 Opened 7 years ago Closed 7 years ago

PJS: Capture stack frames on bail out for debugging

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(4 files)

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.
Depends on: 1019310
Depends on: 1022891
- 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.
Attachment #8437224 - Flags: review?(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]:
-----------------------------------------------------------------

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

Oops, this should be something like

reset();
js_delete(frames_);
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!
Attachment #8437224 - Flags: review?(nmatsakis) → review+
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?
Attachment #8437223 - Flags: review?(jdemooij) → review+
Snapshotted frames strongly hold on to GC things and need to be marked,
especially during evacuation.
Attachment #8443123 - Flags: review?(lhansen)
Attachment #8443123 - Flags: review?(lhansen) → review+
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.
Attachment #8443812 - Flags: review?(sunfish)
Flags: needinfo?(shu)
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.
Attachment #8443812 - Flags: review?(sunfish) → review+
Depends on: 1028734
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 //
Depends on: 1029672
(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 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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee: nobody → shu
Target Milestone: --- → mozilla33
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.