1056411 - [jsdbg2] Remove C JSAPI DebugMode functions and DebugFromC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

jsd1 is dead, long live jsd1.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Remove DebugFromC and related C API and shell features.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Part 1: Remove DebugFromC and clean up compartment debug mode logic.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Part 2: Remove the JSAPI C debug mode functions.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 8476972 [details] [diff] [review]
Part 2: Remove the JSAPI C debug mode functions.

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

Thanks for removing the |jit-flags| debug from all those jit-test/tests/debug tests --- those aren't necessary, and probably weaken our coverage of debug mode switching.

::: js/src/tests/lib/jittests.py
@@ +136,5 @@
>                              test.jitflags.append('--thread-count=' + int(value, 0));
>                          except ValueError:
>                              print("warning: couldn't parse thread-count %s" % value)
>                      else:
> +                        print('%s: warning: unrecognized |jit-test| attribute %s' % (path, part))

(I laughed out loud when I saw this patch hunk. I know the feeling.)

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8476971 [details] [diff] [review]
Part 1: Remove DebugFromC and clean up compartment debug mode logic.

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

This looks great. I appreciate the removal of the useless invalidate arguments, and the renaming of the JSCompartment methods to reflect the fact that being in debug mode is now equivalent to having the compartment's global be a debuggee.

::: js/src/jscompartment.h
@@ +386,3 @@
>    public:
> +    // True if at least one Debugger object is attached to the global
> +    // associated with this compartment.

"Attached" isn't really terminology we use. I'd say:

True if this compartment's global is a debuggee of some Debugger object.

::: js/src/vm/Debugger.cpp
@@ +1173,5 @@
>  Debugger::slowPathOnNewScript(JSContext *cx, HandleScript script, GlobalObject *compileAndGoGlobal_)
>  {
>      Rooted<GlobalObject*> compileAndGoGlobal(cx, compileAndGoGlobal_);
>  
> +    JS_ASSERT(script->compileAndGo() == !!compileAndGoGlobal_);

I think it would be better to use compileAndGoGlobal (the Rooted) throughout, instead of compileAndGoGlobal_ (the argument), if only because it's easier to reason about if we know that the '_' is dead, and the Rooted is our official copy.

@@ +1179,5 @@
>      /*
>       * Build the list of recipients. For compile-and-go scripts, this is the
>       * same as the generic Debugger::dispatchHook code, but non-compile-and-go
>       * scripts are not tied to particular globals. We deliver them to every
>       * debugger observing any global in the script's compartment.

This comment seems like it should be simplified. I have to admit, since non-compile-and-go scripts still can only be used with their compartment's global, I'm not sure what what this should say. But it certainly shouldn't talk about "any global in the script's compartment", so it seems like the two cases are the same.

@@ +1186,5 @@
> +    Rooted<GlobalObject *> global(cx, (script->compileAndGo()
> +                                       ? compileAndGoGlobal_
> +                                       : script->compartment()->maybeGlobal()));
> +    if (GlobalObject::DebuggerVector *debuggers = global->getDebuggers()) {
> +        if (!AddNewScriptRecipients(debuggers, script, &triggered))

AddNewScriptRecipients can be simplified now, too; I submitted bug 1064079, blocked by this.

@@ +2386,5 @@
>                                               GlobalObjectSet::Enum *debugEnum)
>  {
>      /*
> +     * Each debuggee is a HashSet in its debugger (this).
> +     * The caller might be enumerating it; if so,

I think both these lines should be replaced by:

The caller might have found global by enumerating this->debuggees; if so,

Comment 7

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f349b4cc6297
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc2af2e51fb

Comment 8

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/f349b4cc6297
https://hg.mozilla.org/mozilla-central/rev/6cc2af2e51fb