1518661 - (breakpoint-specificity) Make steppable/breakable positions first-class concepts in SpiderMonkey and use in Debugger server

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P1)

Description

[Logan Smyth [:loganfsmyth]]

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.

Comment 1

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 4: Convert bytecode positions to be more expression-oriented. r=jimb!,jorendorff!

Comment 2

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 5: Give SpiderMonkey well-defined sense of step and breakpoint locations. r=jimb!,jorendorff!

Depends on D15993

Comment 3

[Logan Smyth [:loganfsmyth]]

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

Comment 4

[Jim Blandy :jimb]

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

Comment 5

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 1: Remove unnecessary Maybe in CallOrNewEmitter. r=jimb!,jorendorff!

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.

Comment 6

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 1: Pass PropertyName* directly instead of creating ID. r=jimb!,jorendorff!

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

Depends on D17658

Comment 7

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 2: Avoid separate count in pause point test. r=jlast!

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

Comment 8

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 3: Adjust call-expression positions. r=jimb!,jorendorff!

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

Comment 9

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 4: Assign the correct start position to arrow functions. r=jimb!,jorendorff!

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

Comment 10

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 6: Ensure that stepOut from inside onPop behaves properly. r=jlast!

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

Comment 11

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1518661 - Part 7: Update debugger server to use new getPossibleBreakpoints APIs. r=jlast!

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

Comment 12

[Logan Smyth [:loganfsmyth]]

Try run on the overall patch set: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d3c64da1444de35d72f5a24a830aa52f994c7c3

Comment 13

[Logan Smyth [:loganfsmyth]]

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

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

Comment 14

[Logan Smyth [:loganfsmyth]]

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

[Pulsebot]

Pushed by [e-mail]:
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 16

[Stefan Hindli [:stefan_hindli]]

Backed out 4 changesets (Bug 1518661) for spidermonkey build bustages CLOSED TREE

For the following crashes:

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=010180a16818d72bcbb68940509386b3318fd93b&selectedJob=227317489

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=010180a16818d72bcbb68940509386b3318fd93b&selectedJob=227316536

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=010180a16818d72bcbb68940509386b3318fd93b&selectedJob=227309426

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=010180a16818d72bcbb68940509386b3318fd93b&selectedJob=227319742

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227309426&repo=autoland&lineNumber=29060

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227316536&repo=autoland&lineNumber=5877

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227317489&repo=autoland&lineNumber=17063

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227319742&repo=autoland&lineNumber=2136

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ee3ed23dd3c
Backed out 4 changesets for spidermonkey build bustages CLOSED TREE

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Natalia Csoregi [:nataliaCs]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Andreea Pavel [:apavel]]

Backed out for failing test_promise_rejections_from_jsimplemented.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=228072229&searchStr=linux%2Cdebug%2Cmochitests%2Ctest-linux32%2Fdebug-mochitest-3%2Cm%283%29&revision=d381785b0c4c0acf1c733fb8ea77fdafb1d15f37

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228072229&repo=autoland&lineNumber=2278

Backout: https://hg.mozilla.org/integration/autoland/rev/e32e036906b9573147bc59a533b4af0f4e3e6988

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15370 for changes under testing/web-platform/tests

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b7f9a1180f23
https://hg.mozilla.org/mozilla-central/rev/fde198422591
https://hg.mozilla.org/mozilla-central/rev/82a6589f4b91

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/5add2761a3b6
https://hg.mozilla.org/mozilla-central/rev/5c934ede1cfc
https://hg.mozilla.org/mozilla-central/rev/62f3c188b868
https://hg.mozilla.org/mozilla-central/rev/266c1eee61a8