675694 - Implement a way to dump parse trees from within C/C++

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eddy Bruel [:ejpbruel]]

Currently, we have a way to dump parse trees using Reflect.parse from within JavaScript. For debugging purposes, it would be very convenient if we could also dump parse trees from within C/C++.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: First proposal

Here's my first proposal. I've deliberately avoided the use of Vector; in my experience it usually a good idea to avoid allocating memory in debug code (I am aware that Vector is able to use inline storage, but this won't save you in the case of ListNodes). The current design is also more flexible if we want to customize the output for certain node types, but not for others.

I've temporarily added DumpParseTree to the parse() function in the shell, so you can test this code if you'd like. As an example, the following script:

parse("function f(x) { return x + 1; } f(x);")

produces the following output:

(nop (nop (nop (getarg ())
               (nop (nop (add (name)
                              (double))))))
     (nop (call (name)
                (name))))

(Note that the formatting used mimicks that of pretty-print formatting in LISP)

One thing that struck me is that most nodes have nop as their opcode name, which isn't very informative. Maybe we should print the token type instead. Another observation is that when a name node is not used, pn_name cannot be accessed. Why is this the case, and can we do better here?

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 573498 [details] [diff] [review]
First proposal

(In reply to Eddy Bruel [:ejpbruel] from comment #1)
> Here's my first proposal. I've deliberately avoided the use of Vector; in my
> experience it usually a good idea to avoid allocating memory in debug code
> [...]

I used to think that but I’ve never had debug code actually go wrong because of heap allocation. (I have however had it go wrong for any number of other reasons.)

But yeah, avoiding Vector in this code seems fine.

> I've temporarily added DumpParseTree to the parse() function in the shell,
> so you can test this code if you'd like.

Great. I like it.

> One thing that struck me is that most nodes have nop as their opcode name,
> which isn't very informative. Maybe we should print the token type instead.

I agree. Sorry, my initial sloppy code misled you here... See frontend/ParseNode.h for a huge comment about the various types of parse node. They are classified by arity and token type, not opcode.

> Another observation is that when a name node is not used, pn_name cannot be
> accessed. Why is this the case, and can we do better here?

Maybe pn_atom is what you’re looking for.

This is good stuff. Looking forward to having it around. Feel free to commit and push what you’ve got; or do another round of changes here and post a new patch. I'll turn the review around faster next time; I got backed up during my week off.

Comment 3

[David Mandelin [:dmandelin]]

r+'d patch here but no checkin. Is it done?

Comment 4

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/af0ab64cb947

Comment 5

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9228a072133d

because it failed to build with:
js/src/frontend/ParseNode-inl.h:76: error: no 'void js::ParseNode::dump(int)' member function declared in class 'js::ParseNode'
https://tbpl.mozilla.org/php/getParsedLog.php?id=9194189&tree=Mozilla-Inbound

Comment 6

[David Mandelin [:dmandelin]]

Sorry about that--I didn't think to test an opt build, which would have been good given all the #ifdef DEBUG. Relanded with a few more ifdefs:

http://hg.mozilla.org/integration/mozilla-inbound/rev/8426ff79238d

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8426ff79238d