1296441 - debugger steps entirely over an "if"

Status: RESOLVED DUPLICATE of bug 1263355

(DevTools :: Debugger, defect, P2)

Description

[Tom Tromey :tromey]

Attachment: step-bug.html

Consider the attached file.

Set a breakpoint on the first line of "C".
Then in the console invoke "bug()".

Now single step (step in or step over, doesn't matter).
When stepping through the loop, the debugger never stops on the "if".
However, it ought to.

Comment 1

[Eddy Bruel [:ejpbruel]]

(In reply to Tom Tromey :tromey from comment #0)
> Created attachment 8782649 [details]
> step-bug.html
> 
> Consider the attached file.
> 
> Set a breakpoint on the first line of "C".
> Then in the console invoke "bug()".
> 
> Now single step (step in or step over, doesn't matter).
> When stepping through the loop, the debugger never stops on the "if".
> However, it ought to.

Any idea why this happens Tom? My guess would be a bug in how we compute the entry points for the given line, but perhaps I'm wrong.

Comment 2

[Tom Tromey :tromey]

Attachment: step.js

If you run this test case using 'js' (you need a debug build for "dis"),
you'll see from the dump at the end that line 7 (the "if") doesn't have
an entry point:

4 => 14
5 => 15
6 => 26,130
8 => 95
10 => 113
12 => 162
13 => 167


So probably a bug in the entry point computation in SpiderMonkey.

Comment 3

[Tom Tromey :tromey]

I'll append the disassembly.

The bug happens because when we see the loophead instruction at offset 64,
we enter this: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#5811
However, this computes lineno=-1, oops.  I think this confuses later processing,
specifically for offset=66, which is the "if" in question.


flags: NEEDS_CALLOBJECT CONSTRUCTOR
loc     op
-----   --
00000:  arguments
00001:  setaliasedvar "arguments" (hops = 0, slot = 3)
00006:  pop
00007:  functionthis
00008:  setaliasedvar ".this" (hops = 0, slot = 4)
00013:  pop
main:
00014:  debugger
00015:  getgname "val"
00020:  initaliasedlexical "parent" (hops = 0, slot = 5)
00025:  pop
00026:  pushblockscope depth 0 {name: 0}
00031:  uninitialized
00032:  initaliasedlexical "name" (hops = 0, slot = 2)
00037:  pop
00038:  undefined
00039:  initaliasedlexical "name" (hops = 0, slot = 2)
00044:  pop
00045:  getaliasedvar "nl" (hops = 1, slot = 2)
00050:  dup
00051:  symbol 1
00053:  callelem
00054:  swap
00055:  calliter 0
00058:  undefined
00059:  goto 130 (+71)
00064:  loophead
00065:  dup
00066:  getprop "value"
00071:  setaliasedvar "name" (hops = 0, slot = 2)
00076:  pop
00077:  getaliasedvar "parent" (hops = 1, slot = 5)
00082:  getaliasedvar "name" (hops = 0, slot = 2)
00087:  getelem
00088:  not
00089:  ifeq 112 (+23)
00094:  jumptarget
00095:  getaliasedvar "parent" (hops = 1, slot = 5)
00100:  getaliasedvar "name" (hops = 0, slot = 2)
00105:  string "done"
00110:  setelem
00111:  pop
00112:  jumptarget
00113:  getaliasedvar "parent" (hops = 1, slot = 5)
00118:  getaliasedvar "name" (hops = 0, slot = 2)
00123:  getelem
00124:  setaliasedvar "parent" (hops = 1, slot = 5)
00129:  pop
00130:  loopentry 129
00132:  pop
00133:  dup
00134:  dup
00135:  callprop "next"
00140:  swap
00141:  call 0
00144:  checkisobj 0
00146:  dup
00147:  getprop "done"
00152:  ifeq 64 (-88)
00157:  jumptarget
00158:  popn 2
00161:  popblockscope
00162:  getaliasedvar "parent" (hops = 0, slot = 5)
00167:  return
00168:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    3     7 [   7] xdelta  
  1:    3    14 [   7] newline 
  2:    4    14 [   0] colspan 2
  4:    4    15 [   1] newline 
  5:    5    15 [   0] colspan 6
  7:    5    26 [  11] xdelta  
  8:    5    26 [   0] newline 
  9:    6    38 [  12] xdelta  
 10:    6    38 [   0] colspan 11
 12:    6    59 [  21] xdelta  
 13:    6    59 [   0] for-of   closingjump 93
 15:    6    77 [  18] xdelta  
 16:    6    77 [   0] newline 
 17:    7    77 [   0] colspan 8
 19:    7    89 [  12] xdelta  
 20:    7    89 [   0] if      
 21:    7    95 [   6] newline 
 22:    8    95 [   0] colspan 6
 24:    8   113 [  18] xdelta  
 25:    8   113 [   0] setline  lineno 10
 27:   10   113 [   0] colspan 4
 29:   10   130 [  17] xdelta  
 30:   10   130 [   0] setline  lineno 6
 32:    6   130 [   0] colspan 19
 34:    6   141 [  11] xdelta  
 35:    6   141 [   0] colspan -17
 40:    6   162 [  21] xdelta  
 41:    6   162 [   0] setline  lineno 12
 43:   12   162 [   0] colspan 2
 45:   12   167 [   5] newline 

Exception table:
kind      stack    start      end
 for-of       2       64      157

Block table:
   index   parent    start      end
       0   (none)       31      162

Comment 4

[Tom Tromey :tromey]

This patch fixes the problem for me.
I can write a test case, etc, but I wanted to ask :nbp about it first,
since blame says he wrote the lines in question.

diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp
index f5985d1..a3a5f47 100644
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -5791,7 +5791,7 @@ class FlowGraphSummary {
             if (FlowsIntoNext(prevOp))
                 addEdge(prevLineno, prevColumn, r.frontOffset());
 
-            if (BytecodeIsJumpTarget(op)) {
+            if (BytecodeIsJumpTarget(op) && !entries_[r.frontOffset()].hasNoEdges()) {
                 lineno = entries_[r.frontOffset()].lineno();
                 column = entries_[r.frontOffset()].column();
             }

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Tom Tromey :tromey from comment #4)
> This patch fixes the problem for me.
> I can write a test case, etc, but I wanted to ask :nbp about it first,
> since blame says he wrote the lines in question.
> 
> diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp
> index f5985d1..a3a5f47 100644
> --- a/js/src/vm/Debugger.cpp
> +++ b/js/src/vm/Debugger.cpp
> @@ -5791,7 +5791,7 @@ class FlowGraphSummary {
>              if (FlowsIntoNext(prevOp))
>                  addEdge(prevLineno, prevColumn, r.frontOffset());
>  
> -            if (BytecodeIsJumpTarget(op)) {
> +            if (BytecodeIsJumpTarget(op) &&
> !entries_[r.frontOffset()].hasNoEdges()) {
>                  lineno = entries_[r.frontOffset()].lineno();
>                  column = entries_[r.frontOffset()].column();
>              }

Sorry for the delay.
This patch looks good to me. (= r+)

I forgot the case where the loop-head is the first instruction, and the loop entry is later in the bytecode, as this is here.  Thus we have no yet registered any incoming edges for the JSOP_LOOPHEAD, but we should record that later.

Adding this condition will continue the linear iteration over the bytecode taking the lineno & column from the predecessor, even if there is a discontinuity.

I would have preferred to assert that we have incoming edges, but this would not work unless we switch to a different system such as using IonBuilder control flow graph creation.

Comment 6

[Tom Tromey :tromey]

It turns out this was fixed by bug 1263355.