If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[wasm] Expose WebAssembly instance memory and globals via debugger scope

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 months ago
25 days ago

People

(Reporter: yury, Assigned: yury)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 months ago
Currently, we provide access to the wasm function locals via scope we are creating for the debugger. The wasm code often uses its (internal) memory and globals too, so it will be nice to inspects these values too during debugging as well.

A WebAssembly memory is just an object that contains ArrayBuffer. Currently, no devtools can display content of an ArrayBuffer. So some type of watch expression needs to be provided to inspect memory content, e.g. `new DataView(memory0.buffer).getUint32(0x1000, true)`. For that we need to implement an eval functionality for the wasm debug frames.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 months ago
mozreview-review
Comment on attachment 8893485 [details]
Bug 1387115 - Expose WebAssembly instance memory and globals via debugger scope.

https://reviewboard.mozilla.org/r/164556/#review169972

Great!

::: js/src/vm/Scope.cpp:1264
(Diff revision 1)
> +/* static */ WasmInstanceScope*
> +WasmInstanceScope::create(JSContext* cx, WasmInstanceObject* instance)
> +{
> +    Rooted<WasmInstanceScope*> wasmInstanceScope(cx);
> +
> +    {

Is this { } block wrapping most of the function body necessary?

::: js/src/wasm/WasmDebug.cpp:572
(Diff revision 1)
> +            break;
> +          case ValType::F32:
> +            vp.set(NumberValue(value.f32()));
> +            break;
> +          case ValType::F64:
> +            vp.set(NumberValue(value.f64()));

I think you have to CanonicalizeNaN() these since the bits come from the .wasm.

Actually, could you also add CanonicalizeNaN() the f32/f64 constant cases in GetGlobalExport()?  It's missing there for the same reason.

::: js/src/wasm/WasmJS.cpp:1065
(Diff revision 1)
>  
> +    Rooted<WasmInstanceScope*> instanceScope(cx, WasmInstanceScope::create(cx, obj));
> +    if (!instanceScope)
> +        return nullptr;
> +
> +    obj->initReservedSlot(INSTANCE_SCOPE_SLOT, PrivateGCThingValue(instanceScope));

Could you create the WasmInstanceScope lazily, changing the infallible WasmInstanceObject::scope() to a fallible getScope()?
Attachment #8893485 - Flags: review?(luke) → review+

Comment 4

2 months ago
mozreview-review
Comment on attachment 8893486 [details]
Bug 1387115 - Allow debugger to eval using WebAssembly frames.

https://reviewboard.mozilla.org/r/164558/#review170028

Nice!  Could you add tests for all these new paths?
Attachment #8893486 - Flags: review?(luke) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 months ago
mozreview-review-reply
Comment on attachment 8893485 [details]
Bug 1387115 - Expose WebAssembly instance memory and globals via debugger scope.

https://reviewboard.mozilla.org/r/164556/#review169972

> Is this { } block wrapping most of the function body necessary?

I moved comment from WasmFunctionScope::create about CGPtr for Data structure that explain block. However it is not longer needed in WasmFunctionScope::create, shall I remove { } block wrapping there?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 months ago
(In reply to Yury Delendik (:yury) from comment #7)
Ah, I see now this is an idiomatic pattern introduced so it's clear that the Rooted<> has to be the outermost stack lifetime.  n/m this comment, but it does make sense to take out the {} for WasmFunctionScope::create() now that it has no such hazard.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 months ago
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a301b2aa1267
Expose WebAssembly instance memory and globals via debugger scope. r=luke
https://hg.mozilla.org/integration/autoland/rev/4f6eab9d705f
Allow debugger to eval using WebAssembly frames. r=luke
https://hg.mozilla.org/mozilla-central/rev/a301b2aa1267
https://hg.mozilla.org/mozilla-central/rev/4f6eab9d705f
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

26 days ago
See Also: → bug 1380677
(Assignee)

Updated

25 days ago
Blocks: 1232009
You need to log in before you can comment on or make changes to this bug.