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

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
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.
Flags: needinfo?(ejpbruel)

Comment 3

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

5 years ago
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.
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.
Flags: needinfo?(ejpbruel)
Posted patch more-offsets.patch (obsolete) — Splinter Review
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
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Attachment #8409959 - Flags: review?(ejpbruel)
Looks like I need to update some existing assertions in the jit tests.
Posted patch more-offsets.patch (obsolete) — Splinter Review
Now without printfs and with passing jit tests!

https://tbpl.mozilla.org/?tree=Try&rev=978879208669
Attachment #8409959 - Attachment is obsolete: true
Attachment #8409959 - Flags: review?(ejpbruel)
Attachment #8410330 - Flags: review?(ejpbruel)
Comment on attachment 8410330 [details] [diff] [review]
more-offsets.patch

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

Patch looks good to me
Attachment #8410330 - Flags: review?(ejpbruel) → review+
This should fix those errors.

https://tbpl.mozilla.org/?tree=Try&rev=6f13f50624f5
Attachment #8410330 - Attachment is obsolete: true
Comment on attachment 8411923 [details] [diff] [review]
more-offsets.patch

Carrying over ejpbruel's previous r+.
Attachment #8411923 - Flags: review+
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.
https://hg.mozilla.org/mozilla-central/rev/6d4e3460c1f8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Duplicate of this bug: 987536
You need to log in before you can comment on or make changes to this bug.