984696 - [jsdbg2] Debugger.Script needs to be able to give offsets at the expression/statment granularity rather than only line-entry-point granularity

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

When source mapping, a user line is not the same as a line that spidermonkey sees and so often all code is on a single line (minified code) and so only a couple offsets are exposed by the API, but we really want to set a breakpoint on an arbitrary expression inside the line (whatever is source mapped to).

Example:

Minified code:

function foo(){var x=3,y=4,z=9;}

Prettified and source mapped:

function foo() {
  var x = 3,
  y = 4,
  z = 9;
}

We should be able to get an offset for |y = 4| if a user sets a breakpoint on that line, but the Debugger API won't give us one.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Not sure exactly how related this is, but here is |dis| of the ugly and pretty versions:

js> function foo(){var x=3,y=4,z=9;}
function foo(){var x=3,y=4,z=9;}
js> dis(foo)
dis(foo)
flags:
loc     op
-----   --
main:
00000:  int8 3
00002:  setlocal 0
00006:  pop
00007:  int8 4
00009:  setlocal 1
00013:  pop
00014:  int8 9
00016:  setlocal 2
00020:  pop
00021:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    3    21 [  21] xdelta  
  1:    3    21 [   0] colspan 28

js> function bar() {
function bar() {
  var x = 3,
  var x = 3,
  y = 4,
  y = 4,
  z = 9;
  z = 9;
}
}
js> dis(bar)
dis(bar)
flags:
loc     op
-----   --
main:
00000:  int8 3
00002:  setlocal 0
00006:  pop
00007:  int8 4
00009:  setlocal 1
00013:  pop
00014:  int8 9
00016:  setlocal 2
00020:  pop
00021:  retrval

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    5     0 [   0] newline 
  1:    6     7 [   7] newline 
  2:    7    14 [   7] newline 
  3:    8    21 [   7] colspan 3


My understanding is that this reflects what the Debugger API is giving us as well.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I was hoping this would be a quick fix in |DebuggerScript_getAllColumnOffsets| where I could just optionally loosen the offset filtering. However, it appears that for 

    function foo(){var x=3,y=4,z=9;}

every offset's position is reported as line 1, column 0 so that won't work.

The location information comes from the source notes via |BytecodeRangeWithPosition| so it seems that the source notes need to have finer granularity.

Eddy, how difficult is this likely? Pointers/tips for implementation? Thanks.

Comment 3

[Jim Blandy :jimb]

The parse tree has source position information on every node. It's js/src/frontend/BytecodeEmitter.cpp that builds the source codes as it builds the byte code stream. See the calls to, for example, NewSrcNote.

Comment 4

[Jim Blandy :jimb]

Err, "builds the source codes" should be "builds the source notes".

I think "expression granularity" is probably not quite right; something like x = y + z contains *five* expressions. You probably mean largest-containing-expression. But then that's almost exactly the same as "statement": you just need an exception for 'for (A;B;C) D' , saying that each of A, B, C, and D should be distinguished.

This will have some memory impact.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

There seem to be 2 types of expressions that need better source notes ATM:

1. multiple variable declaration+initialization with the same var
2. multiple expressions separated with commas (something minifiers often do)

Trying to find other cases we need to handle as well.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: more-offsets.patch

This patch adds more source notes for:

* Each declared variable in a single var statement.
* Each sub-expression in a comma joined expression (eg "e(),f(),g(),h()").
* Each property of array and object literals.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=871d705d203c

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looks like I need to update some existing assertions in the jit tests.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: more-offsets.patch

Now without printfs and with passing jit tests!

https://tbpl.mozilla.org/?tree=Try&rev=978879208669

Comment 9

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8410330 [details] [diff] [review]
more-offsets.patch

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

Patch looks good to me

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51dbd71f00a5

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out since it might have caused https://tbpl.mozilla.org/php/getParsedLog.php?id=38385566&tree=Mozilla-Inbound

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: more-offsets.patch

This should fix those errors.

https://tbpl.mozilla.org/?tree=Try&rev=6f13f50624f5

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8411923 [details] [diff] [review]
more-offsets.patch

Carrying over ejpbruel's previous r+.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push isn't complete, but it looks pretty good. The failures before weren't intermittent or anything, and we have all the mochitests that have completed thus far (about 2/3 at a glance) passing.

I think this is ready to land again.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4e3460c1f8

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d4e3460c1f8