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.

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

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

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

(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?

(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 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.

(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.

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 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.

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.

(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?

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