PJS: Capture stack frames on bail out for debugging

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Depends on: 1019310
Assignee

Updated

5 years ago
Depends on: 1022891
Assignee

Comment 2

5 years ago
- 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)
Assignee

Comment 3

5 years ago
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+
Assignee

Comment 6

5 years ago
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+
Assignee

Comment 9

5 years ago
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)
Assignee

Updated

5 years ago
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+

Updated

5 years ago
Depends on: 1028734

Comment 12

5 years ago
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: 1029440
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: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → shu
Target Milestone: --- → mozilla33
Version: unspecified → Trunk
Depends on: 1029910
Depends on: 1032264
Depends on: 1034280
Depends on: 1074305
You need to log in before you can comment on or make changes to this bug.