Closed Bug 1387115 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: yury, Assigned: yury)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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 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 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 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?
(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.
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
See Also: → 1380677
Blocks: wasm-tools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: