Closed Bug 1003554 Opened 6 years ago Closed 4 years ago

Strange behavior when stepping through try statement

Categories

(DevTools :: Debugger, defect, P1)

28 Branch
x86_64
Linux
defect

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: skybrian, Assigned: tromey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(1 file, 15 obsolete files)

19.68 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.132 Safari/537.36

Steps to reproduce:

Using this script:

<script>
var at = null;
function runTry() {
	at = 4;
	try {
		at = 6;
		throw "something";
	} catch (e) {
		at = 9;
	} finally {
		at = 11;
	}
	at = 13;
}

</script>
<button onClick="runTry()"/>Try</button>

Set a breakpoint at line 4 and single-step though the script.


Actual results:

The debugger visits Line 9 twice, before and after the finally block. (However, it isn't executed the second time.)



Expected results:

Line 9 shouldn't be visited after the finally block.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Component: JavaScript Engine → Developer Tools: Debugger
Product: Core → Firefox
I've put the sample code here for easier testing:
http://htmlpad.org/stepping-bug/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: strange behavior when stepping through try statement → Strange behavior when stepping through try statement
I've appended the bytecode for this example.

If you set a breakpoint at line 11 ("a = 11" in the finally clause),
this corresponds to PC=0085.

Now if you step, the interpreter will execute 'int8 11', 'setgname "at"',
'pop', and then 'retsub' -- stopping after this instruction at PC=0038
('goto 95').

This stop is why the debugger shows that the step moves to line 9 again.

I am not sure what an appropriate fix would be.

One idea is that since "retsub" is only used when generating a "finally",
the interpreter could special case this for the debugger.  That is, when
single-stepping, don't let a "retsub" stop the single step, but instead
keep stepping until the line number changes again.

However, my current understanding is that the line-number logic is currently
in actors/script.js, so this would be pretty ugly.  It would work ok if
script.js can inspect the bytecode, but I don't know if that is possible.

There are lots of ways to do this in the abstract (mark some lines as
artificial, change the Debugger API to handle this automatically, pass
more information to the onStep callback, etc); but I don't have the context
to know what would be the most in keeping with the overall direction.


js> dis(runTry)
flags:
loc     op
-----   --
main:
00000:  bindgname "at"
00005:  int8 4
00007:  setgname "at"
00012:  pop
00013:  try 43 (+30)
00014:  bindgname "at"
00019:  int8 6
00021:  setgname "at"
00026:  pop
00027:  string "something"
00032:  throw
00033:  gosub 79 (+46)
00038:  goto 95 (+57)
00043:  uninitialized
00044:  initlexical 0
00048:  pop
00049:  exception
00050:  initlexical 0
00054:  pop
00055:  bindname "at"
00060:  int8 9
00062:  setname "at"
00067:  pop
00068:  debugleaveblock
00069:  gosub 79 (+10)
00074:  goto 95 (+21)
00079:  finally
00080:  bindgname "at"
00085:  int8 11
00087:  setgname "at"
00092:  pop
00093:  retsub
00094:  nop
00095:  bindgname "at"
00100:  int8 13
00102:  setgname "at"
00107:  pop
00108:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    3     0 [   0] newline 
  1:    4     0 [   0] colspan 1
  3:    4    13 [  13] xdelta  
  4:    4    13 [   0] newline 
  5:    5    13 [   0] try      offset to jump 25
  7:    5    14 [   1] newline 
  8:    6    14 [   0] colspan 2
 10:    6    27 [  13] xdelta  
 11:    6    27 [   0] newline 
 12:    7    27 [   0] colspan 2
 14:    7    43 [  16] xdelta  
 15:    7    43 [   0] newline 
 16:    8    55 [  12] xdelta  
 17:    8    55 [   0] newline 
 18:    9    55 [   0] colspan 2
 20:    9    79 [  24] xdelta  
 21:    9    79 [   0] newline 
 22:   10    79 [   0] colspan 11
 24:   10    80 [   1] newline 
 25:   11    80 [   0] colspan 2
 27:   11    95 [  15] xdelta  
 28:   11    95 [   0] setline  lineno 13
 30:   13    95 [   0] colspan 1
 32:   13   108 [  13] xdelta  
 33:   13   108 [   0] colspan 8

Exception table:
kind      stack    start      end
 catch        0       14       43
 finally      0       14       79
Thanks for the detailed analysis Tom! I think handling this in the Debugger API or even lower inside SpiderMonkey would be preferred. Jim, what do you think?
Flags: needinfo?(jimb)
I talked to Jim yesterday and he suggested modifying the bytecode emitter
to use a different location for the "goto".  In particular it could be
set to the line at the end of the finally block.
Flags: needinfo?(jimb)
(In reply to Tom Tromey :tromey from comment #4)
> I talked to Jim yesterday and he suggested modifying the bytecode emitter
> to use a different location for the "goto".  In particular it could be
> set to the line at the end of the finally block.

Could I convince you to work on this?
(In reply to Eddy Bruel [:ejpbruel] from comment #5)

> Could I convince you to work on this?

Sure!
Assignee: nobody → ttromey
This is what I'm going to test.
(In reply to Tom Tromey :tromey from comment #4)
> I talked to Jim yesterday and he suggested modifying the bytecode emitter
> to use a different location for the "goto".  In particular it could be
> set to the line at the end of the finally block.

After implementing this and trying it out on a variety of test cases, I think
it is not the best approach.  I applied this to various artificial instructions
generated by the bytecode emitter, and it does make stepping behavior more natural.
However, because it changes line numbers for some instructions, it also changes
the notion of what is an entry point.

E.g., in the example in this bug, if you set a breakpoint on the first line
of the "finally" clause, you will see two stops there.

I think a better approach might be to introduce an "artificial" note into
the line table, and then have the debugger APIs know to ignore artificial
instructions.  This will make stepping work properly (the debugger will step
over artificial instructions), while not changing the reported entry points.
Comment on attachment 8532678 [details] [diff] [review]
fix source location of "goto" to end of a "try"

Trying another approach; but this one was mildly wrong anyhow,
as it should have used UpdateSourceCoordNotes at least, and was
missing some pieces (reported in other bugs) as well.
Attachment #8532678 - Attachment is obsolete: true
Plan B for this bug (and bug 1013219 et al) was to add a SRC_ARTIFICIAL
source note.  I added this as a "gettable" source note type and modified
various spots to either add it or to examine it.

However, this approach had an issue.  In some cases the bytecode emitter
may emit a number of artificial instructions, sometimes indirectly via
other calls -- meaning that the resulting patch would require adding
a number of parameters in various spots in the emitter.  There's also
a potential issue that an instruction may only have a single gettable
note, so if any such artificial note also required another note, we would
be stuck.

It seemed simpler, and not any less clean, to instead treat SRC_ARTIFICIAL
as a kind of line number note, meaning "no line number".  I'll try this approach
next.
This patch implements "Plan C" -- adding a new SRC_ARTIFICIAL source
note which acts a bit like a line number note.

In this approach, SRC_ARTIFICIAL does not change the computation of
the "source line" at all.  However, it does set a separate
"artificial" flag when walking the line table.

This patch then marks up various spots in BytecodeEmitter with
SRC_ARTIFICIAL notes.

Finally it changes the debugger to properly handle artificial
instructions: a stepping operation will never stop on an artificial
instruction, and an artificial instruction will never be reported as
an "entry point".

The new test cases exercise this code.

Note there is one spot I did not update -- the code generated to
terminate a function that doesn't have an explicit "return" or
"yield".  This is bug 1013219; I separated this part out into another
patch because it may warrant different treatment.
Duplicate of this bug: 1052319
(In reply to Tom Tromey :tromey from comment #8)

> However, because it changes line numbers for some instructions, it also
> changes
> the notion of what is an entry point.

One thing I realized recently is that one way this happens is because
jsopcode.h:FlowsIntoNext returns true for JSOP_GOSUB.  This means that
for a sequence:

   gosub <finally>  ; line 3
   goto <after-try> ; line 7

... FlowGraphSummary will think there is an edge from line 3 to line 7.
So, the gosub will appear to be an entry point.

However, this can't actually happen -- it is a spot where the flow graph
doesn't fully model the underlying code.


One argument for trying harder with the line-number approach is that it
can mean an extra stop when a scope is popped.  This would let a user
investigate locals after all modifications but before they are removed.



This current patch works by changing the internals to avoid stops at
artificial locations.  It occurred to me, though, that if we wanted to
support (js-)instruction-level single-stepping, then this decision would
have to be delegated to the debugger API user.  In this case one way to
go would be to extend the API proposed in bug 863089 to also report
whether a given instruction is artificial.
This is a tiny update from the previous patch.

In the earlier patch, BytecodeEmitter.cpp:UpdateLineNumberNotes always
emitted a SRC_SETLINE note when switching out of the artificial state.
However, this was only needed when the line number also remained the
same.  This version of the patch makes the line-number resetting a bit
more efficient by special-casing the SRC_SETLINE behavior.
Attachment #8535785 - Attachment is obsolete: true
Attachment #8536702 - Flags: review?(jimb)
Duplicate of this bug: 1109004
Duplicate of this bug: 1108153
Thank you for working on this Tom. You do great work on fixing these source note issues!
Bytecodes associated with no particular source location introduce a new case that has to be handled higher up in the toolchain; I'd like to avoid that if possible.

Would it be possible to associate the 'goto' with the line of its target instruction? Or would that entail rewriting prior source notes, and be a pain? (Don't we rewrite prior source notes already?)
Comment on attachment 8536702 [details] [diff] [review]
mark compiler-generated instructions as SRC_ARTIFICIAL

Clearing review to get this off my plate, but I'd readily believe there is more to the situation than I understand, so please find me on IRC or needinfo me if you don't think my prior suggestion is a good idea to pursue.
Attachment #8536702 - Flags: review?(jimb)
(In reply to Jim Blandy :jimb from comment #19)
> Bytecodes associated with no particular source location introduce a new case
> that has to be handled higher up in the toolchain; I'd like to avoid that if
> possible.
> 
> Would it be possible to associate the 'goto' with the line of its target
> instruction? Or would that entail rewriting prior source notes, and be a
> pain? (Don't we rewrite prior source notes already?)

I will give it a try.  I think this will still require fixing FlowGraphSummary
per comment#14.  Should be doable though.

There are two potential issues, both related.

One is that the current patch sets the artificial flag when leaving a nested
scope.  Simplest here would be to mark this exit as having the location of
the scope's closing brace -- but that means an extra stop.  (But as you note,
we can look at back-patching the source notes, I'll see about that.)

The other issue is patching a retsub.  In this case there is no single following
line, so I think we'll have to live with the extra stop on the closing brace.
When I wrote comment #14, I thought that the gosub was the only spot
introducing new entry points and thus causing weird behavior.  However,
now I think that is incorrect.

Consider the case of a "goto" at the end of the "then" part of an "if"
statement.  I think if we implement the current plan and give this "goto"
the location of the statement following the "if", then the "goto" instruction
will appear to be an entry point of that following line.  It seems to me that
this will mean double stops on such lines -- once on the goto, and once
on the "real" entry point.

Maybe it would be possible to make the flow graph notice when one
entry point for a line post-dominates the other entry points.  Then
the post-dominating entry point would be the canonical entry point.
I suspect that the artificial notes approach would be simpler than this.
Wouldn't it be better to give the goto the same source location as the last statement of the 'then' block?
Having discussed this a bit more, it seems to me that instructions like a then block's final goto, a try block's gosub, and the goto after that gosub, should generally be given the same source location as their predecessors in the flow graph.

If we can correct FlowGraphSummary's understanding of gosub, so that it sees an edge from the gosub to its target, and edges from the retsub to the successors of all the gosubs, then it it doesn't seem to me that that should introduce any surprising stops.
(In reply to Jim Blandy :jimb from comment #23)

> Having discussed this a bit more, it seems to me that instructions like a
> then block's final goto, a try block's gosub, and the goto after that gosub,
> should generally be given the same source location as their predecessors in
> the flow graph.

It seems to me that an instruction can have multiple predecessors but just a
single location.  For example in a nested if:

function f() {
    if (a) {
	if (b) {
	    c();
	} else {
	    d();
	}
    } else {
	e();
    }
    print("x");
}

Here the "goto" at the end of the outermost "then" has two predecessors -- each
branch of the inner "if".

There isn't a consistent location to give this goto, at least not one based
on its predecessors, I think.  One idea might be to teach FlowGraphSummary about
this somehow, but then there's the problem of identifying which instructions
need this special treatment.  I couldn't think of a way to do this without
reintroducing the artificiality concept.

It is possible to just use the location of the end of the "if" (or whatever)
as the location of these artificial instructions.  This means extra stops.
In the case of blocks the stops will be on the closing brace, which isn't so bad;
but I think for single-statement "if"s the user may see weird results.

> If we can correct FlowGraphSummary's understanding of gosub, so that it sees
> an edge from the gosub to its target, and edges from the retsub to the
> successors of all the gosubs, then it it doesn't seem to me that that should
> introduce any surprising stops.

I suppose we can change FlowGraphSummary to simply never consider retsub instructions
as possible entry points.  That would solve this issue; though if the block also
requires scope-popping then the user will still see a stop on the closing brace.
(In reply to Tom Tromey :tromey from comment #25)

> I suppose we can change FlowGraphSummary to simply never consider retsub
> instructions
> as possible entry points.  That would solve this issue; though if the block
> also
> requires scope-popping then the user will still see a stop on the closing
> brace.

On irc Jim suggested having FlowGraphSummary notice instructions that cannot
produce side effects, and use this information to trim the set of entry
points.

<jimb> getAllOffsets should report the smallest set of bytecode offsets that
       dominate all the line's side effects. How's that?
<jimb> In other words, here's where you must set breakpoints to catch
       execution before this line does anything.
We just discussed this on IRC; let me try to summarize our conclusions.

Let's put the example from comment 25 in pseudo-bytecode:

    1:  ... a ...
    2:  IFEQ 9
    3:  ... b ...
    4:  IFEQ 7
    5:  ... c() ...
    6:  GOTO 8
    7:  ... d() ...
    8:  GOTO 10
    9:  ... e() ...
    10: ... print(x) ...

There are two goals here: to make stepping work, and to make breakpoints work.


Stepping is driven by source locations: we single-step until the source location changes, and then we pause. Line entry points aren't relevant to stepping. So simply assigning one bytecode the same source line as another ensures that single-stepping from the first to the second will not pause.

In that light, we can see that assigning the GOTO at offset 6 the source position of either "c()" (its predecessor) or "print(x)" (its eventual successor), and assigning the GOTO at offset 8 the source position of "print(x)" would cause no unexpected pauses at GOTO instructions while stepping, because every branch shares a source location with its immediate predecessor or successor.


Unlike stepping, breakpoints are driven by entry points: Debugger.Script.prototype.getOffsets returns the offsets at which control might enter a given line. And here we have a problem: offset 10 is a successor to both offsets 8 and 9, which have different line numbers. So setting a breakpoint on "print(x)" would get us two pauses after executing "d()". And if we assign offset 6 the source position of "print(x)", then offset 6 becomes a *third* entry point to "print(x)", so we would get *three* breakpoint hits after executing "c()".

This all follows directly from the way getOffsets is specified. But I think the problem is the specification, not the source position assignments. Rather, getOffsets should return a smallest set of offsets that dominate the given line's side effects. For "print(x)", the unique smallest set is { 10 }, which is exactly where we'd want to set our sole breakpoint.

Debugger.Script.prototype.getAllOffsets and getAllColumnOffsets work on the same principle, and would also need to be changed.

The easiest way to implement this, or an acceptable approximation, would be to note that JOF_JUMP opcodes never have side effects, and hence it should always be safe for FlowGraphSummary to slide across such instructions, never including them in a line's set of entry points. (Well, and RETSUB too, which is not JOF_JUMP; perhaps we want a new JOF_ bit for side-effect-free opcodes?)
(In reply to Jim Blandy :jimb from comment #27)
> Rather, getOffsets should return a smallest set of offsets that dominate the
> given line's side effects.

Well, "dominates" isn't the right term here, without some qualification. Also, side effects like leaving block scopes are visible, but not very interesting. I should have said:

"getOffsets should return the smallest set of offsets that dominate the given line's interesting side effects, when coming from any other source line."

and we should add POPBLOCKSCOPE and DEBUGLEAVEBLOCK to the set of opcodes with our new JOF_ bit, because they're not interesting.

Note that this formulation also works with 'for' loop heads, which generate bytecode looking like this:

for (a; b; c)
  d;

    1:  ... a ...
    2:  GOTO 5
    3:  ... d ...
    4:  ... c ...
    5:  ... b ...
    6:  IFNE 3

Here, everything but offset 3 is attributed to the source line "for(...)". 

If the user sets a breakpoint on "for(...)", we would like to insert breakpoints at offsets 1 and 4. And indeed, { 1, 4 } is "a smallest set of offsets that dominates the given line's interesting side effects, when coming from any other source line".

This shows why the "from any other source line" qualification is needed: offset 1 definitely dominates 4; but because control enters 3, which is a different source line, we need to include 4 in our set.
I guess JOF_UNINTERESTING is my preferred form of Tom's SRC_ARTIFICIAL.
In the end, I think "dominates" is just not what we want to say here. In the 'for' loop example, there's no sense in which offset 4 dominates "... a ...".

"getOffsets should return the smallest set of offsets at which breakpoints must be set to guarantee a hit after any other source line's interesting side effects and this line's interesting side effects."
(In reply to Jim Blandy :jimb from comment #27)

> Stepping is driven by source locations: we single-step until the source
> location changes, and then we pause. Line entry points aren't relevant to
> stepping. So simply assigning one bytecode the same source line as another
> ensures that single-stepping from the first to the second will not pause.

I belatedly remembered why I was thinking about stepping in this context:
if stepping and breakpoints disagree about stopping points, then users will
see double stops when stepping encounters a breakpoint.

Like, suppose a "goto" is marked as line N, and the user also sets a breakpoint
on line N.  The "goto" won't be considered as an entry point, so the breakpoint
will be set on the "goto"s target.  Stepping will stop on the "goto" (due
to its line number) and then a subsequent step will stop again on the goto's
target, due to the breakpoint -- but this will appear to the user as a stop on
the same line.


I thought at first that maybe ignoring JOF_UNINTERESTING opcodes during stepping
would work.  But "break" and "continue" use JSOP_GOTO and are interesting.
So then it looks like maybe looking at the line notes to see the kind of goto
would work.  But then maybe we're in comment #19 territory again?
That's a really good point. Stepping to a line should definitely land you at the same place where a breakpoint would have been set. Once we reach a new line, it ought to always be safe to continue stepping until one reaches one of the offsets returned by getAllOffsets.

I wasn't thinking that stepping should ignore JOF_UNINTERESTING opcodes. Rather, I was imagining that FlowGraphSummary could skip JOF_UNINTERESTING opcodes in an effort to reduce the set of entry points. If the successor of a JOF_UNINTERESTING opcode was on a different line, then you wouldn't do the slide.
(In reply to Jim Blandy :jimb from comment #19)

> Would it be possible to associate the 'goto' with the line of its target
> instruction? Or would that entail rewriting prior source notes, and be a
> pain? (Don't we rewrite prior source notes already?)

I spent some time on this yesterday.  It turns out that such rewriting
is uglier than I expected.  There are two issues.

First, at various spots we need to make a source note to be filled in later.
However, sometimes we need to do this while some other to-be-filled-in
source note is also pending.  If this "outer" pending source note is filled
in, it may grow, invalidating the first index.

I'm solving this with a couple of lists of source note markers that are
updated as other notes are filled in.


The other issue is that source note users scan the table linearly.  In order
to give one of these instructions the location of its referent, we must
add space in the table for new line and column markers and also reset the state
so that the next emitted note avoids space optimizations (like SRC_NEWLINE).
That is, the line notes table will be somewhat bigger than one might expect.
I couldn't think of a decent way to deal with this, I think we'll just have
to live with it.
I've implemented the bytecode emitter changes and have moved on to
the flow graph changes.

One oddity here is that because EmitCatch can emit an artificial JSOP_IFNE,
the sliding algorithm must handle instructions with multiple successors.
I don't think this will cause real problems, but it does mean that we'll
be seeing somewhat more surprising entry point results.  For example
something like "a = b ? c : d", will have two entry points, not one.

I've also patched script.js to only stop when the current offset is an
entry point for the line.  This seems expensive to me unless we memoize
the entry points somewhere.  I haven't decided where or how to do that yet.
Perhaps they can be stuck on the frame or the script.
Some notes from irc:

<jimb> I don't think JSOP_IFNE can ever be an entry point of a statement.
<jimb> tromey: because statements don't leave values on the stack, unless
       you're entering some binding construct.


<jimb> As you emit bytecode, along with the source notes you're building, you
       also keep a vector of offsets within those source notes that need
       updating, and the values to store in them.
<jimb> You never update the source notes while building.
<jimb> So the offsets in the vector stay correct.  [09:08]
<jimb> And the indices in the vector are stable, so you can tell where to put
       the final values when you find them.
<jimb> indices *of* the vector elements, sorry
<jimb> or *into*
<jimb> anyway
       bytecode offsets in this patch vector are strictly increasing, either
       by construction if possible, or by sorting
<jimb> Then you walk it from end to front, and drop in all the final values in
       one pass.  [09:10]
<jimb> The only reason to consider this is if it'd be simpler overall than
       what you're doing.
<jimb> But it does offer indices that remain stable (into the vector, instead
       of into the source notes) during bytecode generation.
[...]
<jimb> Right; all the SetSrcNoteOffset callers would change to use this new
       patch vector.
(In reply to Tom Tromey :tromey from comment #34)

> One oddity here is that because EmitCatch can emit an artificial JSOP_IFNE,
> the sliding algorithm must handle instructions with multiple successors.

I looked at this again and I convinced myself that we can mark these instructions
with the location of the catch body, and so the sliding shouldn't need to handle
multiple successors after all.

It would be great if somebody else could think about this as well.
I think I have it mostly working (but not polished, I added a helper function
rather than JOF_UNINTERESTING and I have to add or rewrite comments).
I find it hard to be confident in this patch as it is quite complicated to reason about.

I added a hack to FlowGraphSummary to treat gosub and retsub specially
to avoid the extra stop in a finally.  The retsub treatment in particular is a
big hack.

Script-getLineOffsets-03.js shows a problem that I haven't figured out how to solve yet.
It examines the script "while (i < 5) i++;", expecting a single stop.
However, the disassembly is:

00013:  goto 43 (+30)
00018:  loophead
00019:  bindgname "i"
00024:  getgname "i"
00029:  pos
00030:  dup
00031:  one
00032:  add
00033:  pick 2
00035:  swap
00036:  setgname "i"
00041:  pop
00042:  pop
00043:  loopentry 129
00045:  getgname "i"
00050:  int8 5
00052:  lt
00053:  ifne 18 (-35)


The issue here is that the initial "goto" is an uninteresting instruction,
so entry point computation skips it.  Instead it chooses the "loopentry"
instruction.  However, this instruction is executed for each iteration of
the loop.

I am not sure yet but maybe this can be handled by making FlowGraphSummary
smarter somehow.
(In reply to Tom Tromey :tromey from comment #37)

> I am not sure yet but maybe this can be handled by making FlowGraphSummary
> smarter somehow.

So far I could only think of two ways to handle this.

One is the hack of making the sliding dependent on the successor
instruction -- that is, ruling out this case by noticing that the target
of the "goto" is a "loopentry".  This seems like a bad approach to me,
as it makes flow graph computation dependent on particulars of bytecode
generation.

Another approach is to only do sliding if there are multiple possible
entry points for a line.  This seems promising, but I haven't totally
convinced myself it is correct in all cases.
(In reply to Tom Tromey :tromey from comment #37)
> The issue here is that the initial "goto" is an uninteresting instruction,
> so entry point computation skips it.  Instead it chooses the "loopentry"
> instruction.  However, this instruction is executed for each iteration of
> the loop.

Why is this a problem? I would like stepping to stop on the condition each time through the loop; isn't that expected?
(In reply to Jim Blandy :jimb from comment #39)

> Why is this a problem? I would like stepping to stop on the condition each
> time through the loop; isn't that expected?

The comment there says:

// getLineOffsets treats one-line compound statements as having only one entry-point.
// (A breakpoint on a line that only executes once will only hit once.)

So basically, I think it's just because this behavior was intended.

One weird thing about just deleting the test is that all the other tests in
that file pass with the patches in place.  That is, a one-line "while" statement
will stop on every iteration, but a one-line "for" or "do" loop will stop just once.

It seems to me that it would be fine to stop on each iteration.  However if we were
to do that for "while", then I think it would be best to do it for all kinds of loops.

As an aside, as a long term goal it seems like it would be better for the debugger to
be indifferent to whether or not statements are collected onto a single line.  That is,
single-stepping should take me to the next statement, whether it is on the same line or
a different line.  I'm not sure this can be done with the information currently emitted
by the bytecode generator, though.
Attached patch P (obsolete) — Splinter Review
Here's the WIP patch.  Jim said it may be helpful to attach it.
It's unclean in various ways, but it does mostly work.
(In reply to Tom Tromey :tromey from comment #40)
> As an aside, as a long term goal it seems like it would be better for the
> debugger to
> be indifferent to whether or not statements are collected onto a single
> line.  That is,
> single-stepping should take me to the next statement, whether it is on the
> same line or
> a different line.  I'm not sure this can be done with the information
> currently emitted
> by the bytecode generator, though.

Definitely. We want to become more statement-oriented, not line-oriented. That would play better with minimized code.
> Definitely. We want to become more statement-oriented, not line-oriented.
> That would play better with minimized code.

One thought I had was to make FlowGraphSummary only consider a PC as an entry
point if it appears in the line table.  This would automatically eliminate from
consideration any PC corresponding to an "artificial" instruction without requiring
changes to the bytecode emitter -- because a line note is never emitted for such
instructions.

The only wrinkle I can think of offhand here is that to preserve the current
line-based behavior, FlowGraphSummary would need a special case for "for" loops.
That is, to get the per-line entry points, ignoring columns as it does now,
it would have to read the "for" source note to avoid eliminating the update entry
point.
Duplicate of this bug: 1129813
We should make sure to include the test case from bug 1129813 in this bug.
(In reply to Jim Blandy :jimb from comment #45)
> We should make sure to include the test case from bug 1129813 in this bug.

My current patch doesn't affect the behavior of that test at all.
I don't think it is the same problem really -- I am going to undup it and
put some more info there.
Here is "Plan E", as discussed earlier this week.

The basic idea is to limit the set of possible entry points to exactly
those PC offsets which have an explicit mention in the line table.
These correspond to "interesting" places to stop, according to
BytecodeEmitter.

Then, script.js is changed so that stepping will only allow a stop at
one of these entry points.  This eliminates "artificial" instructions
from consideration, since such instructions never have an explicit
line table entry.

A few notes on the patch:

* Script-getAllColumnOffsets-06.js only passed due to the bug.
  It expects a stop on "new Error", but this does not occur because
  print returns undefined.  Instead the stop occurred later, at an
  instruction that inherited this same column number.

* I snuck an obviously missing "else" into
  BytecodeRangeWithPosition::updatePosition.

* It seems nearly possible to remove FlowGraphSummary entirely.
  I need to think about it a bit more; if you have any thoughts on the
  topic, let me know.

* It was unclear to me whether the script.js change should try to
  memoize the result of getLineOffsets; or whether this change should
  be implemented instead inside the vm.
It occurs to me as well that this needs a comment somewhere explaining
the reason for checking the line table.

I was also unsure if it needs a comment in Debugger.Frame.md explaining
the correct way to implement stepping.
(In reply to Tom Tromey :tromey from comment #47)

> * It seems nearly possible to remove FlowGraphSummary entirely.

I don't think this can be done without adding a special hack for "for"
loops somewhere.  Otherwise it is difficult to differentiate between:

   something; something; something;

... which should have a single entry point for the line, and

   for (something; something; something)

... which should have two.
Added the missing comment.
Attachment #8536702 - Attachment is obsolete: true
Attachment #8556676 - Attachment is obsolete: true
Attachment #8562378 - Attachment is obsolete: true
Blocks: 1139235
Blocks: 1013219
Depends on: 1134198
Blocks: 1129813
Duplicate of this bug: 1148547
Note that bug 1147067 will affect the viability of Plan E.
If more source notes are emitted, under Plan E, this will change the
set of possible entry points.
Rebased and updated for intervening changes to the parser.
Attachment #8565512 - Attachment is obsolete: true
> Rebased and updated for intervening changes to the parser.

Of course I meant "bytecode emitter" here.
(In reply to Tom Tromey :tromey from comment #53)
> Note that bug 1147067 will affect the viability of Plan E.
> If more source notes are emitted, under Plan E, this will change the
> set of possible entry points.

After a long discussion with shu, we concluded that there is no problem here.
1147067 may introduce new columns, but should not introduce new line notes.
So, the set of entry points should not change.
Blocks: 1151106
Comment on attachment 8588039 [details] [diff] [review]
make entry points correspond to entries in the line table

I think the whole "Plan E" approach is ready for review now.

This first patch lays the groundwork for all the others.
However, I think you will probably want to examine them as a group.
A couple of the later patches are maybe a bit ugly.
Attachment #8588039 - Flags: review?(jimb)
Comment on attachment 8588039 [details] [diff] [review]
make entry points correspond to entries in the line table

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

Still reviewing; some comments as I go:

::: js/src/jit-test/tests/debug/Frame-onStep-11.js
@@ +2,5 @@
> +// step backward to some earlier statement.
> +
> +var g = newGlobal();
> +g.eval("function f() {\n" +
> +       "  var x = 0;\n" +

Use template strings here; they're much more legible.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings

@@ +31,5 @@
> +    }
> +  };
> +};
> +
> +g.eval("f();\n");

You can just say:

g.f();

@@ +33,5 @@
> +};
> +
> +g.eval("f();\n");
> +
> +assertEq(foundLine > debugLine, true);

We should be able to tighten this test up a great deal.

First, we can use g.evaluate instead of g.eval and pass the 'lineNumber' option, to ensure that our code starts at a fixed line number. Then, we can put the 'debugger' at the top of the function and step through the entire rest of the function's execution, recording each new line number we encounter. Then, we can check that we got the series of line numbers that we expected. If any of those change, we want to know.

There's an arraysEqual utility function defined here:

https://hg.mozilla.org/mozilla-central/file/883e17fc475f/js/src/jit-test/lib/array-compare.js#l16

::: js/src/jit-test/tests/debug/Frame-onStep-12.js
@@ +1,4 @@
> +// The bug we're testing for is that we could see a stop on an
> +// instruction that assumed the line number of whatever was emitted
> +// previously, which to the user could appear to be nonsensical.  This
> +// bit of code arranges for that line number to be something

I had a hard time understanding this comment. I think future SpiderMonkey hackers who regress this test will be pretty lost. If the below is correct, could we use it instead?

Because our script source notes record only those bytecode offsets at which
source positions change, the default behavior in the absence of a source note is
to attribute a bytecode instruction to the same source location as the preceding
instruction. When control flows from the preceding bytecode to the one we're
emitting, that's usually plausible. But successors in the bytecode stream are
not necessarily successors in the control flow graph. If the preceding bytecode
was a back edge of a loop, or the jump at the end of a 'then' clause, its source
position can be completely unrelated to that of its successor.

We try to avoid showing such nonsense offsets to the user by requiring
breakpoints and single-stepping to stop only at a line's entry points, as
reported by Debugger.Script.prototype.getLineOffsets; and then ensuring that
those entry points are all offsets mentioned explicitly in the source notes, and
hence deliberately attributed to the given bytecode.

This bit of JavaScript compiles to bytecode ending in a branch instruction whose
source position is the body of an unreachable loop. The first instruction of the
bytecode we emit following it will inherit this nonsense position, if we have
not explicitly emitted a source note for said instruction.

This test steps across such code and verifies that control never appears to
enter the unreachable loop.

@@ +5,5 @@
> +// nonsensical -- the middle of a loop which cannot be entered.  We
> +// then use this to ensure that our plan of only letting stepping stop
> +// on entry points does not stop on such an instruction.
> +
> +var bitOfCode = ("    debugger;\n" +                    // +0

Use template strings here, and for the code blocks elsewhere in this test.

@@ +25,5 @@
> +  frame.onStep = function() {
> +    let foundLine = this.script.getOffsetLine(this.offset);
> +    if (this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) {
> +      print("foundLine=" + foundLine);
> +      assertEq(foundLine <= debugLine + 1 || foundLine >= debugLine + 6, true);

Paranoia, but, this test will also pass if no debugger trap ever runs. Use the same 'assertEq(log, ...)' idiom the other Debugger tests use, to ensure we're hitting the traps we expect. We can clear and check the log in testOne.
This looks like a great bug for the devedition-40 list.
Whiteboard: [devedition-40]
Priority: -- → P1
(In reply to Jim Blandy :jimb from comment #58)
> Comment on attachment 8588039 [details] [diff] [review]
> make entry points correspond to entries in the line table
> 
> Review of attachment 8588039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still reviewing; some comments as I go:
> 
> ::: js/src/jit-test/tests/debug/Frame-onStep-11.js
> @@ +2,5 @@
> > +// step backward to some earlier statement.
> > +
> > +var g = newGlobal();
> > +g.eval("function f() {\n" +
> > +       "  var x = 0;\n" +
> 
> Use template strings here; they're much more legible.
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> template_strings
> 
> @@ +31,5 @@
> > +    }
> > +  };
> > +};
> > +
> > +g.eval("f();\n");
> 
> You can just say:
> 
> g.f();
> 
> @@ +33,5 @@
> > +};
> > +
> > +g.eval("f();\n");
> > +
> > +assertEq(foundLine > debugLine, true);
> 
> We should be able to tighten this test up a great deal.
> 
> First, we can use g.evaluate instead of g.eval and pass the 'lineNumber'
> option, to ensure that our code starts at a fixed line number. Then, we can
> put the 'debugger' at the top of the function and step through the entire
> rest of the function's execution, recording each new line number we
> encounter. Then, we can check that we got the series of line numbers that we
> expected. If any of those change, we want to know.
> 
> There's an arraysEqual utility function defined here:
> 
> https://hg.mozilla.org/mozilla-central/file/883e17fc475f/js/src/jit-test/lib/
> array-compare.js#l16
> 
> ::: js/src/jit-test/tests/debug/Frame-onStep-12.js
> @@ +1,4 @@
> > +// The bug we're testing for is that we could see a stop on an
> > +// instruction that assumed the line number of whatever was emitted
> > +// previously, which to the user could appear to be nonsensical.  This
> > +// bit of code arranges for that line number to be something
> 
> I had a hard time understanding this comment. I think future SpiderMonkey
> hackers who regress this test will be pretty lost. If the below is correct,
> could we use it instead?
> 
> Because our script source notes record only those bytecode offsets at which
> source positions change, the default behavior in the absence of a source
> note is
> to attribute a bytecode instruction to the same source location as the
> preceding
> instruction. When control flows from the preceding bytecode to the one we're
> emitting, that's usually plausible. But successors in the bytecode stream are
> not necessarily successors in the control flow graph. If the preceding
> bytecode
> was a back edge of a loop, or the jump at the end of a 'then' clause, its
> source
> position can be completely unrelated to that of its successor.
> 
> We try to avoid showing such nonsense offsets to the user by requiring
> breakpoints and single-stepping to stop only at a line's entry points, as
> reported by Debugger.Script.prototype.getLineOffsets; and then ensuring that
> those entry points are all offsets mentioned explicitly in the source notes,
> and
> hence deliberately attributed to the given bytecode.
> 
> This bit of JavaScript compiles to bytecode ending in a branch instruction
> whose
> source position is the body of an unreachable loop. The first instruction of
> the
> bytecode we emit following it will inherit this nonsense position, if we have
> not explicitly emitted a source note for said instruction.
> 
> This test steps across such code and verifies that control never appears to
> enter the unreachable loop.
> 
> @@ +5,5 @@
> > +// nonsensical -- the middle of a loop which cannot be entered.  We
> > +// then use this to ensure that our plan of only letting stepping stop
> > +// on entry points does not stop on such an instruction.
> > +
> > +var bitOfCode = ("    debugger;\n" +                    // +0
> 
> Use template strings here, and for the code blocks elsewhere in this test.
> 
> @@ +25,5 @@
> > +  frame.onStep = function() {
> > +    let foundLine = this.script.getOffsetLine(this.offset);
> > +    if (this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) {
> > +      print("foundLine=" + foundLine);
> > +      assertEq(foundLine <= debugLine + 1 || foundLine >= debugLine + 6, true);
> 
> Paranoia, but, this test will also pass if no debugger trap ever runs. Use
> the same 'assertEq(log, ...)' idiom the other Debugger tests use, to ensure
> we're hitting the traps we expect. We can clear and check the log in testOne.

Jim, this patch has been sitting in your review queue for 2 months. I know how busy you are, of course, but this is a high priority bug for the debugger. Do you think you could get to it some time soon? Thanks for looking into it!
Flags: needinfo?(jimb)
Whiteboard: [devedition-40] → [polish-backlog]
Rebased and updated for review (I think - I haven't looked at it
closely recently).
Attachment #8588039 - Attachment is obsolete: true
Attachment #8588039 - Flags: review?(jimb)
I think I didn't finish updating the test case here.
Comment on attachment 8668025 [details] [diff] [review]
make entry points correspond to entries in the line table

I made some of the requested changes but I think didn't update one of the test cases yet.
Attachment #8668025 - Flags: review?(jimb)
Comment on attachment 8668025 [details] [diff] [review]
make entry points correspond to entries in the line table

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

::: js/src/jit-test/tests/debug/Frame-onStep-12.js
@@ +36,5 @@
> +var dbg = Debugger(g);
> +
> +g.eval("function nothing() { }\n");
> +
> +var badStep = false;

This variable doesn't seem to be used, just set twice.

@@ +44,5 @@
> +  frame.onStep = function() {
> +    let foundLine = this.script.getOffsetLine(this.offset);
> +    if (this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) {
> +      print("foundLine=" + foundLine);
> +      assertEq(foundLine <= debugLine + 1 || foundLine >= debugLine + 6, true);

As you noted, this test still needs revision.

::: js/src/vm/Debugger.cpp
@@ +5118,5 @@
>      if (!result)
>          return false;
>      for (BytecodeRangeWithPosition r(cx, script); !r.empty(); r.popFront()) {
> +        if (!r.frontIsEntryPoint())
> +            continue;

If I'm reading right, what this patch does is narrow the results to the *intersection* of those sites that both the FlowGraphSummary and the source notes consider interesting. But do we still want to consider the flow graph data at all? What would go wrong if we just disposed of the FlowGraphSummary altogether, and return for each line the array of offsets at which that line becomes current?

@@ +5189,5 @@
>      if (!result)
>          return false;
>      for (BytecodeRangeWithPosition r(cx, script); !r.empty(); r.popFront()) {
> +        if (!r.frontIsEntryPoint())
> +            continue;

Similarly.
Attachment #8668025 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #64)

> If I'm reading right, what this patch does is narrow the results to the
> *intersection* of those sites that both the FlowGraphSummary and the source
> notes consider interesting. But do we still want to consider the flow graph
> data at all? What would go wrong if we just disposed of the FlowGraphSummary
> altogether, and return for each line the array of offsets at which that line
> becomes current?

See comment #49.
But I am not sure that the conclusion I reached there is correct.
I will need to research it.
(In reply to Tom Tromey :tromey from comment #65)
> (In reply to Jim Blandy :jimb from comment #64)
> 
> > If I'm reading right, what this patch does is narrow the results to the
> > *intersection* of those sites that both the FlowGraphSummary and the source
> > notes consider interesting. But do we still want to consider the flow graph
> > data at all? What would go wrong if we just disposed of the FlowGraphSummary
> > altogether, and return for each line the array of offsets at which that line
> > becomes current?
> 
> See comment #49.
> But I am not sure that the conclusion I reached there is correct.
> I will need to research it.

I hacked this up quickly and ran the test suite to see what would happen.
I think the analysis in comment #49 is incorrect, but the quick hack
yields different results from the current code, so further front end changes
are needed.
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=medium]
Ok, I think I understand this a bit better now.  The reason flow graph summary
is still needed is that we rely on it to deal with column numbers sensibly.


My hack was to add a flag to BytecodeRangeWithPosition to make it possible
to filter on just line number notes -- that is, exclude column number notes.
Then, I removed the use of FlowGraphSummary from DebuggerScript_getAllOffsets
and DebuggerScript_getLineOffsets.

The rationale for the first change was that we don't want to get offsets
at any spot that there is a column-incrementing note.  That would yield way
too many offsets.

However, it turns out we sometimes need these, and this is what the flow graph
summary provides.  Consider this test in debug/Script-getLineOffsets-05.js:

test("if (i === 2)\n" +
     "    log += 'X'; else log += '!';\n");

Here, the test relies on there being two entry points to the line that updates "log".
In particular there is one after the "else" -- but this was rejected by
the BytecodeRangeWithPosition change.

Now, suppose we leave BytecodeRangeWithPosition unchanged.  Then a line
with multiple statements like "f(); g();" will report two stopping offsets.
However, that is not what we want from this API.  (Whether or not this
is the behavior we want from the debugger, I don't know; see bug 1145747
and bug 1145752).

So, on this basis I'm going to go ahead with the patch as is.
Update the test case per review.
Attachment #8668025 - Attachment is obsolete: true
Attachment #8669079 - Flags: review+
Add r= to commit message.
Attachment #8669079 - Attachment is obsolete: true
Attachment #8669083 - Flags: review+
Rebased.  Also, slightly modified the new tests to pass -- the changes
made here are undone in a later patch in the series.
Attachment #8669083 - Attachment is obsolete: true
Attachment #8669718 - Flags: review+
Flags: needinfo?(jimb)
Updated to fix devtools/server unit tests.
Attachment #8669718 - Attachment is obsolete: true
Update comment
Attachment #8669970 - Attachment is obsolete: true
Comment on attachment 8669972 [details] [diff] [review]
make entry points correspond to entries in the line table

This adds a "same frame" check to script.js.  See the comment for an 
explanation.  Without this check some otherwise reasonable unit tests
in devtools/server fail because stepping winds up at unexpected spots.
This is also related to bug 1172572.
Attachment #8669972 - Flags: review?(nfitzgerald)
Comment on attachment 8669972 [details] [diff] [review]
make entry points correspond to entries in the line table

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

Frame-onStep-12 is very thorough! Great stuff, thank you!

Just a few nits below.

::: devtools/server/actors/script.js
@@ +820,5 @@
> +      // and into subsequent calls in the outer frame.  E.g., if there
> +      // is a call "a(b())" and the user steps into b, then this
> +      // condition makes it possible to step out of b and into a.
> +      if (this === startFrame &&
> +          this.script.getLineOffsets(this.script.getOffsetLine(this.offset)).indexOf(this.offset) < 0) {

This line is a bit of a mouthful and is kind of difficult for me to reason about because the same few words are repeated so much. Can we split this off into a simple helper function with a name that makes it more clear what it does semantically? `offsetIsLineEntryPoint(script, offset)` and then `... && !offsetIsLineEntryPoint(...)` in this conditional?

::: js/src/jit-test/tests/debug/Frame-onStep-11.js
@@ +13,5 @@
> +         } finally {            // +7
> +           x = 3;               // +8
> +         }                      // +9
> +         x = 4;                 // +10
> +       }`);                     // +11

FYI, you can use `g.evaluate("...", { lineNumber: 0 })` to specify a start line, so that you don't have to do the line delta arithmetic. You've already written the tests this way, so I leave it up to you to decide if it is worth it to rewrite.

https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp#4528

@@ +17,5 @@
> +       }`);                     // +11
> +
> +var dbg = Debugger(g);
> +
> +let foundLines = '';

Also, I know the debugger's jit-tests really love concatenating esoteric strings, but this could just be an array of line numbers. Again, I leave it up to you, since this doesn't really matter much.

::: js/src/jit-test/tests/debug/Frame-onStep-12.js
@@ +79,5 @@
> +// Test the instructions at the end of a "catch".
> +testOne("testCatchFinally",
> +        `try {
> +           throw new TypeError();
> +         } catch (e if e instanceof TypeError) {

Conditional catches are a spidermonkey-ism, not part of any standard, and therefore are deprecated and slated for removal sooner or later.

Can this test have only one catch which runs the bitOfCode? Is there a reason we need the conditional catch and the empty catchall?

::: js/src/vm/Debugger.cpp
@@ +4898,2 @@
>    private:
> +    bool updatePosition() {

Add a note about what true/false return means.

Easy to make the mistake of assuming that it means success/failure if you don't read too closely.

Maybe this should just mutate isEntryPoint directly, rather than return the value so that popFront can update isEntryPoint? That would make sense to me.
Attachment #8669972 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #76)

> > +          this.script.getLineOffsets(this.script.getOffsetLine(this.offset)).indexOf(this.offset) < 0) {
> 
> This line is a bit of a mouthful and is kind of difficult for me to reason
> about because the same few words are repeated so much. Can we split this off
> into a simple helper function with a name that makes it more clear what it
> does semantically? `offsetIsLineEntryPoint(script, offset)` and then `... &&
> !offsetIsLineEntryPoint(...)` in this conditional?

Yeah, I'll do that.
On irc Jim suggested a follow-up bug to collapse this idiom into a single
method on Debugger.Script; I'll file one.

> Also, I know the debugger's jit-tests really love concatenating esoteric
> strings, but this could just be an array of line numbers. Again, I leave it
> up to you, since this doesn't really matter much.

This was explicitly requested in an earlier review, see comment #58.

> Conditional catches are a spidermonkey-ism, not part of any standard, and
> therefore are deprecated and slated for removal sooner or later.
> 
> Can this test have only one catch which runs the bitOfCode? Is there a
> reason we need the conditional catch and the empty catchall?

This was just to be thorough when testing the emitter changes.
I think modifying it is fine though.
Updated per review.
I left one instance of the SpiderMonkey catch extension, as without it
the particular test doesn't make sense; when the extension is deleted,
the test can be as well.  I added a comment to explain this.
Attachment #8669972 - Attachment is obsolete: true
Attachment #8670301 - Flags: review+
This doesn't work correctly with source-mapped sources; which manifests
in one of the debugger pretty-print test cases.  The issue is that the new
test in script.js uses getLineOffsets, but in the source-mapped case we
need to look at column offsets.

I'm still investigating a fix.  Maybe bug 863089 would help, but I am not
sure yet.
I think bug 863089 is the way to go.  It will let me replace the script.js
change with a simple call to getOffsetLocation -- I plan to add an "is an entry
point" property to the returned object.  This may also suffice to close
bug 1211906 as well.
Depends on: 863089
Add isEntryPoint and use that instead in script.js.

Now we also don't need FlowGraphSummary in getAllColumnOffsets, either.
Attachment #8670301 - Attachment is obsolete: true
Attachment #8675843 - Flags: review?(nfitzgerald)
Comment on attachment 8675843 [details] [diff] [review]
make entry points correspond to entries in the line table

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

\o/

::: devtools/server/actors/script.js
@@ +821,5 @@
> +      // is a call "a(b())" and the user steps into b, then this
> +      // condition makes it possible to step out of b and into a.
> +      if (this === startFrame &&
> +          !this.script.getOffsetLocation(this.offset).isEntryPoint) {
> +        return undefined;

What is an entry point in "normal" (aka hand written) JS is not necessarily an entry point in minified-but-now-pretty-printed JS or more generally any source mapped JS.

I think it makes sense to move this check after the source mapping related things below, or check this._options.useSourceMaps here.
Attachment #8675843 - Flags: review?(nfitzgerald) → review+
(In reply to Tom Tromey :tromey from comment #82)

> Now we also don't need FlowGraphSummary in getAllColumnOffsets, either.

This turned out to be mistaken.

One case I found where this is still needed is when there is a "return"
instruction and then a following "retrval".  In this case, the retrval is
dead -- and the flow graph is what excludes it from consideration as an
entry point.

There may be other cases, I am not sure.  This particular one could
be fixed by changing the bytecode emitter not to emit a dead retrval.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #83)
> Comment on attachment 8675843 [details] [diff] [review]
> make entry points correspond to entries in the line table
> 
> Review of attachment 8675843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> \o/
> 
> ::: devtools/server/actors/script.js
> @@ +821,5 @@
> > +      // is a call "a(b())" and the user steps into b, then this
> > +      // condition makes it possible to step out of b and into a.
> > +      if (this === startFrame &&
> > +          !this.script.getOffsetLocation(this.offset).isEntryPoint) {
> > +        return undefined;
> 
> What is an entry point in "normal" (aka hand written) JS is not necessarily
> an entry point in minified-but-now-pretty-printed JS or more generally any
> source mapped JS.
> 
> I think it makes sense to move this check after the source mapping related
> things below, or check this._options.useSourceMaps here.

I think source mapping can't have an impact here.  That is, there is no way for
source mapping to introduce a new potential stopping point where isEntryPoint
is false.

The way I think about it is that the only candidate offsets for a stopping
point are those PCs that have an explicit entry in the line table.  All other
PCs just inherit their location from the most recently seen line note.

Since source mapping works solely with source locations and not the PCs, I
think it is safe to always do this check early.

I'm not sure if this explanation is very clear.

I will add a bigger comment here explaining the situation.
Comment on attachment 8677548 [details] [diff] [review]
make entry points correspond to entries in the line table

A few updates here.

Testing a later patch in the series revealed that (ugh!) I got the condition
for isEntryPoint completely backwards.  I fixed this and updated the test
case included in this patch to be sure to catch the problem.

I was also mistaken when I thought we could remove some uses of FlowGraphSummary.
See comment #84.  This could be the subject of a future optimization.

I left the new condition in script.js where it is, see comment #85.
Attachment #8677548 - Flags: review?(nfitzgerald)
(In reply to Tom Tromey :tromey from comment #85)
> I think source mapping can't have an impact here.  That is, there is no way for
> source mapping to introduce a new potential stopping point where isEntryPoint
> is false.

Maybe I am misunderstanding, in which case please help me understand :)

Here is my current understanding: we receive minified code like this:

  <stmt1>; <stmt2>; <stmt3>;

and we prettify and source-map it into this:

  <stmt1>;
  <stmt2>;
  <stmt3>;

Now, (assuming none of these statements are loops or function decls) the only bytecodes that would be considered an entry point to the non source mapped line are associated with stmt1.

When we step in the minified/non-source-mapped code, we will just continue to the entry point of the next line and continue past stmt2 and stmt3. However, when stepping through the source-mapped statements we do wish to stop on each statement and line. Those *are* "new potential stopping points"!

Just realized something: I believe we do the "continue-till-on-a-new-line" in the actor, not the Debugger API level. Perhaps you were only talking about the Debugger API level? In which case, I think the scenario I just explained is nullified?

> The way I think about it is that the only candidate offsets for a stopping
> point are those PCs that have an explicit entry in the line table.  All other
> PCs just inherit their location from the most recently seen line note.
> 
> Since source mapping works solely with source locations and not the PCs, I
> think it is safe to always do this check early.

I think part of the problem is that the PCs that get an explicit entry in the source notes are affected by whitespace :(
Comment on attachment 8677548 [details] [diff] [review]
make entry points correspond to entries in the line table

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

Regardless of the presence of further complications, I think we should still land this patch now.
Attachment #8677548 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #88)

> Maybe I am misunderstanding, in which case please help me understand :)
> 
> Here is my current understanding: we receive minified code like this:
> 
>   <stmt1>; <stmt2>; <stmt3>;
> 
> and we prettify and source-map it into this:
> 
>   <stmt1>;
>   <stmt2>;
>   <stmt3>;
> 
> Now, (assuming none of these statements are loops or function decls) the
> only bytecodes that would be considered an entry point to the non source
> mapped line are associated with stmt1.

I think this should still be ok, because the script.js change is considering
"column" entry points (just like D.S.getAllColumnOffsets).  So, I think the
first instruction of each <stmt*> will be considered an entry point in this sense,
and so the early return will not trigger.

For more reassurance, see comment #80.  The earlier line-based approach was regressing
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-08.js
I think this is basically the case described above.


It's true that some logical mappings can't be expressed.  However, this is a more
fundamental problem.  For example suppose you had a bit of lisp like:

  (setf value (+ x y))

Here the translation might be:

  value = x + y;

Now, in Lisp it's perfectly sensible to step into the (+) form.
However, this translation doesn't admit the possibility, because the "+"
isn't considered an entry point -- all the instructions making
up this expression share one entry in the line table.

That is, the debugger could single-step through each instruction, but since
they all share a source location, they would necessarily all map to the same
thing via source maps; and so no extra stop could occur.

A translator can give guidance by emitting separate statements:

  let tmp99237 = x + y;
  value = tmp99237;

This is unwarranted chumminess with the bytecode emitter; but something
that could maybe be addressed by the source map spec.

Another approach would be to have much fuller debug information, like
emitting a location for nearly every operation; then the source map could
reflect whatever translations are desired.

Future work here should probably also take bug 1145747 and bug 1145752 into
consideration.  For those it would be good to have a way to find the end
of the current "statement" so that we could highlight it in the debugger
when "column stepping".
(In reply to Tom Tromey :tromey from comment #90)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #88)
> 
> > Maybe I am misunderstanding, in which case please help me understand :)
> > 
> > Here is my current understanding: we receive minified code like this:
> > 
> >   <stmt1>; <stmt2>; <stmt3>;
> > 
> > and we prettify and source-map it into this:
> > 
> >   <stmt1>;
> >   <stmt2>;
> >   <stmt3>;
> > 
> > Now, (assuming none of these statements are loops or function decls) the
> > only bytecodes that would be considered an entry point to the non source
> > mapped line are associated with stmt1.
> 
> I think this should still be ok, because the script.js change is considering
> "column" entry points (just like D.S.getAllColumnOffsets).  So, I think the
> first instruction of each <stmt*> will be considered an entry point in this
> sense,
> and so the early return will not trigger.
> 
> For more reassurance, see comment #80.  The earlier line-based approach was
> regressing
> devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-08.js
> I think this is basically the case described above.

Ok, that clears everything up for me, thank you.

> Another approach would be to have much fuller debug information, like
> emitting a location for nearly every operation; then the source map could
> reflect whatever translations are desired.

IIRC, the reason we don't keep full location info is a memory optimization. Shu has talked about a mode to retain it all for better locations when profiling, we may be able to piggy back on that if it pans out (or we could decide to make it pan out ourselves, of course).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/093802a6d8ae
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
https://hg.mozilla.org/integration/mozilla-inbound/rev/c11218a05463e65878b122132344d04be680cfef
Fix a broken JS test. It landed in rev 093802a6d8ae (bug 1003554) and was apparently fine until it was merged to m-c/m-i, where it probably collided with rev bug 1217099 or bug 1217001. no_r=bustage to a CLOSED TREE.
Duplicate of this bug: 1134187
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.