1063330 - [jsdbg2] Remove the shell's evalInFrame

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

No doubt.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Remove the JS shell's evalInFrame.

Comment 2

[Luke Wagner [:luke]]

Nice.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8484742 [details] [diff] [review]
Remove the JS shell's evalInFrame.

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ -2224,5 @@
>  
> -    ZCREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("debuggees-set"),
> -        cStats.debuggeesSet,
> -        "The debuggees set.");
> -

Oops, looks like I committed this hunk to the wrong patch.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Remove the JS shell's evalInFrame.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Remove the JS shell's evalInFrame.

Forgot to commit Jim's IRC suggestion over using the gross (1,eval)('this')

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8485328 [details] [diff] [review]
Remove the JS shell's evalInFrame.

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

The Xdr.h change looks wrong. I had a few other questions, too. r=me with those addressed.

::: js/src/jit-test/lib/evalInFrame.js
@@ +3,5 @@
> +  return function evalInFrame(upCount, code) {
> +    if (!dbg) {
> +      var dbgGlobal = newGlobal();
> +      dbg = new dbgGlobal.Debugger();
> +    }

A later comment notes that loading this file might be expected to put the current global in debug mode immediately. If that's so, then we could simply create the Debugger in the outer anonymous function, not the first time evalInFrame is called.

(It's so easy to defeat the self-debugging logic. *sigh*)

::: js/src/jit-test/tests/auto-regress/bug765483.js
@@ +3,5 @@
>  // Binary: cache/js-dbg-64-de23a9fc29db-linux
>  // Flags: --ion-eager
>  //
>  
> +load(libdir + "evalInFrame.js");

Loading evalInFrame.js doesn't put the compartment in debug mode until the first time evalInFrame gets called, so this doesn't quite preserve the behavior of the original test.

::: js/src/jit-test/tests/basic/eif-generator.js
@@ -1,4 @@
> -var global = newGlobal();
> -var dbg = new global.Debugger(this);
> -// Force dbg to observe all execution.
> -dbg.onDebuggerStatement = function () {};

Is it really okay to drop this? (Here and elsewhere.)

::: js/src/vm/Xdr.h
@@ -27,5 @@
>   * this wiki page:
>   *
>   *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
>   */
> -static const uint32_t XDR_BYTECODE_VERSION = uint32_t(0xb973c0de - 182);

Why is this necessary?

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #6)
> Comment on attachment 8485328 [details] [diff] [review]
> Remove the JS shell's evalInFrame.
> 
> Review of attachment 8485328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The Xdr.h change looks wrong. I had a few other questions, too. r=me with
> those addressed.
> 
> ::: js/src/jit-test/lib/evalInFrame.js
> @@ +3,5 @@
> > +  return function evalInFrame(upCount, code) {
> > +    if (!dbg) {
> > +      var dbgGlobal = newGlobal();
> > +      dbg = new dbgGlobal.Debugger();
> > +    }
> 
> A later comment notes that loading this file might be expected to put the
> current global in debug mode immediately. If that's so, then we could simply
> create the Debugger in the outer anonymous function, not the first time
> evalInFrame is called.
> 
> (It's so easy to defeat the self-debugging logic. *sigh*)
> 
> ::: js/src/jit-test/tests/auto-regress/bug765483.js
> @@ +3,5 @@
> >  // Binary: cache/js-dbg-64-de23a9fc29db-linux
> >  // Flags: --ion-eager
> >  //
> >  
> > +load(libdir + "evalInFrame.js");
> 
> Loading evalInFrame.js doesn't put the compartment in debug mode until the
> first time evalInFrame gets called, so this doesn't quite preserve the
> behavior of the original test.
> 

Hm, yeah, might as well eagerly put the global in debug mode.

> ::: js/src/jit-test/tests/basic/eif-generator.js
> @@ -1,4 @@
> > -var global = newGlobal();
> > -var dbg = new global.Debugger(this);
> > -// Force dbg to observe all execution.
> > -dbg.onDebuggerStatement = function () {};
> 
> Is it really okay to drop this? (Here and elsewhere.)
> 

Yes. I marked this bug as dependent on bug 1032869. When D.S objects are created the frames are deoptimized.

> ::: js/src/vm/Xdr.h
> @@ -27,5 @@
> >   * this wiki page:
> >   *
> >   *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
> >   */
> > -static const uint32_t XDR_BYTECODE_VERSION = uint32_t(0xb973c0de - 182);
> 
> Why is this necessary?

Hm, you're right, it isn't. I was blindly changing it because I removed a message in js.msg. In general, when that happens, since the new js.msg doesn't have stable error numbers, the bytecode version needs to be bumped since the message numbers might be baked into self-hosted code. For this patch, I don't think a version bump is needed.

Comment 8

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e316fa65bb

Comment 9

[Luke Wagner [:luke]]

Btw, you could now remove EvaluateInEnv from Debugger.h and inline EvaluateInEnv into its (now one) caller.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8e316fa65bb