Closed Bug 494645 Opened 12 years ago Closed 12 years ago

option for shell dis() to disassemble recursively

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

Currently dis() in js shell does not have an option to disassemble the nested function embedded in a function or script. This prevents using the function to check the source of optimized non-escaping closures as dis(closure) by definition makes such closure to escape.
Attached patch v1Splinter Review
To follow with the current tradition in dis() that interprets the "-l" argument to signal to include the line numbers that patch adds support for "-r" to mean disassemble recursively. The following demonstrates "-r" utility:

@watson~/m/tm/js/src> cat ~/s/x.js
function f(x)
{   
    ++x;
    function h() {
        function i() {
            return x;
        }
        i();
    }
    h();
}

dis("-r", f);
@watson~/m/tm/js/src> ~/build/js32.tm.dbg/js ~/s/x.js
flags: NULL_CLOSURE
main:
00000:  deflocalfun 0 function h() {
function i() {return x;}

i();}
00005:  incarg 0
00008:  pop
00009:  nop
00010:  calllocal 0
00013:  call 0
00016:  pop
00017:  stop

Source notes:
  0:     0 [   0] newline
  1:     0 [   0] setline  lineno 4
  3:     5 [   5] setline  lineno 3
  5:     9 [   4] newline
  6:     9 [   0] funcdef  function 0 (function h() {
function i() {return x;}

i();})
  8:    10 [   1] setline  lineno 10
 10:    13 [   3] pcbase   offset 3
 12:    17 [   4] setline  lineno 1

flags: NULL_CLOSURE
main:
00000:  deflocalfun 0 function i() {return x;}
00005:  nop
00006:  calllocal 0
00009:  call 0
00012:  pop
00013:  stop

Source notes:
  0:     0 [   0] newline
  1:     5 [   5] funcdef  function 0 (function i() {return x;})
  3:     6 [   1] setline  lineno 8
  5:     9 [   3] pcbase   offset 3

flags: NULL_CLOSURE
main:
00000:  getupvar 0
00003:  return
00004:  stop

Source notes:
  0:     0 [   0] newline
Attachment #379386 - Flags: review?(mrbkap)
(In reply to comment #1)
...
> flags: NULL_CLOSURE
> main:
> 00000:  getupvar 0


Hm, this contradicts the documentation in jsfun.h that states, http://hg.mozilla.org/tracemonkey/file/0463edf878f6/js/src/jsfun.h#l103 :

 FUN_NULL_CLOSURE implies FUN_INTERPRETED and u.i.script->upvarsOffset == 0.

The above disassembly contradicts this for the "i" function from

function f(x)
{   
    ++x;
    function h() {
        function i() {
            return x;
        }
        i();
    }
    h();
}

To Brendan: is it a bug in documentation?
Another problem from that example from the comment 0 is that the nested function h is also marked as a null closure despite that it includes a closure that refers to the variable from the parent of h.
Oops, the comment is wrong. NULL_CLOSURE means that only the global object is needed on the scope chain when the function is invoked. Either there are zero upvars or they're all handled by JSOP_{GET,CALL}UPVAR. I'll r+ a fix or revise the comment myself if you prefer. Thanks,

/be
(In reply to comment #3)
> Another problem from that example from the comment 0 is that the nested
> function h is also marked as a null closure despite that it includes a closure
> that refers to the variable from the parent of h.

This is not a problem. Did you mean a problem for the DISPLAYCALL op proposal? That is true when considering the debugger eval'ing a call to a closure in a context the compiler did not see, but DISPLAYCALL still seems worth doing to minimize display update costs.

/be
(In reply to comment #5)
> Did you mean a problem for the DISPLAYCALL op proposal?

Yes, it is a complication for the bug 494235.
(In reply to comment #0)
> Currently dis() in js shell does not have an option to disassemble the nested
> function embedded in a function or script.

Hurrah to Igor for fixing this!
Comment on attachment 379386 [details] [diff] [review]
v1

>+static JSBool
>+Disassemble(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+    bool lines = false, recursive = false;
>+
>+    while (argc > 0 && JSVAL_IS_STRING(argv[0])) {
>+        const char *bytes = JS_GetStringBytes(JSVAL_TO_STRING(argv[0]));
>+        if (!bytes)
>+            return false;

JS_GetStringBytes can't fail. Also, you have JSBool/bool confusion here. I think for JSNatives, this needs to be JS_FALSE (here and below).
Attachment #379386 - Flags: review?(mrbkap) → review+
(In reply to comment #8)
> (From update of attachment 379386 [details] [diff] [review])
> >+static JSBool
> >+Disassemble(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> >+{
> >+    bool lines = false, recursive = false;
> >+
> >+    while (argc > 0 && JSVAL_IS_STRING(argv[0])) {
> >+        const char *bytes = JS_GetStringBytes(JSVAL_TO_STRING(argv[0]));
> >+        if (!bytes)
> >+            return false;
> 
> JS_GetStringBytes can't fail. Also, you have JSBool/bool confusion here. I
> think for JSNatives, this needs to be JS_FALSE (here and below).

No, false and true are fine JSBool values. The JSBool return value is required for various API and (for FASTCALL traceable natives, ABI) reasons. But where we can use a new broom to sweep clean, we should use true and false consistently.

/be
http://hg.mozilla.org/mozilla-central/rev/265f98384b1f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I forgot to include the TM repo link for this bug when landing it there:

http://hg.mozilla.org/tracemonkey/rev/265f98384b1f
Whiteboard: fixed-in-tracemonkey
You need to log in before you can comment on or make changes to this bug.