Closed Bug 1343579 Opened 7 years ago Closed 7 years ago

Baldr: make Pause-on-Exception break in wasm code

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: luke, Assigned: yury)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Right now, if the "Pause on Exception" button is enabled and you trap in wasm, the exception will propagate until the first JS caller and that's where the Debugger will break.  Ideally, we'd break at the trapping instruction.  I think this would be a *super* useful option for anyone debugging out-of-bounds or div-by-zero etc traps.

Attached is .html/.wasm test case.
Attached file trap.tar.gz
Assignee: nobody → ydelendik
Status: NEW → ASSIGNED
Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120766

Is implementing isInCatchScope the only thing needed to get this functionality in the frontend, or are there more patches?

::: js/src/vm/Debugger.cpp:6903
(Diff revision 2)
> -    size_t offset;
> -    if (!ScriptOffset(cx, script, args[0], &offset))
> +  public:
> +    explicit DebuggerScriptIsInCatchScopeMatcher(JSContext* cx, size_t offset) : cx_(cx), offset_(offset) { }
> +    using ReturnType = bool;
> +
> +    ReturnType match(HandleScript script) {
> +        if (!EnsureScriptOffsetIsValid(cx_, script, offset_))

This will not propagate errors correctly. The matcher returns true/false to mean "is in catch scope", not fallibility.
Attachment #8843183 - Flags: review?(shu)
Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120766

That's correct. Implementing isInCatchScope fixes the test case provided in comment 0.

> This will not propagate errors correctly. The matcher returns true/false to mean "is in catch scope", not fallibility.

Fixed. Thanks for spotting it.
Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120800

The implementation of isInCatchScope looks fine, though I don't really understand how this enables the frontend debugger to pause on wasm exceptions.
Attachment #8843183 - Flags: review?(shu) → review+
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/014cd346dff3
Implements Debugger.Script.isInCatchScope for wasm scripts. r=shu
Comment on attachment 8843183 [details]
Bug 1343579 - Implements Debugger.Script.isInCatchScope for wasm scripts.

https://reviewboard.mozilla.org/r/116950/#review120800

We implemented most of the logic in previous bugs (e.g. bug 1286948). The devtools server part was breaking on isInCatchScope at https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js#1832, after fixing Debugger API for wasm script everything started working.
https://hg.mozilla.org/mozilla-central/rev/014cd346dff3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: