[jsdbg2] Remove the shell's evalInFrame

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

5 years ago
No doubt.
Assignee

Comment 1

5 years ago
Attachment #8484742 - Flags: review?(jimb)
Nice.
Assignee

Comment 3

5 years ago
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.
Attachment #8484742 - Flags: review?(jimb)
Assignee

Comment 4

5 years ago
Attachment #8484742 - Attachment is obsolete: true
Attachment #8484797 - Flags: review?(jimb)
Assignee

Updated

5 years ago
Depends on: 1063328, 1032869, 1056411
Assignee

Updated

5 years ago
Assignee: nobody → shu
Status: NEW → ASSIGNED
Assignee

Comment 5

5 years ago
Forgot to commit Jim's IRC suggestion over using the gross (1,eval)('this')
Attachment #8484797 - Attachment is obsolete: true
Attachment #8484797 - Flags: review?(jimb)
Attachment #8485328 - Flags: review?(jimb)

Comment 6

5 years ago
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?

Updated

5 years ago
Attachment #8485328 - Flags: review?(jimb) → review+
Assignee

Comment 7

5 years ago
(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.
Btw, you could now remove EvaluateInEnv from Debugger.h and inline EvaluateInEnv into its (now one) caller.
https://hg.mozilla.org/mozilla-central/rev/f8e316fa65bb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.