Closed Bug 675694 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Details

Attachments

(1 file)

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++.
Assignee: general → ejpbruel
Attached patch First proposalSplinter Review
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?
Attachment #573498 - Flags: review?(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.
Attachment #573498 - Flags: review?(jorendorff) → review+
r+'d patch here but no checkin. Is it done?
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
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
https://hg.mozilla.org/mozilla-central/rev/8426ff79238d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.