863089 - Replace Debugger.Script.getOffsetLine with Debugger.Script.prototype.getOffsetLocation (which returns line and column numbers, not only the line number)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

getOffsetColumn()

Returns the column number in the original source code file on which the specified bytecode offset occurs.

Number getOffsetColumn(
  offset
);

Parameters:

offset
    The offset to the bytecode for which to retrieve the corresponding source code column number.

Return value:

The column number on which the specified bytecode offset occurs.

Exceptions thrown:

TypeError
    The specified offset is invalid.

Comment 1

[Jeff Walden [:Waldo]]

I thought the debugger API/spec/etc. deliberately didn't consider anything about bytecode, as bytecode is an implementation detail.  Am I wrong?  :-\

Comment 2

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

Waldo,

We already have |Debugger.Script.prototype.getOffsetLine| but we also need columns so we can properly set breakpoints in minified scripts. See my related work in bug 860035, which has a wip patch with a shim to get around the lack of this method.

Whether it was a good idea to expose bytecode offsets originally, well I trust the designers of the API, and since they're already out, I don't think I can contribute anything by discussing it more right now :)

Comment 3

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

After talking with Jim on IRC, we decided we should just change the existing method to return an object which has both line and column numbers. No need to do the search twice.

Comment 4

[Jim Blandy :jimb]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> I thought the debugger API/spec/etc. deliberately didn't consider anything
> about bytecode, as bytecode is an implementation detail.  Am I wrong?  :-\

That would have been nice, but Debugger does expose bytecode offsets, because the source relationship was too complicated for me to feel sure I'd covered all the cases. Instead, Debugger simply exposes the data we have, and makes the JS responsible for sorting out the details. :(

Comment 5

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

Unassigning myself because I am caught up in other things at the moment and I'm about to be gone for two weeks in Poland.

Comment 6

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

Hey Eddy, you were asking me for places you could help out, how does this sound to you?

Jim and I were talking on IRC about having this return {lineNumber, columnNumber, offset} objects just like other jsdbg2 methods.

Comment 7

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Hey Eddy, you were asking me for places you could help out, how does this
> sound to you?
> 
> Jim and I were talking on IRC about having this return {lineNumber,
> columnNumber, offset} objects just like other jsdbg2 methods.

Hey Nick, sorry for not replying on this bug for so long. It kind of disappeared from my radar for a while. I'll assign this bug to me, and I'll take a look at it as soon as I can make time.

Comment 8

[Tom Tromey :tromey]

Attachment: replace Debugger.Script.getOffsetLine with getOffsetLocation

Initial patch for safekeeping.
This still needs tests to exercise the new functionality.

Comment 9

[Tom Tromey :tromey]

Attachment: replace Debugger.Script.getOffsetLine with getOffsetLocation

Added a test case derived from the existing getAllColumnOffsets tests.

Comment 11

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

Comment on attachment 8675839 [details] [diff] [review]
replace Debugger.Script.getOffsetLine with getOffsetLocation

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

Thanks very much!

Comment 12

[Tom Tromey :tromey]

Attachment: replace Debugger.Script.getOffsetLine with getOffsetLocation

Fix some bugs and tests.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8677545 [details] [diff] [review]
replace Debugger.Script.getOffsetLine with getOffsetLocation

A couple of notes on the changes.

One change was trivial -- changing a "let" to a "var" in a test.
I did this just for consistency, it isn't important.


The other change is a bit funnier.  I had to update test_blackboxing-01.js
to adapt to this patch.

The code here used to use PCToLineNumber, but now it uses BytecodeRangeWithPosition.
Previously I had thought that these did the same thing, but in fact they
differ in their handling of function prologue.  In particular, 
BytecodeRangeWithPosition skips the prologue, while PCToLineNumber does not.

What this means is that PCToLineNumber returns the "zeroth" line number of
the function when given an offset in the prologue, but BytecodeRangeWithPosition
gives the line number of the first "main" instruction instead.

I think the BytecodeRangeWithPosition behavior is better, but I'm re-requesting
review just in case.

Comment 14

[Tom Tromey :tromey]

It's worth noting:

https://bugzilla.mozilla.org/show_bug.cgi?id=969816#c7

Maybe the current behavior is intentional.

I still think the change is for the better.

However, if not, then it seems odd that the other Debugger.Script methods
skip the prologue.

Comment 15

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

Comment on attachment 8677545 [details] [diff] [review]
replace Debugger.Script.getOffsetLine with getOffsetLocation

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

r=me

Comment 16

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a2de389ad3b

Comment 17

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe3cb773f98

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57714b421d53

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57714b421d53