Closed Bug 984696 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 2 obsolete files)

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)
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.
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)
Attached 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.
Attached 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: 7 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.