Adding visitors to regurgitate expression types or reset them.

Adding visitors to regurgitate expression types or reset them.

Adding an AstExpressionVisitor to touch each expression node in
an AST.

Adding TypingReseter to clear the slate after a failed asm.js
validation that has set partial typing information.

Adding a ExpressionTypeCollector to walk the expressions
in an AST and emit them as a string for testing.

Adding tests of the above.

LOG=N
BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=test-typing-reset,test-ast-expression-visitor
R=rossberg@chromium.org,titzer@chromium.org

Committed: https://crrev.com/10f2c5c33a65a4b92fb44a7809e8e7be253264dc
Cr-Commit-Position: refs/heads/master@{#30336}

[bradn, 2015-08-19 07:50:28]

Patchset #2 (id:20001) has been deleted

[bradn, 2015-08-19 07:50:37]

Patchset #1 (id:1) has been deleted

[titzer, 2015-08-19 12:06:37]

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
src/ast-expression-visitor.cc:167: ++depth_;
IIUC, you are using the depth counter as the expression depth, since you don't
increment the depth when you recurse on statements. Could you make
Recurse(Statement) and a Recurse(Expression) methods that do the stack check,
with the latter incrementing the depth?

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
File src/ast-expression-visitor.h (right):

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
src/ast-expression-visitor.h:26: void* operator new(size_t size, Zone* zone) {
return zone->New(size); }
I don't think you want to define your own new here. I think you want to extend
ZoneObject and use "new (zone)". It's not safer, but it's the V8 way.

https://codereview.chromium.org/1288773007/diff/40001/test/cctest/expression-...
File test/cctest/expression-type-collector.cc (right):

https://codereview.chromium.org/1288773007/diff/40001/test/cctest/expression-...
test/cctest/expression-type-collector.cc:38: void
ExpressionTypeCollector::VisitExpression(Expression* expression) {
Collecting the types as a string is nice for debugging, but for testing purposes
maybe it'd be better to collect a tree or something that's easier to
programmatically check? E.g. if these tests fail, we'll want to debug why.

[bradn, 2015-08-20 04:01:41]

PTAL

This now gathers a list of expression type entries and uses macros to express
the expected tree structure. Failures will pinpoint the line and reason for the
failure.

This should allow tests confirming that typing is correct.
The macros could be made less verbose I suppose, at the cost of imprecision of
which line has the issue.

WDYT?

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
src/ast-expression-visitor.cc:167: ++depth_;
On 2015/08/19 12:06:37, titzer wrote:
> IIUC, you are using the depth counter as the expression depth, since you don't
> increment the depth when you recurse on statements. Could you make
> Recurse(Statement) and a Recurse(Expression) methods that do the stack check,
> with the latter incrementing the depth?

Done.

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
File src/ast-expression-visitor.h (right):

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
src/ast-expression-visitor.h:26: void* operator new(size_t size, Zone* zone) {
return zone->New(size); }
On 2015/08/19 12:06:37, titzer wrote:
> I don't think you want to define your own new here. I think you want to extend
> ZoneObject and use "new (zone)". It's not safer, but it's the V8 way.

Not groking. But maybe I'm missing something basic :-)

I can't inherit from ZoneObject without doing multiple inheritance here, as I
need to inherit from AstVisitor. Also, AstVisitor inherits from Embedded, which
in debug mode has a clashing operator new.

Currently this inlines ZoneObject in this class, so for instance in
TypingReseter::Run() I can do new (zone).
Or are you suggesting I do placement new there, ie:
new (zone->New(sizeof(TypingReseter))) TypingReseter ?

FWIW, this is the pattern used in typing.h and a few other places.

https://codereview.chromium.org/1288773007/diff/40001/test/cctest/expression-...
File test/cctest/expression-type-collector.cc (right):

https://codereview.chromium.org/1288773007/diff/40001/test/cctest/expression-...
test/cctest/expression-type-collector.cc:38: void
ExpressionTypeCollector::VisitExpression(Expression* expression) {
On 2015/08/19 12:06:37, titzer wrote:
> Collecting the types as a string is nice for debugging, but for testing
purposes
> maybe it'd be better to collect a tree or something that's easier to
> programmatically check? E.g. if these tests fail, we'll want to debug why.

Restructured to gather a list of expression type info entries, and macros to
assert their structure.

[rossberg, 2015-08-20 16:34:27]

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
File src/ast-expression-visitor.h (right):

https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visi...
src/ast-expression-visitor.h:26: void* operator new(size_t size, Zone* zone) {
return zone->New(size); }
On 2015/08/20 04:01:40, bradn wrote:
> On 2015/08/19 12:06:37, titzer wrote:
> > I don't think you want to define your own new here. I think you want to
extend
> > ZoneObject and use "new (zone)". It's not safer, but it's the V8 way.
> 
> Not groking. But maybe I'm missing something basic :-)
> 
> I can't inherit from ZoneObject without doing multiple inheritance here, as I
> need to inherit from AstVisitor. Also, AstVisitor inherits from Embedded,
which
> in debug mode has a clashing operator new.
> 
> Currently this inlines ZoneObject in this class, so for instance in
> TypingReseter::Run() I can do new (zone).
> Or are you suggesting I do placement new there, ie:
> new (zone->New(sizeof(TypingReseter))) TypingReseter ?
> 
> FWIW, this is the pattern used in typing.h and a few other places.

Honestly, I don't know why typing.h does that. Why would one want to store a
visitor on the heap? You can just have it on the stack, as in

TypingReseter visitor(info);
visitor.VisitAll();

or even

TypingReseter(info).VisitAll();

https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc
File src/typing-reset.cc (right):

https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc#ne...
src/typing-reset.cc:22: TypingReseter* visitor = new (info->zone())
TypingReseter(info);
No need to heap-allocate the visitor, AFAICT.

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
File test/cctest/expression-type-collector.cc (right):

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
test/cctest/expression-type-collector.cc:32: ExpressionTypeCollector* visitor =
Same here, no reason for heap allocation.

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
File test/cctest/expression-type-collector.h (right):

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
test/cctest/expression-type-collector.h:13: // Macros to define checking of a
tree walk.
That's some fancy macro fu there! :)

[bradn, 2015-08-20 21:35:47]

PTAL

That it much niced not needing to plumb in the zone.

I've sent out separate CL to change typing.h similarly.
I also looked at one of the items in hydrogen.h too that uses this pattern, but
it appears to actually need it, as that object gets handed to an
OptimizedCompileJob, which is also Zone allocated, but I'm unsure if the zone
outlives the scope.

https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc
File src/typing-reset.cc (right):

https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc#ne...
src/typing-reset.cc:22: TypingReseter* visitor = new (info->zone())
TypingReseter(info);
On 2015/08/20 16:34:26, rossberg wrote:
> No need to heap-allocate the visitor, AFAICT.

Ok, changed round.

By the way, a terminology question.
You referred to this as heap-allocation, but I had the impression that usually
refers to allocation in V8's GC heap? Whereas isn't this just in a Zone scoped
to this particular compile? Or have I misunderstood something about Zones + GC
heap?

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
File test/cctest/expression-type-collector.cc (right):

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
test/cctest/expression-type-collector.cc:32: ExpressionTypeCollector* visitor =
On 2015/08/20 16:34:26, rossberg wrote:
> Same here, no reason for heap allocation.

Done.

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
File test/cctest/expression-type-collector.h (right):

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
test/cctest/expression-type-collector.h:13: // Macros to define checking of a
tree walk.
On 2015/08/20 16:34:27, rossberg wrote:
> That's some fancy macro fu there! :)

Not sure whether that's a compliment of an insult :-)
Too much, or ok?

[titzer, 2015-08-21 10:33:18]

On 2015/08/20 21:35:47, bradn wrote:
> PTAL
> 
> That it much niced not needing to plumb in the zone.
> 
> I've sent out separate CL to change typing.h similarly.
> I also looked at one of the items in hydrogen.h too that uses this pattern,
but
> it appears to actually need it, as that object gets handed to an
> OptimizedCompileJob, which is also Zone allocated, but I'm unsure if the zone
> outlives the scope.

OptimizedCompileJobs go between threads, so it's likely they do. I didn't look
into this case yet, tho.

> 
> https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc
> File src/typing-reset.cc (right):
> 
>
https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc#ne...
> src/typing-reset.cc:22: TypingReseter* visitor = new (info->zone())
> TypingReseter(info);
> On 2015/08/20 16:34:26, rossberg wrote:
> > No need to heap-allocate the visitor, AFAICT.
> 
> Ok, changed round.
> 
> By the way, a terminology question.
> You referred to this as heap-allocation, but I had the impression that usually
> refers to allocation in V8's GC heap? Whereas isn't this just in a Zone scoped
> to this particular compile? Or have I misunderstood something about Zones + GC
> heap?
> 
>
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
> File test/cctest/expression-type-collector.cc (right):
> 
>
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
> test/cctest/expression-type-collector.cc:32: ExpressionTypeCollector* visitor
=
> On 2015/08/20 16:34:26, rossberg wrote:
> > Same here, no reason for heap allocation.
> 
> Done.
> 
>
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
> File test/cctest/expression-type-collector.h (right):
> 
>
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
> test/cctest/expression-type-collector.h:13: // Macros to define checking of a
> tree walk.
> On 2015/08/20 16:34:27, rossberg wrote:
> > That's some fancy macro fu there! :)
> 
> Not sure whether that's a compliment of an insult :-)
> Too much, or ok?

[titzer, 2015-08-21 12:05:51]

https://codereview.chromium.org/1288773007/diff/160001/src/ast-expression-vis...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1288773007/diff/160001/src/ast-expression-vis...
src/ast-expression-visitor.cc:17: #define RECURSE(call)               \
Can we make these real methods instead of macros?

https://codereview.chromium.org/1288773007/diff/160001/test/cctest/expression...
File test/cctest/expression-type-collector.h (right):

https://codereview.chromium.org/1288773007/diff/160001/test/cctest/expression...
test/cctest/expression-type-collector.h:13: // Macros to define checking of a
tree walk.
Can we move the macros to the test file?

[rossberg, 2015-08-21 12:36:51]

https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc
File src/typing-reset.cc (right):

https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc#ne...
src/typing-reset.cc:22: TypingReseter* visitor = new (info->zone())
TypingReseter(info);
On 2015/08/20 21:35:47, bradn wrote:
> By the way, a terminology question.
> You referred to this as heap-allocation, but I had the impression that usually
> refers to allocation in V8's GC heap? Whereas isn't this just in a Zone scoped
> to this particular compile? Or have I misunderstood something about Zones + GC
> heap?

I was just being sloppy. Meant heap-allocated as opposed to stack-allocated.

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
File test/cctest/expression-type-collector.h (right):

https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression...
test/cctest/expression-type-collector.h:13: // Macros to define checking of a
tree walk.
On 2015/08/20 21:35:47, bradn wrote:
> On 2015/08/20 16:34:27, rossberg wrote:
> > That's some fancy macro fu there! :)
> 
> Not sure whether that's a compliment of an insult :-)
> Too much, or ok?

Can't make up my mind. :) I think it's fine and useful for tests, but perhaps
would be too much in "real" code.

[bradn, 2015-08-21 21:37:08]

PTAL

https://codereview.chromium.org/1288773007/diff/160001/src/ast-expression-vis...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1288773007/diff/160001/src/ast-expression-vis...
src/ast-expression-visitor.cc:17: #define RECURSE(call)               \
On 2015/08/21 12:05:51, titzer wrote:
> Can we make these real methods instead of macros?

I've inlined them where they're needed for correct early out.
I've replaced the depth one with a scoped object.

https://codereview.chromium.org/1288773007/diff/160001/test/cctest/expression...
File test/cctest/expression-type-collector.h (right):

https://codereview.chromium.org/1288773007/diff/160001/test/cctest/expression...
test/cctest/expression-type-collector.h:13: // Macros to define checking of a
tree walk.
On 2015/08/21 12:05:51, titzer wrote:
> Can we move the macros to the test file?

As they're shared, moved them to a separate header.

[bradn, 2015-08-22 00:58:13]

Patchset #9 (id:200001) has been deleted

[bradn, 2015-08-22 01:19:13]

https://codereview.chromium.org/1288773007/diff/220001/src/ast-expression-vis...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1288773007/diff/220001/src/ast-expression-vis...
src/ast-expression-visitor.cc:63: RECURSE(Visit(stmt));
So Visit has baked in a short-circuit:

#define DEFINE_AST_VISITOR_SUBCLASS_MEMBERS()               \
 public:                                                    \
  void Visit(AstNode* node) final {                         \
    if (!CheckStackOverflow()) node->Accept(this);          \
  }                                                         \

So in terms of the suggestion you made by chat, wrapping this in VisitStmt or
somesuch, won't reduce the depth. Also, the macro has the advantage that in a
loop like this (or anywhere else where more than one visit call happens in
sequence), that you immediately exit the current function, instead of trying
each and backing out after each guard. Putting it in a function prevents the
return from skipping the rest of the function.

I could do for RECURSE what I did in this version for VisitDeeperExpression,
return a bool indicating if a short-circuit return should happen, but I'm not
sure that's more readable than the macro.

WDYT?
I'll try to catch you at some point to talk it over.

I do wonder if in general there might be some more coherent way to handle the
stack checks. It seems a shame to need to semi-reliably put in what amounts to
non-zero cost exception handling.
I would be interested in chatting about what the constraints really are here in
terms of using page faults etc.

In terms of specifically the AST visitor, would it be worthwhile to restructure
it to use a non-c-stack recursive pattern? I.e. Visit doesn't actually call
Accept directly, but instead pushes pending Visits onto a heap stack in the
visitor and an outer iterative loop dispatches the visits in the same order they
currently happen?

[titzer, 2015-08-24 09:51:25]

On 2015/08/22 01:19:13, bradn wrote:
>
https://codereview.chromium.org/1288773007/diff/220001/src/ast-expression-vis...
> File src/ast-expression-visitor.cc (right):
> 
>
https://codereview.chromium.org/1288773007/diff/220001/src/ast-expression-vis...
> src/ast-expression-visitor.cc:63: RECURSE(Visit(stmt));
> So Visit has baked in a short-circuit:
> 
> #define DEFINE_AST_VISITOR_SUBCLASS_MEMBERS()               \
>  public:                                                    \
>   void Visit(AstNode* node) final {                         \
>     if (!CheckStackOverflow()) node->Accept(this);          \
>   }                                                         \
> 
> So in terms of the suggestion you made by chat, wrapping this in VisitStmt or
> somesuch, won't reduce the depth. Also, the macro has the advantage that in a
> loop like this (or anywhere else where more than one visit call happens in
> sequence), that you immediately exit the current function, instead of trying
> each and backing out after each guard. Putting it in a function prevents the
> return from skipping the rest of the function.
> 
> I could do for RECURSE what I did in this version for VisitDeeperExpression,
> return a bool indicating if a short-circuit return should happen, but I'm not
> sure that's more readable than the macro.
> 
> WDYT?
> I'll try to catch you at some point to talk it over.
>

After seeing the alternatives I think I like RECURSE and RECURSE_EXPRESSION
macros the best.
 
> I do wonder if in general there might be some more coherent way to handle the
> stack checks. It seems a shame to need to semi-reliably put in what amounts to
> non-zero cost exception handling.
> I would be interested in chatting about what the constraints really are here
in
> terms of using page faults etc.
> 
> In terms of specifically the AST visitor, would it be worthwhile to
restructure
> it to use a non-c-stack recursive pattern? I.e. Visit doesn't actually call
> Accept directly, but instead pushes pending Visits onto a heap stack in the
> visitor and an outer iterative loop dispatches the visits in the same order
they
> currently happen?

That's beyond the scope of this CL. We've gotten by with other visitors
remaining recursive, so I don't see the need. Besides, you end up needing a
stack of both AST nodes and an index of some kind to keep track of where you
were in the recursion, and that gets ugly quick.

[bradn, 2015-08-24 16:50:58]

Back to the macros as discussed.

[titzer, 2015-08-24 17:00:26]

lgtm

[bradn, 2015-08-24 17:03:34]

The CQ bit was checked by bradnelson@google.com

[commit-bot: I haz the power, 2015-08-24 17:03:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288773007/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288773007/240001

[commit-bot: I haz the power, 2015-08-24 17:16:44]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2015-08-24 17:17:01]

Patchset 10 (id:??) landed as
https://crrev.com/10f2c5c33a65a4b92fb44a7809e8e7be253264dc
Cr-Commit-Position: refs/heads/master@{#30336}