Closed Bug 1254893 Opened 8 years ago Closed 8 years ago

[jsdbg2] Synthesize Debugger.Scripts and Debugger.Sources for wasm

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(14 files, 8 obsolete files)

7.83 KB, patch
jimb
: review+
Details | Diff | Splinter Review
15.98 KB, patch
jimb
: review+
Details | Diff | Splinter Review
15.06 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.44 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.36 KB, patch
jimb
: review+
Details | Diff | Splinter Review
13.24 KB, patch
jimb
: review+
Details | Diff | Splinter Review
8.08 KB, patch
jimb
: review+
Details | Diff | Splinter Review
2.75 KB, patch
jimb
: review+
Details | Diff | Splinter Review
28.63 KB, patch
jimb
: review+
Details | Diff | Splinter Review
4.39 KB, patch
jimb
: review+
Details | Diff | Splinter Review
9.75 KB, patch
terrence
: review+
Details | Diff | Splinter Review
20.44 KB, patch
jimb
: review+
Details | Diff | Splinter Review
12.51 KB, patch
jimb
: review+
Details | Diff | Splinter Review
13.12 KB, patch
shu
: review+
Details | Diff | Splinter Review
The dream is to have as much of the existing devtools to work out of the box with wasm as we can. To that end we'll try to synthesize various Debugger wrappers to transparently support wasm so that devtools JS doesn't need to treat wasm differently.

This is the very first part: synthesize enough to display placeholder source text for wasm modules.
Blocks: 1232014
Depends on: 1254453
Attachment #8728297 - Flags: review?(terrence)
Attachment #8728297 - Flags: feedback?(luke)
Attachment #8728299 - Flags: review?(jimb)
Attachment #8728300 - Flags: review?(jimb)
I'll write tests tomorrow. I am tired now.
Example output right now:

shu@cz ~/m/c/j/s/Debug (dbg-wasm)
% cat test.js
var g = newGlobal();
g.eval(`o = Wasm.instantiateModule(wasmTextToBinary('(module (func) (export "" 0))'));`);
var dbg = new Debugger(g);
var scripts = dbg.findScripts();
for (let script of scripts) {
    if (script.format === "wasm")
        print(script.source.text);
}
                                                                                                                                                                
shu@cz ~/m/c/j/s/Debug (dbg-wasm)
% dist/bin/js test.js
.--.      .--.   ____       .-'''-. ,---.    ,---.
|  |_     |  | .'  __ `.   / _     \|    \  /    |
| _( )_   |  |/   '  \  \ (`' )/`--'|  ,  \/  ,  |
|(_ o _)  |  ||___|  /  |(_ o _).   |  |\_   /|  |
| (_,_) \ |  |   _.-`   | (_,_). '. |  _( )_/ |  |
|  |/    \|  |.'   _    |.---.  \  :| (_ o _) |  |
|  '  /\  `  ||  _( )_  |\    `-'  ||  (_,_)  |  |
|    /  \    |\ (_ o _) / \       / |  |      |  |
`---'    `---` '.(_,_).'   `-...-'  '--'      '--'
Assignee: nobody → shu
Status: NEW → ASSIGNED
Comment on attachment 8728295 [details] [diff] [review]
Prep Debugger.Script for a tagged union referent.

Review of attachment 8728295 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +4905,5 @@
> +//   2. A wasm JSFunction, denoting a synthesized wasm function script.
> +//      NYI!
> +using DebuggerScriptReferent = Variant<JSScript*, WasmModuleObject*>;
> +
> +// Very very scary. For internal use only.

Comments like this are unhelpful. And this function is perfectly well-typed anyway.

@@ +5049,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);                                       \
> +    RootedObject obj(cx, DebuggerScript_check(cx, args.thisv(), fnname));           \
> +    if (!obj)                                                                       \
> +        return false;                                                               \
> +    Rooted<DebuggerScriptReferent> referent(cx, GetScriptReferent(obj))

This is possible because we #included "js/GCVariant.h", from bug 1254453, right?
Attachment #8728295 - Flags: review?(jimb) → review+
(In reply to Shu-yu Guo [:shu] from comment #9)
> Example output right now:

Oh, I love it.
See Also: → 1254984
(In reply to Shu-yu Guo [:shu] from comment #9)
> .--.      .--.   ____       .-'''-. ,---.    ,---.
> |  |_     |  | .'  __ `.   / _     \|    \  /    |
> | _( )_   |  |/   '  \  \ (`' )/`--'|  ,  \/  ,  |
> |(_ o _)  |  ||___|  /  |(_ o _).   |  |\_   /|  |
> | (_,_) \ |  |   _.-`   | (_,_). '. |  _( )_/ |  |
> |  |/    \|  |.'   _    |.---.  \  :| (_ o _) |  |
> |  '  /\  `  ||  _( )_  |\    `-'  ||  (_,_)  |  |
> |    /  \    |\ (_ o _) / \       / |  |      |  |
> `---'    `---` '.(_,_).'   `-...-'  '--'      '--'

https://youtu.be/Mh5LY4Mz15o?t=3s
Comment on attachment 8728301 [details] [diff] [review]
Add a .format property on Debugger.Script and Debugger.Source.

Review of attachment 8728301 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by nit (the best kind of nit): needs docs.
(In reply to Jim Blandy :jimb from comment #10)
> Comment on attachment 8728295 [details] [diff] [review]
> Prep Debugger.Script for a tagged union referent.
> 
> Review of attachment 8728295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +5049,5 @@
> > +    CallArgs args = CallArgsFromVp(argc, vp);                                       \
> > +    RootedObject obj(cx, DebuggerScript_check(cx, args.thisv(), fnname));           \
> > +    if (!obj)                                                                       \
> > +        return false;                                                               \
> > +    Rooted<DebuggerScriptReferent> referent(cx, GetScriptReferent(obj))
> 
> This is possible because we #included "js/GCVariant.h", from bug 1254453,
> right?

That's right. I'll land the other bug with this one.
Comment on attachment 8728297 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

Review of attachment 8728297 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Attachment #8728297 - Flags: review?(terrence) → review+
Comment on attachment 8728297 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

Review of attachment 8728297 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks!
Attachment #8728297 - Flags: feedback?(luke) → feedback+
Attachment #8728298 - Attachment is obsolete: true
Attachment #8728298 - Flags: review?(jimb)
Attachment #8728903 - Flags: review?(jimb)
Attachment #8728299 - Attachment is obsolete: true
Attachment #8728299 - Flags: review?(jimb)
Attachment #8728904 - Flags: review?(jimb)
Attachment #8728300 - Attachment is obsolete: true
Attachment #8728300 - Flags: review?(jimb)
Attachment #8728905 - Flags: review?(jimb)
Debugger.Source can be distinguished via .introductionType == "wasm", so
.format isn't needed.
Attachment #8728301 - Attachment is obsolete: true
Attachment #8728301 - Flags: review?(jimb)
Attachment #8728907 - Flags: review?(jimb)
Attachment #8728909 - Flags: review?(jimb)
Attached patch Perfunctory tests. (obsolete) — Splinter Review
These really aren't very good, but I don't want to put too much effort into
testing something that's moving so quickly.
Attachment #8728910 - Flags: review?(jimb)
The problem here is that the wasm modules have the caller's filename (i.e. the
file that called Wasm.instantiateModule, which is the current nonstandard way
to make wasm modules). If that filename happens to be blacklisted for whatever
reason, no wasm sources will get displayed.

This is a quick and dirty hack to work around this until wasm modules become
real modules and have real URLs.
Attachment #8728912 - Flags: review?(jimb)
Comment on attachment 8728296 [details] [diff] [review]
Prep Debugger.Source for a tagged union referent.

Review of attachment 8728296 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +102,5 @@
> +
> +// Either a real ScriptSourceObject or a synthesized.
> +//
> +// If synthesized, the referent is a WasmModuleObject, denoting the
> +// synthesized source of a wasm module.

Simplify:

// Either a ScriptSourceObject, for ordinary JS, or a WasmModuleObject, denoting
// the synthesized source of a wasm module.

@@ +3914,5 @@
> +            DebuggerSourceReferent referent = GetSourceReferent(&debuggerSource.toObject());
> +            if (!referent.is<ScriptSourceObject*>()) {
> +                JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_UNEXPECTED_TYPE,
> +                                     "query object's 'source' property",
> +                                     "not undefined nor a Debugger.Source object for a JS script");

"neither undefined"

@@ +6144,2 @@
>          TraceManuallyBarrieredCrossCompartmentEdge(trc, obj, &referent,
> +                                                   "Debugger.Source source referent");

Why this change to the string?

@@ +6210,5 @@
>      return false;
>  }
>  
>  static NativeObject*
> +DebuggerSource_check(JSContext* cx, const Value& thisv, const char* fnname)

Wouldn't it be better to use HandleValue here?
Attachment #8728296 - Flags: review?(jimb) → review+
Blocks: 1255630
The previous version's filtering of sources was plain wrong. Keep a variant and
match on that; it's simpler and correcter.
Attachment #8729333 - Flags: review?(jimb)
Attachment #8728903 - Attachment is obsolete: true
Attachment #8728903 - Flags: review?(jimb)
Attachment #8728910 - Attachment is obsolete: true
Attachment #8728910 - Flags: review?(jimb)
I ended up having to add a sweep method to update the weak backpointer on
wasm::Module due to WasmModuleObjects moving due to compaction.

Could use a second look. Is adding a sweep phase the only way to fix up these
weak backpointers?
Attachment #8728297 - Attachment is obsolete: true
Attachment #8729359 - Flags: review?(terrence)
Comment on attachment 8729359 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

Review of attachment 8729359 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC, the Module -> ModuleObject edge should just be strong so that the normal heap traversal will update the edge when compacting.
Attachment #8729359 - Flags: review?(terrence)
Strong reference version. Much simpler.
Attachment #8729359 - Attachment is obsolete: true
Attachment #8729683 - Flags: review?(terrence)
Attachment #8729683 - Flags: review?(terrence) → review+
Comment on attachment 8728903 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.

Review of attachment 8728903 [details] [diff] [review]:
-----------------------------------------------------------------

Posting comments I had so far on the old version of patch. I will look at the new one.

::: js/src/vm/Debugger.h
@@ +678,5 @@
> +
> +    template <typename ReferentVariant, typename Referent, typename Map>
> +    JSObject* wrapVariantReferent(JSContext* cx, Map& map, CrossCompartmentKey::Kind keyKind,
> +                                  Handle<ReferentVariant> referent);
> +    JSObject* wrapVariantReferent(JSContext* cx, Handle<DebuggerScriptReferent> referent);

These function names (newVariantWrapper, wrapVariantReferent) are terrible. I have no idea what a "variant wrapper" is.

@@ +984,5 @@
> +     * script), synthesizing a new one if needed. The context |cx| must be in
> +     * the debugger compartment; |wasmModule| must be a WasmModuleObject in
> +     * the debuggee compartment.
> +     */
> +    JSObject* synthesizeWasmScript(JSContext* cx, Handle<WasmModuleObject*> wasmModule);

This should be named wrapWasmModule.
Comment on attachment 8729333 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.

Review of attachment 8729333 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/doc/Debugger/Debugger.Script.md
@@ +42,5 @@
>  equivalent functions or evaluated code—that is, scripts representing the
>  same source code, at the same position in the same source file,
>  evaluated in the same lexical environment.
>  
> +# Debugger.Script for WebAssembly

The number of '#' marks determines the header level, which affects the nesting of headers. By making this a top-level header, you've made all the subsequent sections children of "Debugger.Script for WebAssembly", which is wrong.

It's not adequate to simply say, at the top, "A `Debugger.Script` instance may refer to a sequence of bytecode", and then mention "these instances" six paragraphs later. You need to start at the top and say something like:

A `Debugger.Script` refers to either a sequence of JavaScript bytecode, or a block of WebAssembly code. The two cases are distinguished by the `format` property, which is either `"js"` or `"wasm"`.

For JavaScript, ...

For WebAssembly, ...

The format property must also be documented in its place along with all the other accessors.

This is not the first time I've sent back doc patches to you for being obviously inadequate, in ways that you shouldn't need a patch review to see. You have experience reading documentation; I feel like these problems should be apparent to you. I suspect that you didn't put much care into writing the documentation. If I allow patches like this to land, the documentation will become worse and worse. 

If you feel that the documentation has no audience and isn't worth maintaining, then that's a issue we can discuss with the team. Perhaps I'm overestimating the value of internal documentation of this sort. But until such a decision is reached, we should maintain a certain level of quality.

As things stand, this makes me pretty upset, because I put a lot of time into writing these docs originally, and tried to make them good.

@@ +47,5 @@
> +
> +A `Debugger.Script` instance may also refer to WebAssembly code. These
> +instances are distinguished by their `format` property being `"wasm"`.
> +
> +Currently, only the `format` and `text` properties works for WebAssembly

There is no `text` property on a Debugger.Script.

::: js/src/vm/Debugger.cpp
@@ +3927,5 @@
>                  return false;
>              }
>  
> +            hasSource = true;
> +            source = GetSourceReferent(&debuggerSource.toObject());

Yes, this is much nicer.

@@ +5066,5 @@
> +
> +        CrossCompartmentKey key(keyKind, object, untaggedReferent);
> +        if (!object->compartment()->putWrapper(cx, key, ObjectValue(*wrapper))) {
> +            map.remove(untaggedReferent);
> +            ReportOutOfMemory(cx);

Wouldn't the call to putWrapper have reported the OOM already itself?

@@ +5099,3 @@
>  
> +JSObject*
> +Debugger::synthesizeWasmScript(JSContext* cx, Handle<WasmModuleObject*> wasmModule)

This should be named wrapWasmScript.
Attachment #8729333 - Flags: review?(jimb) → review+
Attachment #8728904 - Flags: review?(jimb) → review+
Attachment #8728905 - Flags: review?(jimb) → review+
Comment on attachment 8728907 [details] [diff] [review]
Add a .format property on Debugger.Script.

Review of attachment 8728907 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +5304,5 @@
> +struct DebuggerScriptGetFormatMatcher
> +{
> +    using ReturnType = const char*;
> +    ReturnType match(HandleScript script) { return "js"; }
> +    ReturnType match(Handle<WasmModuleObject*> wasmModule) { return "wasm"; }

You could just put these in CommonPropertyNames.h; but it's fine like this too.
Attachment #8728907 - Flags: review?(jimb) → review+
Attachment #8729748 - Flags: review?(jimb)
Comment on attachment 8728908 [details] [diff] [review]
Support wasm for most of the Debugger.Source properties.

Review of attachment 8728908 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +6713,5 @@
>      args.rval().setUndefined();
>      return true;
>  }
>  
> +class DebuggerSourceGetSourceMapUrlMatcher

Is there a reason we can't have ReturnType = const char16_t *, and then leave all the string allocation logic in DebuggerSource_getSourceMapUrl?
Attachment #8728908 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #35)
> Comment on attachment 8728908 [details] [diff] [review]
> Support wasm for most of the Debugger.Source properties.
> 
> Review of attachment 8728908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Debugger.cpp
> @@ +6713,5 @@
> >      args.rval().setUndefined();
> >      return true;
> >  }
> >  
> > +class DebuggerSourceGetSourceMapUrlMatcher
> 
> Is there a reason we can't have ReturnType = const char16_t *, and then
> leave all the string allocation logic in DebuggerSource_getSourceMapUrl?

Because then we can't distinguish a nullptr between being OOM and simply not having a source map URL, for which we return null.
Attachment #8728909 - Flags: review?(jimb) → review+
(In reply to Shu-yu Guo [:shu] from comment #36)
> (In reply to Jim Blandy :jimb from comment #35)
> > Comment on attachment 8728908 [details] [diff] [review]
> > Support wasm for most of the Debugger.Source properties.
> > 
> > Review of attachment 8728908 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/vm/Debugger.cpp
> > @@ +6713,5 @@
> > >      args.rval().setUndefined();
> > >      return true;
> > >  }
> > >  
> > > +class DebuggerSourceGetSourceMapUrlMatcher
> > 
> > Is there a reason we can't have ReturnType = const char16_t *, and then
> > leave all the string allocation logic in DebuggerSource_getSourceMapUrl?
> 
> Because then we can't distinguish a nullptr between being OOM and simply not
> having a source map URL, for which we return null.

Wait, that doesn't make any sense. I'll change ReturnType = const char16_t*.
Comment on attachment 8728912 [details] [diff] [review]
Append "> wasm" to URLs of wasm Debugger.Sources to hack around blacklisting in devtools.

Review of attachment 8728912 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +6511,5 @@
>  struct DebuggerSourceGetURLMatcher
>  {
> +    JSContext* cx_;
> +    explicit DebuggerSourceGetURLMatcher(JSContext* cx) : cx_(cx) { }
> +  public:

Why the 'public:'? It's a struct.

@@ +6527,5 @@
> +        // TODOshu: Until wasm modules have real URLs, append "> wasm" to the
> +        // end to prevent them from being blacklisted by devtools by having
> +        // the same value as a source mapped URL.
> +        char buf[4096];
> +        JS_snprintf(buf, sizeof(buf), "%s > wasm", wasmModule->module().filename());

Could we use JS_smprintf and JS_smprintf_free here instead?
Attachment #8728912 - Flags: review?(jimb) → review+
Attachment #8729759 - Flags: review?(jimb)
Comment on attachment 8729334 [details] [diff] [review]
Tests for perfunctory functionality of wasm Debugger.Scripts and Debugger.Sources.

Review of attachment 8729334 [details] [diff] [review]:
-----------------------------------------------------------------

Need to check that the .format of an ordinary Debugger.Script is js.

Need to check cases of more than one Wasm module in the map; I didn't see anything that creates more than one.

Need to check that wrapping the same Wasm module twice gets the identical Debugger.Script; I guess doing two findScripts calls is the only way to make this happen.

Need to check that findScript; compacting GC; findScript gives you same Wasm module wrapper (i.e. that your weakmaps are getting correctly updated for object relocations)

::: js/src/jit-test/tests/debug/wasm-01.js
@@ +14,5 @@
> +    break;
> +  }
> +}
> +
> +print(dbg.findScripts().length);

This can probably come out, right?
Attachment #8729334 - Flags: review?(jimb) → review+
Comment on attachment 8729334 [details] [diff] [review]
Tests for perfunctory functionality of wasm Debugger.Scripts and Debugger.Sources.

Review of attachment 8729334 [details] [diff] [review]:
-----------------------------------------------------------------

Test that onNewScript reports a distinct wrapper for each new Wasm module.
Attachment #8729748 - Flags: review?(jimb) → review+
Attachment #8729759 - Flags: review?(jimb) → review+
Attachment #8729812 - Flags: review?(shu)
Comment on attachment 8729812 [details] [diff] [review]
cross-the-streams

Review of attachment 8729812 [details] [diff] [review]:
-----------------------------------------------------------------

The usability issue with having to reload is a bummer, but fine by me since this is all experimental.

::: js/src/asmjs/Wasm.cpp
@@ +1600,5 @@
>  
>      if (!CreateInstance(cx, exportObj, instance))
>          return false;
>  
> +    if (cx->compartment()->debuggerObservesAsmJS()) {

This use is fine by me for the demo, but I'm not sure how this flag should interact with wasm yet. This flag being on *prevents* asm.js compilation at all, so firstly, logically I'm not sure we should reach this point if the flag is meant to behave analogously for wasm.

Secondly, I'm all about the debugger working seamlessly whether it was opened after the fact or before. I hope we eventually can generate text regardless of when the debugger was opened (by refetching the code, maybe).
Attachment #8729812 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #46)
> This flag being on *prevents* asm.js compilation at
> all, so firstly, logically I'm not sure we should reach this point if the
> flag is meant to behave analogously for wasm.

With asm.js we can fall back to JS.  Not so for wasm.  Actually we have an improvement here over asm.js: with debugger open, wasm games run at full speed.

> Secondly, I'm all about the debugger working seamlessly whether it was
> opened after the fact or before. I hope we eventually can generate text
> regardless of when the debugger was opened (by refetching the code, maybe).

Yes, agreed we should improve here.  I like the idea of (1) always save bits when "names" (i.e. symbols) section is present and (2) if not, attempt to fetch bits from the network (though that requires es6 module integration).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: