The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(14 attachments, 8 obsolete attachments)

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
(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1232014
Depends on: 1254453
(Assignee)

Comment 1

a year ago
Created attachment 8728295 [details] [diff] [review]
Prep Debugger.Script for a tagged union referent.
Attachment #8728295 - Flags: review?(jimb)
(Assignee)

Comment 2

a year ago
Created attachment 8728296 [details] [diff] [review]
Prep Debugger.Source for a tagged union referent.
Attachment #8728296 - Flags: review?(jimb)
(Assignee)

Comment 3

a year ago
Created attachment 8728297 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.
Attachment #8728297 - Flags: review?(terrence)
Attachment #8728297 - Flags: feedback?(luke)
(Assignee)

Comment 4

a year ago
Created attachment 8728298 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.
Attachment #8728298 - Flags: review?(jimb)
(Assignee)

Comment 5

a year ago
Created attachment 8728299 [details] [diff] [review]
Synthesize Debugger.Sources for wasm modules.
Attachment #8728299 - Flags: review?(jimb)
(Assignee)

Comment 6

a year ago
Created attachment 8728300 [details] [diff] [review]
Display placeholder text for synthesized Debugger.Sources.
Attachment #8728300 - Flags: review?(jimb)
(Assignee)

Comment 7

a year ago
Created attachment 8728301 [details] [diff] [review]
Add a .format property on Debugger.Script and Debugger.Source.
Attachment #8728301 - Flags: review?(jimb)
(Assignee)

Comment 8

a year ago
I'll write tests tomorrow. I am tired now.
(Assignee)

Comment 9

a year ago
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)

Updated

a year ago
Assignee: nobody → shu
Status: NEW → ASSIGNED

Comment 10

a year ago
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+

Comment 11

a year ago
(In reply to Shu-yu Guo [:shu] from comment #9)
> Example output right now:

Oh, I love it.
See Also: → bug 1254984
Depends on: 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.
(Assignee)

Comment 14

a year ago
(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+
(Assignee)

Comment 17

a year ago
Created attachment 8728903 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.
Attachment #8728298 - Attachment is obsolete: true
Attachment #8728298 - Flags: review?(jimb)
Attachment #8728903 - Flags: review?(jimb)
(Assignee)

Comment 18

a year ago
Created attachment 8728904 [details] [diff] [review]
Synthesize Debugger.Sources for wasm modules.
Attachment #8728299 - Attachment is obsolete: true
Attachment #8728299 - Flags: review?(jimb)
Attachment #8728904 - Flags: review?(jimb)
(Assignee)

Comment 19

a year ago
Created attachment 8728905 [details] [diff] [review]
Display placeholder text for synthesized Debugger.Sources.
Attachment #8728300 - Attachment is obsolete: true
Attachment #8728300 - Flags: review?(jimb)
Attachment #8728905 - Flags: review?(jimb)
(Assignee)

Comment 20

a year ago
Created attachment 8728907 [details] [diff] [review]
Add a .format property on Debugger.Script.

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)
(Assignee)

Comment 21

a year ago
Created attachment 8728908 [details] [diff] [review]
Support wasm for most of the Debugger.Source properties.
Attachment #8728908 - Flags: review?(jimb)
(Assignee)

Comment 22

a year ago
Created attachment 8728909 [details] [diff] [review]
Fire onNewScript for new wasm modules.
Attachment #8728909 - Flags: review?(jimb)
(Assignee)

Comment 23

a year ago
Created attachment 8728910 [details] [diff] [review]
Perfunctory tests.

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)
(Assignee)

Comment 24

a year ago
Created attachment 8728912 [details] [diff] [review]
Append "> wasm" to URLs of wasm Debugger.Sources to hack around blacklisting in devtools.

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 25

a year ago
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+
(Assignee)

Updated

a year ago
Blocks: 1255630
(Assignee)

Comment 26

a year ago
Created attachment 8729333 [details] [diff] [review]
Synthesize Debugger.Scripts for wasm modules and find them via findScripts.

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)
(Assignee)

Updated

a year ago
Attachment #8728903 - Attachment is obsolete: true
Attachment #8728903 - Flags: review?(jimb)
(Assignee)

Comment 27

a year ago
Created attachment 8729334 [details] [diff] [review]
Tests for perfunctory functionality of wasm Debugger.Scripts and Debugger.Sources.
Attachment #8729334 - Flags: review?(jimb)
(Assignee)

Updated

a year ago
Attachment #8728910 - Attachment is obsolete: true
Attachment #8728910 - Flags: review?(jimb)
(Assignee)

Comment 28

a year ago
Created attachment 8729359 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

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)
(Assignee)

Comment 30

a year ago
Created attachment 8729683 [details] [diff] [review]
Keep a list of wasm::Modules per compartment.

Strong reference version. Much simpler.
Attachment #8729359 - Attachment is obsolete: true
Attachment #8729683 - Flags: review?(terrence)
Attachment #8729683 - Flags: review?(terrence) → review+

Comment 31

a year ago
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 32

a year ago
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+

Updated

a year ago
Attachment #8728904 - Flags: review?(jimb) → review+

Updated

a year ago
Attachment #8728905 - Flags: review?(jimb) → review+

Comment 33

a year ago
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+
(Assignee)

Comment 34

a year ago
Created attachment 8729748 [details] [diff] [review]
Update Debugger.Script docs.
Attachment #8729748 - Flags: review?(jimb)

Comment 35

a year ago
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+
(Assignee)

Comment 36

a year ago
(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.

Updated

a year ago
Attachment #8728909 - Flags: review?(jimb) → review+
(Assignee)

Comment 37

a year ago
(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 38

a year ago
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+
(Assignee)

Comment 39

a year ago
Created attachment 8729759 [details] [diff] [review]
Update Debugger.Source docs.
Attachment #8729759 - Flags: review?(jimb)

Comment 40

a year ago
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 41

a year ago
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.

Updated

a year ago
Attachment #8729748 - Flags: review?(jimb) → review+

Updated

a year ago
Attachment #8729759 - Flags: review?(jimb) → review+

Comment 42

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae46c02868d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab08650fe473
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a5f0786a8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d769eeda1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/937999c2a3f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/134ae6e2f92e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f257362ae80d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ecce85ac43
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0831a7ef13
https://hg.mozilla.org/integration/mozilla-inbound/rev/3300390349c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac34b44f276
https://hg.mozilla.org/integration/mozilla-inbound/rev/293d196105f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf4a26491ea5

Comment 43

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2bed5822e5

Comment 44

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a44b86faaa
Created attachment 8729812 [details] [diff] [review]
cross-the-streams
Attachment #8729812 - Flags: review?(shu)
(Assignee)

Comment 46

a year ago
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+

Comment 47

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5dd6053cc4
(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).

Comment 49

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/940ebacc15d2

Comment 50

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ae46c02868d
https://hg.mozilla.org/mozilla-central/rev/ab08650fe473
https://hg.mozilla.org/mozilla-central/rev/f9a5f0786a8b
https://hg.mozilla.org/mozilla-central/rev/67d769eeda1f
https://hg.mozilla.org/mozilla-central/rev/937999c2a3f8
https://hg.mozilla.org/mozilla-central/rev/134ae6e2f92e
https://hg.mozilla.org/mozilla-central/rev/f257362ae80d
https://hg.mozilla.org/mozilla-central/rev/c1ecce85ac43
https://hg.mozilla.org/mozilla-central/rev/eb0831a7ef13
https://hg.mozilla.org/mozilla-central/rev/3300390349c6
https://hg.mozilla.org/mozilla-central/rev/9ac34b44f276
https://hg.mozilla.org/mozilla-central/rev/293d196105f1
https://hg.mozilla.org/mozilla-central/rev/bf4a26491ea5
https://hg.mozilla.org/mozilla-central/rev/ec2bed5822e5
https://hg.mozilla.org/mozilla-central/rev/a3a44b86faaa
https://hg.mozilla.org/mozilla-central/rev/ba5dd6053cc4
https://hg.mozilla.org/mozilla-central/rev/940ebacc15d2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1257045

Comment 51

5 months ago
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/584c27117cd6
Forgot to commit test.

Comment 52

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/584c27117cd6
You need to log in before you can comment on or make changes to this bug.