We would like to be able to use Debugger for applications that need asm.js's special code generation, and are able to tolerate the inconsistencies between the source code and the state presented by Debugger that asm.js introduces. There should be an accessor, Debugger.prototype.allowUnobservedAsmJS, false by default, which determines whether future compilations in debuggee globals may apply optimizations to asm.js code that hide information from Debugger. Note that asm.js optimizations are not presently transparent to Debugger even without this new accessor. This flag is only meant to permit additional non-transparency when set, not to forbid the extant non-transparency when cleared.

This sounds great. So when the console is opened, we create a `Debugger` object and set `allowUnobservedAsmJS` to true, and then debugger is opened, we simply flip that flag to `false`? That sounds wonderfully simple.

How hard would this be? Is it possible in Q1?

Does this also mean we can enable debugging so that we got a notification when a `debugger` statement is hit, and throw the user into the debugger?

Tests forthcoming.

Comment on attachment 8562576 [details] [diff] [review] Add a .allowUnobservedAsmJS accessor on Debugger instances. Review of attachment 8562576 [details] [diff] [review]: ----------------------------------------------------------------- FWIW, if we write a baseline asm.js compiler (already a goal b/c it would allow a faster initial startup (while full Ion AOT compilation proceeded in the background) while staying (hopefully) within 4x native), then I think we can make it a design goal to make it work with the debugger and thus we could avoid all these exceptions and make things Just Work (albeit at a small factor slowdown, but way better than the general Baseline JIT). ::: js/src/asmjs/AsmJSValidate.cpp @@ +9232,5 @@ > > if (!parser.options().compileAndGo) > return Warn(parser, JSMSG_USE_ASM_TYPE_FAIL, "Temporarily disabled for event-handler and other cloneable scripts"); > > + if (!cx->compartment()->debugObservesAsmJS()) This condition reads backwards: if the debugger observes asm.js, then we should disable Odin. I don't know if this is a bug or just a naming thing. How about debuggerInhibitsAsmJS()?

Comment on attachment 8562576 [details] [diff] [review] Add a .allowUnobservedAsmJS accessor on Debugger instances. Review of attachment 8562576 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/AsmJSValidate.cpp @@ +9232,5 @@ > > if (!parser.options().compileAndGo) > return Warn(parser, JSMSG_USE_ASM_TYPE_FAIL, "Temporarily disabled for event-handler and other cloneable scripts"); > > + if (!cx->compartment()->debugObservesAsmJS()) Ah, that's a bug. :) Should've been |if (cx->compartment()->debugObservesAsmJS())| It's also named that way for consistency with debugObservesAllExecution. Your name is a fine name, but I'm going to keep the current one for similarity elsewhere in the code.

Ah, good; at least the names mean what I expected :) I guess the reason jit-tests didn't catch this is because debugObservesAsmJS() is part of isAsmJSCompilationAvailable() (which is *supposed* to only return false when the underlying hardware prevents Odin (no hwfp, weird page size)), so could you just remove debugObservesAsmJS() from isAsmJSCompilationAvailable?

Can you all possibly answer my questions in comment 1 and comment 3? Just want to make sure I understand this right.

(In reply to James Long (:jlongster) from comment #1) > This sounds great. So when the console is opened, we create a `Debugger` > object and set `allowUnobservedAsmJS` to true, and then debugger is opened, > we simply flip that flag to `false`? > > That sounds wonderfully simple. I would prefer that the console's Debugger instance sets 'allowUnobservedAsmJS' to true, and when switching away, calling removeDebuggee on the content global. The debugger tab's Debugger instance can leave 'allowUnobservedAsmJS' unchanged from its default, false. (In reply to James Long (:jlongster) from comment #3) > Does this also mean we can enable debugging so that we got a notification > when a `debugger` statement is hit, and throw the user into the debugger? Making globals debuggees still isn't free. I don't want to penalize the layout debugging use cases or other use cases that don't touch JS at all. So like I said, I still wouldn't do this for every tool in devtools. Either restrict the |debugger;| trapping to some tools, or make it a pref.

(In reply to Shu-yu Guo [:shu] from comment #10) > > (In reply to James Long (:jlongster) from comment #3) > > Does this also mean we can enable debugging so that we got a notification > > when a `debugger` statement is hit, and throw the user into the debugger? > > Making globals debuggees still isn't free. I don't want to penalize the > layout debugging use cases or other use cases that don't touch JS at all. So > like I said, I still wouldn't do this for every tool in devtools. Either > restrict the |debugger;| trapping to some tools, or make it a pref. I thought that was our goal. It's super weird that our devtools behavior depends on whether or not the "debugger" tab has been clicked or not. As a developer, I just don't want to think about it. Still, even if we just do that in the console, we're going to have to use the debugger's Debugger object. When a `debugger` statement is hit we need to throw the user into the debugger, paused at that point.

Comment on attachment 8563071 [details] [diff] [review] Add an .allowUnobservedAsmJS accessor on Debugger instances. Thanks

Comment on attachment 8563071 [details] [diff] [review] Add an .allowUnobservedAsmJS accessor on Debugger instances. Review of attachment 8563071 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, I think it would simplify the management of the compartment's observation flags a lot if we defined a new JSCompartment member function, updateObservedFlags, that simply scans all the compartment's debuggers, checks what they observe, and sets the compartment's debugModeBits accordingly. This would replace Debugger::anyOtherDebuggerObservingAllExecution, make the addition of ObservableExecutionKind unnecessary, and (I think?) simplify the code that currently tries to keep those bits set correctly. ::: js/src/doc/Debugger/Debugger.md @@ +26,5 @@ > events or handlers or "points" we add to the interface. > > +`allowUnobservedAsmJS` > +: A boolean value indicating whether this `Debugger` instance's debuggee > + globals allows asm.js functions are invisible to this instance's handlers, This sentence doesn't parse. Also, should asm.js be in `code quotes`? @@ +29,5 @@ > +: A boolean value indicating whether this `Debugger` instance's debuggee > + globals allows asm.js functions are invisible to this instance's handlers, > + and breakpoints. In other words, setting this to `false` inhibits the > + ahead-of-time asm.js compiler and forces asm.js code to run as normal > + JavaScript. This is an cacessor property with a getter and setter. It is "accessor" @@ +33,5 @@ > + JavaScript. This is an cacessor property with a getter and setter. It is > + initially `false` in a freshly created `Debugger` instance. > + > + This flag is intended to be used when one wishes to use the subsystems of > + the Debugger API (e.g, [`Debugger.Source`]) for other purposes than step I think you need to say: [`Debugger.Source`][source] and "for purposes other" ::: js/src/jscompartment.h @@ +475,5 @@ > debugModeBits &= ~DebugObservesAllExecution; > } > > + // True if this compartment's global is a debuggee of some Debugger object > + // whose allowUnobservedAsmJS is true. missing "flag" ::: js/src/vm/Debugger.cpp @@ +2020,5 @@ > } > > /* static */ bool > +Debugger::anyOtherDebuggerObservingExecution(GlobalObject *global, > + ObservableExecutionKind kind) const While we're here, could you drop the 'static' marker on this? It's not a static method. @@ +2628,5 @@ > + > +/* static */ bool > +Debugger::setAllowUnobservedAsmJS(JSContext *cx, unsigned argc, Value *vp) > +{ > + THIS_DEBUGGER(cx, argc, vp, "get allowUnobservedAsmJS", args, dbg); "set"

This patch also renames the debugModeBits flags to be prefixed by Debugger instead of Debug. I had meant to separate out the renaming to a separate patch, but I accidentally squashed the patches. Sorry, Jim.

(In reply to James Long (:jlongster) from comment #11) > I thought that was our goal. It's super weird that our devtools behavior > depends on whether or not the "debugger" tab has been clicked or not. As a > developer, I just don't want to think about it. FWIW, I find the current behavior highly valuable. I frequently use the console to see log messages but wouldn't want to have exceptions or `debugger` statements pause execution. Yes, the fact that there's a permanent mode switch caused by opening the debugger panel once is weird, but it's also not entirely useless.

(In reply to Till Schneidereit [:till] from comment #15) > (In reply to James Long (:jlongster) from comment #11) > > I thought that was our goal. It's super weird that our devtools behavior > > depends on whether or not the "debugger" tab has been clicked or not. As a > > developer, I just don't want to think about it. > > FWIW, I find the current behavior highly valuable. I frequently use the > console to see log messages but wouldn't want to have exceptions or > `debugger` statements pause execution. Yes, the fact that there's a > permanent mode switch caused by opening the debugger panel once is weird, > but it's also not entirely useless. By default, exceptions don't pause the debugger. Usually people use turn that on when they really want it, and turn it off when they are done. `debugger` statements are similar, added in the code when you really do want to stop. If you really want that behavior, but you accidentally already opened the debugger, whoops it doesn't work. You have to close and re-open the devtools. It's super weird. I think we need to turn on pausing by default and look into if we really want to add an option to ignore it. Like I said it usually shouldn't pause (exceptions don't pause by default). Anyway, maybe we can take this up somewhere else since it's not directly related to the work here.

Forgot to address comments about docs.

Comment on attachment 8563591 [details] [diff] [review] Add an .allowUnobservedAsmJS accessor on Debugger instances. Review of attachment 8563591 [details] [diff] [review]: ----------------------------------------------------------------- Yay. ::: js/src/asmjs/AsmJSValidate.cpp @@ -9395,5 @@ > bool available = false; > #else > bool available = cx->jitSupportsFloatingPoint() && > cx->gcSystemPageSize() == AsmJSPageSize && > - !cx->compartment()->isDebuggee() && It seems like this change isn't quite behavior-preserving, but since it's only used in tests, and makes things expect asm.js more often, and there are no failures, it must be fine. ::: js/src/jscompartment.h @@ +470,4 @@ > } > + > + // True if this compartment's global is a debuggee of some Debugger object > + // whose allowUnobservedAsmJS flag is true. "is false", right?