Closed Bug 1056411 Opened 5 years ago Closed 5 years ago

[jsdbg2] Remove C JSAPI DebugMode functions and DebugFromC

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(2 files, 1 obsolete file)

jsd1 is dead, long live jsd1.
Blocks: 1032869
Attachment #8476378 - Flags: review?(jimb)
Attachment #8476378 - Attachment is obsolete: true
Attachment #8476378 - Flags: review?(jimb)
Attachment #8476971 - Flags: review?(jimb)
Blocks: 1063330
Duplicate of this bug: 1063296
Depends on: 800200
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.)
Attachment #8476972 - Flags: review?(jimb)
Attachment #8476972 - Flags: review+
Blocks: 1064079
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,
Attachment #8476971 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/f349b4cc6297
https://hg.mozilla.org/mozilla-central/rev/6cc2af2e51fb
Assignee: nobody → shu
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.