Closed Bug 1134198 Opened 5 years ago Closed 5 years ago

[jsdbg2] Call Debugger.Frame.onPop at the location that caused the frame to unwind, before further scope and frame unwinding

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: tromey, Assigned: shu)

References

Details

Attachments

(9 files, 12 obsolete files)

444 bytes, text/plain
Details
5.26 KB, patch
tromey
: review+
Details | Diff | Splinter Review
6.27 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.29 KB, patch
tromey
: review+
Details | Diff | Splinter Review
1.30 KB, patch
jimb
: review+
Details | Diff | Splinter Review
1.77 KB, patch
tromey
: review+
Details | Diff | Splinter Review
20.41 KB, patch
jimb
: review+
Details | Diff | Splinter Review
22.70 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.34 KB, patch
tromey
: review+
Details | Diff | Splinter Review
Attached file fail2.js
The attached test case examines the frame's "offset" from an onPop
callback.  When run with --baseline-eager, it gets the wrong result --
it reports that the offset is the offset of the "debugger" statement.
However, when run without --baseline-eager, it works.

pokyo. ./js/src/js --baseline-eager /tmp/fail2.js 
debug = 7
pop = 7
uncaught exception: 23
pokyo. ./js/src/js  /tmp/fail2.js 
debug = 7
pop = 21
Thanks for filing and the test case, Tom!

2 bugs here.

1) We only need to manually unwind all scopes during a DebugEpilogue via forced
return due to exception handling. Unexceptional calls to DebugEpilogue don't
need to do anything special.

2) When unwinding all scopes for a forced return, Baseline set the pc to the
first pc instead of the last pc.
Attachment #8566241 - Flags: review?(jdemooij)
I think this approach interacts badly with the patch in bug 1013219.
The basic issue is that it isn't always correct to report the last PC
as the frame-popping PC.

Consider this test case derived from errorcolumnblame.js:

function test(f) {
    try {
        f();
    } catch (e) {
	print("column: "+ e.columnNumber);
	dis(f);
    }
}


test(function() {
//234567890123456789
             throw new Error('a');
});


The output, with both patches installed, is:

pokyo. ./obj-x86_64-unknown-linux-gnu/dist/bin/js /tmp/a.js
column: 0
flags: LAMBDA
loc     op
-----   --
main:
00000:  getgname "Error"
00005:  undefined
00006:  string "a"
00011:  new 1
00014:  throw
00015:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:   10     0 [   0] setline  lineno 12
  2:   12     0 [   0] colspan 13
  4:   12    11 [  11] xdelta  
  5:   12    11 [   0] colspan 6
  7:   12    15 [   4] newline 


The "retrval" is the last instruction -- but it isn't where the frame
is popped.  That happens at PC=14, the throw.  The last instruction is
never executed, so reporting it as the location seem incorrect.

This isn't a problem with the current tree, because the retrval inherits
the most recently emitted location.  However, this is incorrect in other
scenarios; see the associated bugs for the gory details.

In the above the correct answer is "column: 19", not "0"; with a reporting
offset of 14.
(In reply to Tom Tromey :tromey from comment #2)
> I think this approach interacts badly with the patch in bug 1013219.
> The basic issue is that it isn't always correct to report the last PC
> as the frame-popping PC.

The concern you raise is valid. We never fully specced at what point should onPop be called. The ideal, the one that you want, (and after discussion with Jim), is that onPop be called right before *any* unwinding happens, scope-wise or frame-wise. Currently, onPop tends to happen after scope unwinding, but before frame unwinding.

This patch is indeed wrong if we want to shoot for that ideal. I'll look at how feasible that is.
Comment on attachment 8566241 [details] [diff] [review]
Fix pc when calling DebugEpilogue in Baseline during exception handling.

Cancelling since we might be redesigning.
Attachment #8566241 - Flags: review?(jdemooij)
(In reply to Shu-yu Guo [:shu] from comment #3)
> (In reply to Tom Tromey :tromey from comment #2)
> > I think this approach interacts badly with the patch in bug 1013219.
> > The basic issue is that it isn't always correct to report the last PC
> > as the frame-popping PC.
> 
> The concern you raise is valid. We never fully specced at what point should
> onPop be called. The ideal, the one that you want, (and after discussion
> with Jim), is that onPop be called right before *any* unwinding happens,
> scope-wise or frame-wise. Currently, onPop tends to happen after scope
> unwinding, but before frame unwinding.
> 

Oh, since I didn't make it clear. Why scope unwinding affects reporting of .offset is that a script's scope contour is keyed by PC. So we can't unwind scope and not change the .offset.
> This patch is indeed wrong if we want to shoot for that ideal. I'll look at
> how feasible that is.

Thank you.  In retrospect I am not sure that the problem I found is really
a problem added by this patch.  But on the other hand it will block the other
patches anyhow, so maybe it's best to try to fix it all at once.
Summary: Frame.onPop reports incorrect offset with --baseline-eager → trynotes are crazy
Refactoring patch to pave the way for the rewrite.
Attachment #8567453 - Flags: review?(ttromey)
Refactoring patch to pave the way for rewrite.

Jan, I tried to think of a reason why it'd be preferable to check for wrapping
errors every time we want to know if the pending exception is
JS_GENERATOR_CLOSING and couldn't think of one.

If the exception is not JS_GENERATOR_CLOSING, then the wrapping error will
occur regardless of propagating through generator frames or not. If the
exception is JS_GENERATOR_CLOSING, then wrapping is infallible since it's not a
GC thing.
Attachment #8567454 - Flags: review?(jdemooij)
Comment on attachment 8567454 [details] [diff] [review]
Refactor JS_GENERATOR_CLOSED checking.

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

Ah, much nicer!
Attachment #8567454 - Flags: review?(jdemooij) → review+
Attachment #8566241 - Attachment is obsolete: true
This is to avoid extra logic to avoid calling onLeaveFrame multiple times.
Seems logical to unset debuggee-ness once the Debugger doesn't track the frame
anymore.
Attachment #8567541 - Flags: review?(jimb)
Attachment #8567544 - Flags: review?(ttromey)
Summary: trynotes are crazy → [jsdbg2] Call Debugger.Frame.onPop at the location that caused the frame to unwind, before further scope and frame unwinding
Assignee: nobody → shu
Status: NEW → ASSIGNED
Comment on attachment 8567544 [details] [diff] [review]
Update docs for new Debugger.Frame.onPop spec.

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

::: js/src/doc/Debugger/Debugger.Frame.md
@@ +224,5 @@
>      value. The function should return a [resumption value][rv] indicating
>      how execution should proceed. On newly created frames, this property's
>      value is `undefined`.
>  
> +    When this handle is called, its frame is settled on the location that

"handler"

I think you want to actually spell out what's going on, rather than use the verb "settled", which nothing in this file has ever used before. Perhaps:

"When this handler is called, this frame's current execution location, as reflected in its `offset` and `environment` properties, is the operation which caused it to be unwound."
Comment on attachment 8567543 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT.

Logic is wrong.
Attachment #8567543 - Attachment is obsolete: true
Attachment #8567543 - Flags: review?(jdemooij)
Comment on attachment 8567542 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter.

Logic is wrong.
Attachment #8567542 - Attachment is obsolete: true
Attachment #8567542 - Flags: review?(luke)
Comment on attachment 8567541 [details] [diff] [review]
Make Debugger::onLeaveFrame unmark frames as debuggee.

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

I'm a little confused by the changes to the assertions. The intent there is to accommodate the transitional state after the call to onPop but before the frame is really removed, right?

Debugger::inFrameMaps(f) is true if f has any Debugger.Frame instances referring to it. A frame running a script with breakpoints set must be a debuggee frame, even though it may not have any D.Fs. So this change weakens the assertion conditions too much, as it will permit us to call unsetIsDebuggee on frames that must be debuggees because they're running debuggee scripts, just because those frames have no D.Fs.

I can't think of any better way to flag frames as "pop already reported to Debugger" than clearing their isDebuggee flags, though. Perhaps a separate bit, parallel to isDebuggee, on all three frame representations, with all the AbstractFramePtr dynamic dispatch gunk, whose only purpose is to provide a fast rejection in Debugger::onLeaveFrame? What a pain.

If we do make this change, I would think the comments in jscompartment.h talking about when frames must be debuggees would need to be updated. Now it's not the case that a frame running a debuggee script must always be a debuggee frame; we have this special transient state.

::: js/src/vm/Debugger.cpp
@@ +671,5 @@
>      removeFromFrameMapsAndClearBreakpointsIn(cx, frame);
>  
> +    // Unset the frame as a debuggee, now that all its Debugger.Frame
> +    // instances are removed. This prevents onLeaveFrame from being called
> +    // multiple times (e.g., due to exception handling or forced return).

I read your changes to Debugger::onLeaveFrame's assertions to mean that that function might indeed see frames that have had their debuggee flag cleared here previously. So it's not true that Debugger::onLeaveFrame isn't called multiple times on a single frame. Isn't it more accurate to say that Debugger::slowPathOnLeaveFrame doesn't get called multiple times?
Attachment #8567541 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #18)
> Comment on attachment 8567541 [details] [diff] [review]
> Make Debugger::onLeaveFrame unmark frames as debuggee.
> 
> Review of attachment 8567541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little confused by the changes to the assertions. The intent there is
> to accommodate the transitional state after the call to onPop but before the
> frame is really removed, right?
> 
> Debugger::inFrameMaps(f) is true if f has any Debugger.Frame instances
> referring to it. A frame running a script with breakpoints set must be a
> debuggee frame, even though it may not have any D.Fs. So this change weakens
> the assertion conditions too much, as it will permit us to call
> unsetIsDebuggee on frames that must be debuggees because they're running
> debuggee scripts, just because those frames have no D.Fs.
> 

Good catch, I didn't think about the breakpoint case. Hm... the weakening was needed for the first version of the patch. I wonder if it's possible to restructure to code such that the weakening of the assertion isn't necessary and makes this entire patch unneeded.
Do the slower, but much easier to reason about thing.
Attachment #8567541 - Attachment is obsolete: true
Attachment #8567738 - Attachment is obsolete: true
Attachment #8567739 - Flags: review?(jimb)
Thanks for the edits, Jim! Ever the wordsmith.
Attachment #8567544 - Attachment is obsolete: true
Attachment #8567544 - Flags: review?(ttromey)
Attachment #8567740 - Flags: review?(ttromey)
This rewrites the trynotes code, which was necessary to get more desirable
behavior out of Debugger.Frame.onPop.

This could use a careful review, aka, a Luke-quality review.
Attachment #8567742 - Flags: review?(luke)
Mirrors the logic from the interpreter patch into Baseline.
Attachment #8567743 - Flags: review?(jdemooij)
Comment on attachment 8567453 [details] [diff] [review]
Rename assertNotInFrameMaps to inFrameMaps.

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

Looks good.
Attachment #8567453 - Flags: review?(ttromey) → review+
Comment on attachment 8567740 [details] [diff] [review]
Update docs for new Debugger.Frame.onPop spec.

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

One little nit, but otherwise good.

::: js/src/doc/Debugger/Debugger.Frame.md
@@ +226,5 @@
>      value is `undefined`.
>  
> +    When this handler is called, this frame's current execution location, as
> +    reflected in its `offset` and `environment` properties, is the operation
> +    which caused it to be unwound. In frames that are returning or threw an

I think it should read "...returning or throwing an exception..."
Attachment #8567740 - Flags: review?(ttromey) → review+
Comment on attachment 8567540 [details] [diff] [review]
Update tests to reflect new specced behavior on Debugger.Frame.onPop.

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

Looks good, in particular I think it is the logical consequence of the other patches.
Attachment #8567540 - Flags: review?(ttromey) → review+
Comment on attachment 8567742 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter.

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

I'm not really familiar with the debugger invariants being maintained by this patch; probably better to have jimb to review here.

One comment, though:

::: js/src/vm/Interpreter.h
@@ +262,5 @@
>      RootedScript script; /* TryNotIter is always stack allocated. */
>      uint32_t pcOffset;
>      JSTryNote *tn, *tnEnd;
>  
> +    virtual uint32_t stackDepth() const = 0;

Instead of this and the awkward requirement on settle() stated below, how about

template <class StackDepthOp>
class TryNoteIter {
  StackDepthOp getStackDepth_;
  TryNoteIter(StackDepthOp getStackDepth) : getStackDepth_(getStackDepth) {}
  ..

where TryNoteIter calls "getStackDepth_()" when it wants the stack depth?

::: js/src/vm/Stack.cpp
@@ +416,4 @@
>  
>  /*****************************************************************************/
>  
>  // Unlike the other methods of this calss, this method is defined here so that

pre-existing: s/calss/class/
(In reply to Luke Wagner [:luke] from comment #28)
> Comment on attachment 8567742 [details] [diff] [review]
> Call Debugger::onPop at the point that caused the frame to pop before any
> unwinding in the interpreter.
> 
> Review of attachment 8567742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not really familiar with the debugger invariants being maintained by
> this patch; probably better to have jimb to review here.
> 

Okay, very well.

> Instead of this and the awkward requirement on settle() stated below, how
> about
> 
> template <class StackDepthOp>
> class TryNoteIter {
>   StackDepthOp getStackDepth_;
>   TryNoteIter(StackDepthOp getStackDepth) : getStackDepth_(getStackDepth) {}
>   ..

Good idea. I had tried CRTP and inheritance, this is probably simpler.
Attachment #8567742 - Flags: review?(luke)
Switch to Jim on Luke's request.
Attachment #8568909 - Flags: review?(jimb)
Attachment #8567742 - Attachment is obsolete: true
Attachment #8567743 - Attachment is obsolete: true
Attachment #8567743 - Flags: review?(jdemooij)
Attachment #8568910 - Flags: review?(jdemooij)
Comment on attachment 8568909 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter.

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

::: js/src/vm/Interpreter.cpp
@@ +983,5 @@
>  js::UnwindScopeToTryPc(JSScript *script, JSTryNote *tn)
>  {
> +    jsbytecode *pc = script->main() + tn->start;
> +    if (tn->kind == JSTRY_CATCH || tn->kind == JSTRY_FINALLY)
> +        pc -= JSOP_TRY_LENGTH;

Drive-by comment: MOZ_ASSERT(*pc == JSOP_TRY); here.
Comment on attachment 8568910 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT.

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

Nice refactoring but some issues below.

::: js/src/jit/JitFrames.cpp
@@ +575,5 @@
> +    for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) {
> +        JSTryNote *tn = *tni;
> +
> +        switch (tn->kind) {
> +          case JSTRY_CATCH: {

This case assumes cx->isExceptionPending() right? But UnwindIteratorForException in the JSTRY_ITER case can make that false I think.

@@ +643,4 @@
>      RootedScript script(cx, frame.baselineFrame()->script());
>  
> +    if (cx->isExceptionPending()) {
> +        if(cx->compartment()->isDebuggee() && !cx->isClosingGenerator()) {

Nit: add space after "if("

@@ +647,5 @@
> +            switch (Debugger::onExceptionUnwind(cx, frame.baselineFrame())) {
> +              case JSTRAP_ERROR:
> +                // Uncatchable exception.
> +                MOZ_ASSERT(!cx->isExceptionPending());
> +                break;

ProcessTryNotes assumes cx->isExceptionPending(), AFAICS (it should assert that if true), so in this case we don't want the code below calling ProcessTryNotes.

@@ +693,5 @@
>          }
> +
> +        if (script->hasTrynotes())
> +            ProcessTryNotes(cx, frame, si.ref(), rfe, &pc);
> +        frameOk = HandleClosingGeneratorReturn(cx, frame.baselineFrame(), frameOk);

Questions:

(1) ProcessTryNotes can make us enter a finally-block when we are closing a generator, right? Is it OK to call HandleClosingGeneratorReturn in that case?

(2) The if-branch above calls HandleClosingGeneratorReturn and we will call it here too. Even if that's fine, it's confusing and it'd be nice to restructure the code to avoid this.
Attachment #8568910 - Flags: review?(jdemooij)
Comment on attachment 8568910 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT.

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

::: js/src/jit/JitFrames.cpp
@@ +575,5 @@
> +    for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) {
> +        JSTryNote *tn = *tni;
> +
> +        switch (tn->kind) {
> +          case JSTRY_CATCH: {

Good catch, you're right.

@@ +647,5 @@
> +            switch (Debugger::onExceptionUnwind(cx, frame.baselineFrame())) {
> +              case JSTRAP_ERROR:
> +                // Uncatchable exception.
> +                MOZ_ASSERT(!cx->isExceptionPending());
> +                break;

Yes, also true.

@@ +693,5 @@
>          }
> +
> +        if (script->hasTrynotes())
> +            ProcessTryNotes(cx, frame, si.ref(), rfe, &pc);
> +        frameOk = HandleClosingGeneratorReturn(cx, frame.baselineFrame(), frameOk);

1) Yeah, this is tricky, but yeah this is a bug here.

2) This is actually ok but is pretty hard to restructure. I can duplicate a bit of code and split the isDebuggee path off completely, maybe.
Attachment #8567739 - Flags: review?(jimb) → review+
Comment on attachment 8567739 [details] [diff] [review]
Don't call Debugger::slowPathOnLeaveFrame on frames no longer in Debugger frame maps.

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

::: js/src/vm/Debugger.cpp
@@ +604,5 @@
> +    // The onPop handler and associated clean up logic should not run multiple
> +    // times on the same frame. If slowPathOnLeaveFrame has already been
> +    // called, the frame will not be present in the Debugger frame maps.
> +    if (!inFrameMaps(frame))
> +        return frameOk;

Actually, I did have one thing to suggest:

If I remember, inFrameMaps is implemented by creating a FrameRange, and then checking if it's empty. Building a FrameRange isn't expensive, but it's not super-cheap either.

We're probably about to create a FrameRange in the loop below ("Build a list of the recipients"). Instead of calling inFrameMaps, would it be too ugly to create a FrameRange here, check if it's empty for the early return, and then use it in the loop?
(In reply to Jim Blandy :jimb from comment #35)
> Comment on attachment 8567739 [details] [diff] [review]
> Don't call Debugger::slowPathOnLeaveFrame on frames no longer in Debugger
> frame maps.
> 
> Review of attachment 8567739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Debugger.cpp
> @@ +604,5 @@
> > +    // The onPop handler and associated clean up logic should not run multiple
> > +    // times on the same frame. If slowPathOnLeaveFrame has already been
> > +    // called, the frame will not be present in the Debugger frame maps.
> > +    if (!inFrameMaps(frame))
> > +        return frameOk;
> 
> Actually, I did have one thing to suggest:
> 
> If I remember, inFrameMaps is implemented by creating a FrameRange, and then
> checking if it's empty. Building a FrameRange isn't expensive, but it's not
> super-cheap either.
> 
> We're probably about to create a FrameRange in the loop below ("Build a list
> of the recipients"). Instead of calling inFrameMaps, would it be too ugly to
> create a FrameRange here, check if it's empty for the early return, and then
> use it in the loop?

Fine with me.
Address comments. They're all basically fixed by adding an 'again:' label like
in the interpreter's HandleError. Personally I think this use of goto is more
readable than wrapping the entire thing in a loop and setting some control
variable.
Attachment #8568910 - Attachment is obsolete: true
Attachment #8570814 - Flags: review?(jdemooij)
Attachment #8568909 - Attachment is obsolete: true
Attachment #8568909 - Flags: review?(jimb)
Attachment #8571565 - Flags: review?(jimb)
Comment on attachment 8570814 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT.

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

Looks good but these changes are really hard to review, so many cases to consider..

::: js/src/jit/JitFrames.cpp
@@ +590,5 @@
> +
> +    for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) {
> +        JSTryNote *tn = *tni;
> +
> +        switch (tn->kind) {

MOZ_ASSERT(cx->isExceptionPending()); above this line.

@@ +646,3 @@
>  static void
>  HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFromException *rfe,
> +                        jsbytecode *pc)

Nice that we can get rid of the unwoundScopeToPc and calledDebugEpilogue gunk.

@@ +661,4 @@
>      RootedScript script(cx, frame.baselineFrame()->script());
>  
> +again:
> +    if (cx->isExceptionPending()) {

This makes the interpreter and Baseline exception handlers more similar, I think. Maybe we could even have a shared exception handler that takes an AbstractFramePtr? Please compare them and try to make them look the same as much as possible.
Attachment #8570814 - Flags: review?(jdemooij) → review+
Comment on attachment 8571565 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter.

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

I'm concerned about the changes to HandleError. It seems to me that we essentially make three near copies of the try note handling path:

1) frame isDebuggee() and CoveredByCatchOrFinally. This copy is incomplete, as it only needs to process the next note and resume.

2) frame isDebuggee() and not CoveredByCatchOrFinally. This copy calls the onLeaveFrame hook early.

3) frame !isDebuggee(). This skips the onLeaveFrame call.

It seems to me that the only reason for this variation is that JSTRY_ITER notes modify REGS.pc and REGS.sp. If JSTRY_ITER left those alone, then we could simply loop over try notes, returning early as needed for catch and finally clauses, and call onLeaveFrame only after the loop runs to completion. When leaving a frame with a 'finally' clause, onPop should see the pc at the end of the 'finally', and indeed that's where the proposed loop would leave REGS.pc.

And I don't see that JSTRY_ITER has any real need to adjust REGS.pc and REGS.sp. The JSTRY_ITER note specifies the slot containing the iterator, so we can find it. Aside from legacy iterators, no JS runs when we process a TRY_ITER (and if those legacy iterators get stack traces with a less-than-perfect pc, we can live with that).

::: js/src/vm/Interpreter.cpp
@@ +1020,5 @@
> +    regs.pc = regs.fp()->script()->main() + tn->start + tn->length;
> +    regs.sp = regs.spForStackDepth(tn->stackDepth);
> +}
> +
> +class InterpreterFrameStackDepthOp

This could be a class internal to TryNoteIterInterpreter, named merely "StackDepthOp".

::: js/src/vm/Interpreter.h
@@ +260,3 @@
>  class TryNoteIter
>  {
> +    RootedScript script_; /* TryNotIter is always stack allocated. */

If this is the case, let's put MOZ_STACK_CLASS on TryNoteIter.

Also: "TryNotIter"
Attachment #8571565 - Flags: review?(jimb)
I haven't forgotten about this, but won't get cycles to work on it for a bit. I hope to address review in the next 2 weeks sometime.
Blocks: 1003554
(In reply to Jim Blandy :jimb from comment #40)
> Comment on attachment 8571565 [details] [diff] [review]
> Call Debugger::onPop at the point that caused the frame to pop before any
> unwinding in the interpreter.
> 
> Review of attachment 8571565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Interpreter.cpp
> @@ +1020,5 @@
> > +    regs.pc = regs.fp()->script()->main() + tn->start + tn->length;
> > +    regs.sp = regs.spForStackDepth(tn->stackDepth);
> > +}
> > +
> > +class InterpreterFrameStackDepthOp
> 
> This could be a class internal to TryNoteIterInterpreter, named merely
> "StackDepthOp".

I can't, because TryNoteIterInterpreter inherits from TryNoteIter<T>, and T can't be an inner class to TryNoteIterInterpreter. I could virtualize the functor, but I don't think it's a big deal to not be able to define the StackDepthOp as an internal class.

> 
> ::: js/src/vm/Interpreter.h
> @@ +260,3 @@
> >  class TryNoteIter
> >  {
> > +    RootedScript script_; /* TryNotIter is always stack allocated. */
> 
> If this is the case, let's put MOZ_STACK_CLASS on TryNoteIter.
> 
> Also: "TryNotIter"
(In reply to Shu-yu Guo [:shu] from comment #42)
> (In reply to Jim Blandy :jimb from comment #40)
> > Comment on attachment 8571565 [details] [diff] [review]
> > Call Debugger::onPop at the point that caused the frame to pop before any
> > unwinding in the interpreter.
> > 
> > Review of attachment 8571565 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/vm/Interpreter.cpp
> > @@ +1020,5 @@
> > > +    regs.pc = regs.fp()->script()->main() + tn->start + tn->length;
> > > +    regs.sp = regs.spForStackDepth(tn->stackDepth);
> > > +}
> > > +
> > > +class InterpreterFrameStackDepthOp
> > 
> > This could be a class internal to TryNoteIterInterpreter, named merely
> > "StackDepthOp".
> 
> I can't, because TryNoteIterInterpreter inherits from TryNoteIter<T>, and T
> can't be an inner class to TryNoteIterInterpreter. I could virtualize the
> functor, but I don't think it's a big deal to not be able to define the
> StackDepthOp as an internal class.

Oh, hmm. Yes, it was just a suggestion. InterpreterFrameStackDepthOp just seems like internal structure to me, so I was hoping we could narrow its scope and shorten its name and all that good stuff.
Addressed comments and simplified logic. JSTRY_FOR_IN notes no longer changes
the pc and sp of the frame.
Attachment #8571565 - Attachment is obsolete: true
Attachment #8582135 - Flags: review?(jimb)
Updated the JIT part per the simplified logic of the interpreter part. Carrying
r=jandem; not sure if warrants re-review.
Attachment #8570814 - Attachment is obsolete: true
Attachment #8582136 - Flags: review+
Comment on attachment 8582135 [details] [diff] [review]
Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter.

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

::: js/src/vm/Interpreter.cpp
@@ +995,5 @@
>  js::UnwindScopeToTryPc(JSScript *script, JSTryNote *tn)
>  {
> +    jsbytecode *pc = script->main() + tn->start;
> +    if (tn->kind == JSTRY_CATCH || tn->kind == JSTRY_FINALLY)
> +        pc -= JSOP_TRY_LENGTH;

Wouldn't Jandem want a MOZ_ASSERT(*pc == JSOP_TRY) here?

@@ +1027,5 @@
> +    /*
> +     * Set pc to the first bytecode after the the try note to point
> +     * to the beginning of catch or finally or to [enditer] closing
> +     * the for-in loop.
> +     */

I understand that you're just moving code around here, but could we not mix // and /* */ comments so close together?

@@ +1164,2 @@
>  
> +        switch (ProcessTryNotes(cx, si, regs)) {

Since this is now the only call to ProcessTryNotes, wouldn't it make sense to inline it here, and have one less switch in total?

@@ +1174,4 @@
>          }
>  
> +        ok = HandleClosingGeneratorReturn(cx, regs.fp(), ok);
> +        ok = Debugger::onLeaveFrame(cx, regs.fp(), ok);

And this is our backstop, making sure that no matter what, we eventually call the hook, right?
Attachment #8582135 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #46)
> Comment on attachment 8582135 [details] [diff] [review]
> Call Debugger::onPop at the point that caused the frame to pop before any
> unwinding in the interpreter.
> 
> Review of attachment 8582135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Interpreter.cpp
> 
> @@ +1164,2 @@
> >  
> > +        switch (ProcessTryNotes(cx, si, regs)) {
> 
> Since this is now the only call to ProcessTryNotes, wouldn't it make sense
> to inline it here, and have one less switch in total?
> 

Exception handling code is hard to read, I think it's worth it for readability to keep it in a separate function.

> @@ +1174,4 @@
> >          }
> >  
> > +        ok = HandleClosingGeneratorReturn(cx, regs.fp(), ok);
> > +        ok = Debugger::onLeaveFrame(cx, regs.fp(), ok);
> 
> And this is our backstop, making sure that no matter what, we eventually
> call the hook, right?

Yeah.
Comment on attachment 8587575 [details] [diff] [review]
Fix up tests now that onPop and onExceptionUnwind may be called at different locations than previously.

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

Thank you.
Attachment #8587575 - Flags: review?(ttromey) → review+
Depends on: 1252464
Depends on: 1257198
You need to log in before you can comment on or make changes to this bug.