1586452 - [jsdbg2] Trace breakpoints directly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jim Blandy :jimb]

The algorithm the garbage collector uses to retain Debuggers due to live breakpoint handlers is more complicated than necessary, and should be replaced with a more direct approach.

In the current code, each time through the iterative weak map marking cycle, we search the JSRuntime for marked global objects, see if they are debuggees, and if so, see if their Debuggers have any breakpoints set in marked JSScripts or WasmInstanceObjects. If so, then we mark the Debugger, which marks its breakpoints.

It would be equivalent and simpler to have tracing a JSScript or WasmInstanceObject trace their breakpoint sites, which then traces breakpoints, which then traces Debuggers.

Comment 1

[Jim Blandy :jimb]

Attachment: Bug 1586452: Remove stray references to 'enabled' debuggers.

The Debugger.prototype.enabled accessor was removed in bug 1564168, but there
are still a few stray references to 'enabled' or 'disabled' debuggers in the
docs and the code. These should be cleaned up.

Comment 2

[Jim Blandy :jimb]

Attachment: Bug 1586452: Remove BreakpointSite::enabledCount.

Between breakpoint operations, a BreakpointSite's enabledCount is greater than
zero if and only if its list of breakpoints is empty. Since checking whether the
list is empty is cheap, there's no reason to maintain a separate count.

Depends on D48465

Comment 3

[Jim Blandy :jimb]

Attachment: skip-toggle.patch

Yury, I would expect this patch to introduce a bug, because it skips the call to BreakpointSite::recompile for Wasm breakpoints. The test file js/src/jit-test/tests/debug/wasm-breakpoint.js does cover this case, but it seems like the problem gets fixed up by accident someplace else.

Why this patch? I introduced an equivalent bug in the midst of another patch series, and noticed it by eye, and was worried that the tests hadn't caught it. I tried to come up with a test case that would expose the bug, but I haven't been able to.

Can you see any way to come up with a test case that detects the bug this patch introduces?

Comment 4

[Jim Blandy :jimb]

Attachment: Bug 1586452: Use tighter type JSBreakpointSite where possible. r?jonco

Depends on D48465

Comment 5

[Jim Blandy :jimb]

Attachment: Bug 1586452: Make BreakpointSite constructor and destructor protected. r?jonco

Depends on D49850

Comment 6

[Jim Blandy :jimb]

Attachment: Bug 1586452: Let WasmBreakpointSite hold a WasmInstanceObject*, not a wasm::Instance*. r?jonco

Depends on D49851

Comment 7

[Jim Blandy :jimb]

Attachment: Bug 1586452: Remove class js::WasmBreakpoint; use plain js::Breakpoint instead. r?jonco

Depends on D49852

Comment 8

[Jim Blandy :jimb]

Attachment: Bug 1586452: Make `js::BreakpointSite::remove` the pure virtual method, and hoist `removeIfEmpty' into the base class. r?jonco

Depends on D49853

Comment 9

[Jim Blandy :jimb]

Attachment: Bug 1586452: Manage JIT traps when we create and remove breakpoint sites. r?jonco

Depends on D49854

Comment 10

[Jim Blandy :jimb]

Attachment: Bug 1586452: SetBreakpointMatcher: success path should be main line. r?jonco

Depends on D49855

Comment 11

[Jim Blandy :jimb]

Attachment: Bug 1586452: Separate removal and destruction of Breakpoints into two methods. r?jonco

Depends on D49856

Comment 12

[Jim Blandy :jimb]

Attachment: Bug 1586452: Let JSScripts and wasm::Instances own their BreakpointSites and Breakpoints. r?jonco

Depends on D49857

Comment 13

[Jim Blandy :jimb]

That last is a work in progress. Current failures:

    debug/Debugger-onNativeCall-01.js
    debug/Script-clearBreakpoint-01.js
    debug/Script-clearBreakpoint-02.js
    debug/Script-clearBreakpoint-03.js
    debug/Script-getBreakpoints-01.js
    debug/Script-getBreakpoints-02.js
    debug/breakpoint-multi-02.js
    debug/breakpoint-multi-04.js
    debug/wasm-breakpoint.js

Comment 14

[Phabricator Automation]

Comment on attachment 9099434 [details]
Bug 1586452: Remove stray references to 'enabled' debuggers.

Revision D48465 was moved to bug 1590589. Setting attachment 9099434 [details] to obsolete.

Comment 15

[Jim Blandy :jimb]

Okay, that's the updated patch queue.

JS-centric try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29e6a9f0db9bc1f21b9133a805c5bc6d86af9ff8

Comment 16

[Jim Blandy :jimb]

Looks green! I'm astonished.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1acb660e5fc4
Use tighter type JSBreakpointSite where possible. r=jonco
https://hg.mozilla.org/integration/autoland/rev/0a1e6f5aa6c3
Make BreakpointSite constructor and destructor protected. r=jonco
https://hg.mozilla.org/integration/autoland/rev/d0d79b88fd49
Let WasmBreakpointSite hold a WasmInstanceObject*, not a wasm::Instance*. r=jonco
https://hg.mozilla.org/integration/autoland/rev/4d3fe0b93397
Remove class js::WasmBreakpoint; use plain js::Breakpoint instead. r=jonco
https://hg.mozilla.org/integration/autoland/rev/7fc4169017bc
Make `js::BreakpointSite::remove` the pure virtual method, and hoist `removeIfEmpty' into the base class. r=jonco
https://hg.mozilla.org/integration/autoland/rev/b49320ce9db5
Manage JIT traps when we create and remove breakpoint sites. r=jonco
https://hg.mozilla.org/integration/autoland/rev/df4696b00c37
SetBreakpointMatcher: success path should be main line. r=jonco
https://hg.mozilla.org/integration/autoland/rev/dca8e34d8b27
Separate removal and destruction of Breakpoints into two methods. r=jonco
https://hg.mozilla.org/integration/autoland/rev/843d64235cfa
Let JSScripts and wasm::Instances own their BreakpointSites and Breakpoints. r=jonco

Comment 18

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1acb660e5fc4
https://hg.mozilla.org/mozilla-central/rev/0a1e6f5aa6c3
https://hg.mozilla.org/mozilla-central/rev/d0d79b88fd49
https://hg.mozilla.org/mozilla-central/rev/4d3fe0b93397
https://hg.mozilla.org/mozilla-central/rev/7fc4169017bc
https://hg.mozilla.org/mozilla-central/rev/b49320ce9db5
https://hg.mozilla.org/mozilla-central/rev/df4696b00c37
https://hg.mozilla.org/mozilla-central/rev/dca8e34d8b27
https://hg.mozilla.org/mozilla-central/rev/843d64235cfa