757188 - [jsdbg2] Debugger should distinguish source positions by column as well as line

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Right now, Debugger.Script only resolves a <script, bytecode offset> pair to a source line within a url, not to the column within that line. The same is true for the reverse mapping: one cannot find the bytecode offset corresponding to a particular column within a line, only the offset corresponding to the beginning of a line. Functions like Debugger.Script.prototype.getAllOffsets return line-oriented output.

These limitations make it impossible to effectively debug minimized JS, but also limit the debugging of non-minimized JS. For example, in the code:

g = function f() { ... }

it's perfectly reasonable to want to set a breakpoint on either the assignment to g, or upon entry to f. These could be distinguished by column number, but not by line alone.

Bug 568142, "Expand source notes to carry column information", is a prerequisite for this.

Once that's done, we need to design and implement a Debugger-level API for reporting and querying column-level positions.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor BytecodeRangeWithLineNumbers and FlowGraphSummary

I've created a patch that refactors BytecodeRangeWithLineNumbers and FlowGraphSummary to contain column information.

Please take a close look at this patch to see if it makes sense, particulary how BytecodeRangeWithLineNumbers computes the current column and how FlowGraphSummary computes edges.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 712875 [details] [diff] [review]
Refactor BytecodeRangeWithLineNumbers and FlowGraphSummary

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

Stealing review. Nice work! Bring on part 2.

::: js/src/vm/Debugger.cpp
@@ +2993,5 @@
> +    class Entry {
> +      public:
> +        static Entry createWithNoEdges() {
> +            return Entry(-1, 0);
> +        };

Style nit: No extra semicolons after functions, please. :)

@@ +3060,2 @@
>              return false;
>          FlowGraphSummary &self = *this;

You could get rid of self here and just use entries_[i] rather than self[i] below. (I'm not sure what possessed me to make this a Vector subclass in the first place.)

Comment 3

[Cosmin Radoi]

Attachment: getLineOffsets not returns a sparse array, i.e. object, from column to bytecode offset

Comment 4

[Cosmin Radoi]

*now returns

Not much of a patch but probably doesn't hurt.

Comment 5

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Stealing review.

Thank you!!

Comment 6

[Eddy Bruel [:ejpbruel]]

(In reply to Cosmin Radoi from comment #4)
> *now returns
> 
> Not much of a patch but probably doesn't hurt.

Jim and I agreed that we don't want to break the existing API. It's probably better to add a new function that does this. Ok if I take this patch from here?

Comment 7

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> (I'm not sure what possessed me to make this a Vector subclass in the
> first place.)

Conflating is-a and has-a is a hallowed Mozilla tradition. Just look at any one of our hundreds of C++ classes that implement a half-dozen unrelated IDL classes.

(Seriously, making that a subclass of Vector is insane.)

Comment 8

[Jim Blandy :jimb]

Comment on attachment 714464 [details] [diff] [review]
getLineOffsets not returns a sparse array, i.e. object, from column to bytecode offset

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

I agree with Eddy: while I think it's a decent idea for how to present the information, let's do it via a new API method, not by making an incompatible change to an existing method.

::: js/src/vm/Debugger.cpp
@@ +3227,5 @@
>      if (!flowData.populate(cx, script))
>          return false;
>  
> +    /* Second pass: build the result sparse array, i.e., object. */
> +    JSObject *result = JS_NewObject(cx, NULL, NULL, NULL);

This still needs to be a RootedObject, as JS_SetPropertyById can GC.

Comment 9

[Cosmin Radoi]

(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> Jim and I agreed that we don't want to break the existing API. It's probably
> better to add a new function that does this. Ok if I take this patch from
> here?

Not sure if you're asking me but, if you do, of course. I'm happy to go back to my pointer-free world.

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Implement getAllColumnOffsets + test

Tests are admittedly a bit meager, but none of the existing tests for getLineOffsets seem to translate 1-on-1, and getAllColumnOffsets reuses most of the same code. If you have any suggestions for tests, I'd love to hear it.

Comment 11

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a02cad4f3238

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a02cad4f3238

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 715298 [details] [diff] [review]
Implement getAllColumnOffsets + test

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

::: js/src/jit-test/tests/debug/Script-getAllColumnOffsets-01.js
@@ +5,5 @@
> +    var script = frame.eval("f").return.script;
> +    script.getAllColumnOffsets().forEach(function (offset) {
> +        script.setBreakpoint(offset.offset, {
> +            hit: function (frame) {
> +                global.log += offset.columnNumber + " ";

This test is fine. It'd be nice to check that .lineNumber is actually there.

::: js/src/vm/Debugger.cpp
@@ +3217,5 @@
> +                return false;
> +
> +            RootedId id(cx, NameToId(cx->names().lineNumber));
> +            RootedValue value(cx, NumberValue(lineno));
> +            if (!JSObject::defineGeneric(cx, entry, id, value))

Better to use defineProperty, which takes a PropertyName * as the third argument. No need to call NameToId.

Comment 14

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54f36ac812fd

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54f36ac812fd

Comment 16

[Cosmin Radoi]

Hello,

I think there might be a problem with the getAllColumnOffsets method. For this example:

var foo = function() {
	x = 0;
}
var a = [1, 2, 3]; 

It outputs:
[{lineNumber:1, columnNumber:0, offset:10}, {lineNumber:4, columnNumber:0, offset:26}, {lineNumber:4, columnNumber:17, offset:42}]

It misses everything that is inside foo(). I've also tried with larger examples.

I am using Attachment #715298 [details] [diff]. Hopefully my small changes in other parts didn't break the method - they shouldn't.

Comment 17

[Eddy Bruel [:ejpbruel]]

(In reply to Cosmin Radoi from comment #16)
> Hello,
> 
> I think there might be a problem with the getAllColumnOffsets method. For
> this example:
> 
> var foo = function() {
> 	x = 0;
> }
> var a = [1, 2, 3]; 
> 
> It outputs:
> [{lineNumber:1, columnNumber:0, offset:10}, {lineNumber:4, columnNumber:0,
> offset:26}, {lineNumber:4, columnNumber:17, offset:42}]
> 
> It misses everything that is inside foo(). I've also tried with larger
> examples.
> 
> I am using Attachment #715298 [details] [diff] [diff]. Hopefully my small changes
> in other parts didn't break the method - they shouldn't.

I would expect getAllColumnOffsets to miss everything inside foo, since every function has its own script. Doesn't getLineOffsets do this too?

jimb, can you tell me if this behavior is expected?

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Yes, that's how getLineOffsets behaves in my experience. You need to call it on the inner script to get those bytecodes.