Bug 1518661 (breakpoint-specificity)

Make steppable/breakable positions first-class concepts in SpiderMonkey and use in Debugger server

RESOLVED FIXED in Firefox 67

Status

enhancement
P1
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: loganfsmyth, Assigned: loganfsmyth)

Tracking

(Blocks 1 bug)

Trunk
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

4 months ago

As we implement column breakpoints, we want to be able to pause at exactly the right bytecode.

Right now, the difficulty is that while all bytecodes have some position associated with them, the introduction of a new position automatically introduces a new possible pause location. It's also possible for multiple bytecodes to map to the same location, meaning that the debugger basically has to take the first one as the breakpoint bytecode, and hope for the best.

Our goal is to make breakpoint locations a first-class concept in SpiderMonkey's bytecode emitter so that we can clearly define what positions are breakable. Along those same lines, there is also the question of how far to step whenever the user uses the "Step" button in the debugger. While stepping could step to the next breakpoint, usually that would be much too often. To resolve this, the work here also marks the first breakable point in a statement as a steppable point as well.

Assignee

Comment 3

4 months ago

Alright, here's my prototyped approach, very curious to hear thoughts.

Comment 4

4 months ago

Okay, I think I've commented on both patches now.

Alias: breakpoint-specificity
Priority: P2 → P1
Assignee

Updated

4 months ago
Summary: Make steppable/breakable positions first-class concepts in SpiderMonkey → Make steppable/breakable positions first-class concepts in SpiderMonkey and use in Debugger server
Assignee

Comment 5

4 months ago

This Maybe<> isn't needed and means that when we eventually mark these bytecodes as
breakpoints, we'd have to skip if there is no position, which could add complexity.

Assignee

Comment 6

4 months ago

This is just a bit of cleanup I'd noticed while writing new implementations of these.

Depends on D17658

Assignee

Comment 7

4 months ago

Making changes to this test is kind of annoying because the count and
the array items need to be kept in sync. This just avoids that.

Depends on D17659

Assignee

Comment 8

4 months ago

When we mark call expressions as breakpoints, we want to make it as likely
as possible that the call has its own unique positon. The existing logic
means that it is more likely that the beginning of a call will align
with the start of an expression statement or other debuggable step point.
By using the property-access location, we're less likely to collide.

Thid also adds a new bytecodes that were missed in the original code that
added this position handling logic.

Depends on D17660

Assignee

Comment 9

4 months ago

This position ends up being used for source notes in some cases now, meaning that this can
cause breakpoints to be given the wrong position when assigned to a variable. This fixes
that by using the correct token for the position value.

Depends on D17661

Attachment #9035178 - Attachment description: Bug 1518661 - Part 1: Convert bytecode positions to be more expression-oriented. r=jimb,jorendorff,bhackett → Bug 1518661 - Part 6: Convert bytecode positions to be more expression-oriented. r=jimb!,jorendorff!
Attachment #9035179 - Attachment description: Bug 1518661 - Part 2: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb,jorendorff,bhackett → Bug 1518661 - Part 7: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb!,jorendorff!
Assignee

Comment 10

4 months ago

The step-out logic currently has a special case to skip pausing onPop
when stepping out of the current frame. This logic gets confused if
you are already in the onPop of the current frame though, and
causes you to also fail to pause onPop in the parent frame.

Depends on D15994

Assignee

Comment 11

4 months ago

Making use of the new SpiderMonkey APIs for available breakpoints means
that the server needs to think a lot less about where it is pausing and
allows us to drop the concept of a pause points from the server entirely.
It is now up to SpiderMonkey to decide where it will and will not stop
when it is stepping.

Depends on D17663

Attachment #9039154 - Attachment description: Bug 1518661 - Part 2: Pass PropertyName* directly instead of creating ID. r=jimb!,jorendorff! → Bug 1518661 - Part 1: Pass PropertyName* directly instead of creating ID. r=jimb!,jorendorff!
Attachment #9039156 - Attachment description: Bug 1518661 - Part 3: Avoid separate count in pause point test. r=jlast! → Bug 1518661 - Part 2: Avoid separate count in pause point test. r=jlast!
Attachment #9039157 - Attachment description: Bug 1518661 - Part 4: Adjust call-expression positions. r=jimb!,jorendorff! → Bug 1518661 - Part 3: Adjust call-expression positions. r=jimb!,jorendorff!
Attachment #9039158 - Attachment description: Bug 1518661 - Part 5: Assign the correct start position to arrow functions. r=jimb!,jorendorff! → Bug 1518661 - Part 4: Assign the correct start position to arrow functions. r=jimb!,jorendorff!
Attachment #9035178 - Attachment description: Bug 1518661 - Part 6: Convert bytecode positions to be more expression-oriented. r=jimb!,jorendorff! → Bug 1518661 - Part 5: Convert bytecode positions to be more expression-oriented. r=jimb!,jorendorff!
Attachment #9035179 - Attachment description: Bug 1518661 - Part 7: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb!,jorendorff! → Bug 1518661 - Part 6: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb!,jorendorff!
Attachment #9039159 - Attachment description: Bug 1518661 - Part 8: Ensure that stepOut from inside onPop behaves properly. r=jlast! → Bug 1518661 - Part 7: Ensure that stepOut from inside onPop behaves properly. r=jlast!
Attachment #9039162 - Attachment description: Bug 1518661 - Part 9: Update debugger server to use new getPossibleBreakpoints APIs. r=jlast! → Bug 1518661 - Part 8: Update debugger server to use new getPossibleBreakpoints APIs. r=jlast!
Attachment #9039153 - Attachment is obsolete: true
Assignee

Comment 13

4 months ago

Alright, this addresses all the current comments and rebases to fix some conflicts.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba6af5abc616502ccb6fdd682c80c65f634b9a8

Assignee

Comment 14

4 months ago

That last Try run was I guess based on a broken commit, so here's a new one rebased again this morning:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59ad650be545f96654a8d32a93fa87258c676361&selectedJob=226235154

Comment 15

3 months ago
Pushed by lsmyth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bad1d8c8944
Part 1: Pass PropertyName* directly instead of creating ID. r=jimb,jorendorff
https://hg.mozilla.org/integration/autoland/rev/02ddcca425a2
Part 2: Avoid separate count in pause point test. r=jlast
https://hg.mozilla.org/integration/autoland/rev/dd2caed01823
Part 3: Adjust call-expression positions. r=jimb,jorendorff
https://hg.mozilla.org/integration/autoland/rev/6c45ce836a12
Part 4: Assign the correct start position to arrow functions. r=jimb,jorendorff

Comment 17

3 months ago
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ee3ed23dd3c
Backed out 4 changesets for spidermonkey build bustages CLOSED TREE
Attachment #9035178 - Attachment description: Bug 1518661 - Part 5: Convert bytecode positions to be more expression-oriented. r=jimb!,jorendorff! → Bug 1518661 - Part 4: Convert bytecode positions to be more expression-oriented. r=jimb!,jorendorff!
Attachment #9035179 - Attachment description: Bug 1518661 - Part 6: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb!,jorendorff! → Bug 1518661 - Part 5: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb!,jorendorff!
Attachment #9039159 - Attachment description: Bug 1518661 - Part 7: Ensure that stepOut from inside onPop behaves properly. r=jlast! → Bug 1518661 - Part 6: Ensure that stepOut from inside onPop behaves properly. r=jlast!
Attachment #9039162 - Attachment description: Bug 1518661 - Part 8: Update debugger server to use new getPossibleBreakpoints APIs. r=jlast! → Bug 1518661 - Part 7: Update debugger server to use new getPossibleBreakpoints APIs. r=jlast!
Attachment #9039158 - Attachment is obsolete: true

Comment 18

3 months ago
Pushed by lsmyth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4ff0bd9ff80
Part 1: Pass PropertyName* directly instead of creating ID. r=jimb,jorendorff
https://hg.mozilla.org/integration/autoland/rev/12df8c46ef78
Part 2: Avoid separate count in pause point test. r=jlast
https://hg.mozilla.org/integration/autoland/rev/aeaa74707320
Part 3: Adjust call-expression positions. r=jimb,jorendorff

Backed out for dom/bindings/test/test_exception_options_from_jsimplemented.html failures

backout: https://hg.mozilla.org/integration/autoland/rev/e14384fe64d93d63c18518d9b877d721f192e9ca

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=aeaa74707320c6c9af5fd1b8c234cba7608f2671&selectedJob=227828747

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227828747&repo=autoland&lineNumber=2149

[task 2019-02-12T04:08:52.756Z] 04:08:52 INFO - TEST-START | dom/bindings/test/test_exception_options_from_jsimplemented.html
[task 2019-02-12T04:08:52.854Z] 04:08:52 INFO - GECKO(1241) | ++DOMWINDOW == 70 (0xe26c0000) [pid = 1241] [serial = 103] [outer = 0xdc3217a0]
[task 2019-02-12T04:08:52.936Z] 04:08:52 INFO - TEST-INFO | started process screentopng
[task 2019-02-12T04:08:53.406Z] 04:08:53 INFO - TEST-INFO | screentopng: exit 0
[task 2019-02-12T04:08:53.406Z] 04:08:53 INFO - Buffered messages logged at 04:08:52
[task 2019-02-12T04:08:53.406Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have an Error here
[task 2019-02-12T04:08:53.406Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should not have DOMException here
[task 2019-02-12T04:08:53.406Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should not have a 'code' property
[task 2019-02-12T04:08:53.407Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should not have an interesting name here
[task 2019-02-12T04:08:53.411Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have the right message
[task 2019-02-12T04:08:53.413Z] 04:08:53 INFO - Buffered messages finished
[task 2019-02-12T04:08:53.415Z] 04:08:53 INFO - TEST-UNEXPECTED-FAIL | dom/bindings/test/test_exception_options_from_jsimplemented.html | Exception stack should still only show our code - got "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_exception_options_from_jsimplemented.html:31:9\nAsync*@http://mochi.test:8888/tests/dom/bindings/test/test_exception_options_from_jsimplemented.html:153:17\n", expected "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_exception_options_from_jsimplemented.html:31:7\nAsync*@http://mochi.test:8888/tests/dom/bindings/test/test_exception_options_from_jsimplemented.html:153:3\n"
[task 2019-02-12T04:08:53.416Z] 04:08:53 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:16
[task 2019-02-12T04:08:53.418Z] 04:08:53 INFO - doTest@dom/bindings/test/test_exception_options_from_jsimplemented.html:38:7
[task 2019-02-12T04:08:53.420Z] 04:08:53 INFO - Async*@dom/bindings/test/test_exception_options_from_jsimplemented.html:153:17
[task 2019-02-12T04:08:53.421Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have the right file name
[task 2019-02-12T04:08:53.423Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have the right line number
[task 2019-02-12T04:08:53.425Z] 04:08:53 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-02-12T04:08:53.428Z] 04:08:53 INFO - TEST-UNEXPECTED-FAIL | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have the right column number - got 9, expected 7
[task 2019-02-12T04:08:53.431Z] 04:08:53 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:16
[task 2019-02-12T04:08:53.433Z] 04:08:53 INFO - doTest@dom/bindings/test/test_exception_options_from_jsimplemented.html:46:7
[task 2019-02-12T04:08:53.434Z] 04:08:53 INFO - Async*@dom/bindings/test/test_exception_options_from_jsimplemented.html:153:17
[task 2019-02-12T04:08:53.436Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should also have an Error here
[task 2019-02-12T04:08:53.438Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have DOMException here
[task 2019-02-12T04:08:53.440Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have the right name here
[task 2019-02-12T04:08:53.444Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should also have the right message
[task 2019-02-12T04:08:53.446Z] 04:08:53 INFO - TEST-PASS | dom/bindings/test/test_exception_options_from_jsimplemented.html | Should have the right 'code'
[task 2019-02-12T04:08:53.448Z] 04:08:53 INFO - Not taking screenshot here: see the one that was previously logged

Comment 20

3 months ago
Pushed by lsmyth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64ebb085a6b3
Part 1: Pass PropertyName* directly instead of creating ID. r=jimb,jorendorff
https://hg.mozilla.org/integration/autoland/rev/4a8b38e5373b
Part 2: Avoid separate count in pause point test. r=jlast
https://hg.mozilla.org/integration/autoland/rev/d381785b0c4c
Part 3: Adjust call-expression positions. r=jimb,jorendorff
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15370 for changes under testing/web-platform/tests

Comment 23

3 months ago
Pushed by lsmyth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7f9a1180f23
Part 1: Pass PropertyName* directly instead of creating ID. r=jimb,jorendorff
https://hg.mozilla.org/integration/autoland/rev/fde198422591
Part 2: Avoid separate count in pause point test. r=jlast
https://hg.mozilla.org/integration/autoland/rev/82a6589f4b91
Part 3: Adjust call-expression positions. r=jimb,jorendorff
Assignee

Updated

3 months ago
Status: RESOLVED → REOPENED
Flags: needinfo?(lsmyth)
Resolution: FIXED → ---

Comment 25

3 months ago
Pushed by lsmyth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5add2761a3b6
Part 4: Convert bytecode positions to be more expression-oriented. r=jimb,jorendorff
https://hg.mozilla.org/integration/autoland/rev/5c934ede1cfc
Part 5: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb,bhackett
https://hg.mozilla.org/integration/autoland/rev/62f3c188b868
Part 6: Ensure that stepOut from inside onPop behaves properly. r=jlast
https://hg.mozilla.org/integration/autoland/rev/266c1eee61a8
Part 7: Update debugger server to use new getPossibleBreakpoints APIs. r=jlast
Assignee

Updated

3 months ago
Blocks: 1527671
Depends on: 1528654

Updated

3 months ago
Depends on: 1529332
Depends on: 1530423
No longer depends on: 1530423
Depends on: 1532748
You need to log in before you can comment on or make changes to this bug.