494645 - option for shell dis() to disassemble recursively

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: v1

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

Comment 2

[Igor Bukanov]

(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?

Comment 3

[Igor Bukanov]

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.

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Brendan Eich [:brendan]]

(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

Comment 6

[Igor Bukanov]

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

Yes, it is a complication for the bug 494235.

Comment 7

[Jim Blandy :jimb]

(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 8

[Blake Kaplan (:mrbkap) (inactive)]

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).

Comment 9

[Brendan Eich [:brendan]]

(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

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/265f98384b1f

Comment 11

[Igor Bukanov]

I forgot to include the TM repo link for this bug when landing it there:

http://hg.mozilla.org/tracemonkey/rev/265f98384b1f