Last Comment Bug 675694 - Implement a way to dump parse trees from within C/C++
: Implement a way to dump parse trees from within C/C++
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Eddy Bruel [:ejpbruel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-01 11:35 PDT by Eddy Bruel [:ejpbruel]
Modified: 2012-02-09 10:17 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First proposal (8.74 KB, patch)
2011-11-10 06:21 PST, Eddy Bruel [:ejpbruel]
jorendorff: review+
Details | Diff | Splinter Review

Description Eddy Bruel [:ejpbruel] 2011-08-01 11:35:55 PDT
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] 2011-11-10 06:21:37 PST
Created attachment 573498 [details] [diff] [review]
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] 2011-11-23 11:51:29 PST
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] 2011-12-06 14:17:09 PST
r+'d patch here but no checkin. Is it done?
Comment 4 David Mandelin [:dmandelin] 2012-02-08 17:22:14 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/af0ab64cb947
Comment 5 Matt Brubeck (:mbrubeck) 2012-02-08 17:38:17 PST
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] 2012-02-08 18:18:13 PST
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] 2012-02-09 10:17:22 PST
https://hg.mozilla.org/mozilla-central/rev/8426ff79238d

Note You need to log in before you can comment on or make changes to this bug.