[builtins] support exception handling in TFJ builtins

[builtins] support exception handling in TFJ builtins

BUG=v8:5264, v8:5268
R=mstarzinger@chromium.org, bmeurer@chromium.org, epertoso@chromium.org

Committed: https://crrev.com/f93ac8968a64cd44d636d7093884d810cbbbef6f
Cr-Commit-Position: refs/heads/master@{#38792}

[caitp, 2016-08-18 03:08:57]

This is a WIP, but some feedback would be appreciated.

I've started implementing the approach more or less suggested by mstarzinger in
the google doc I linked earlier today.

Tests pass locally in a debug build, but there are still questions and things
left to do.

https://codereview.chromium.org/2247353005/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/2247353005/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.h:751: void BranchIfException(Node* node,
RawMachineLabel* if_exception);
I very much prefer a separate method vs adding optional if_exception Labels to
the millions of operations that the various Assemblers can perform.

https://codereview.chromium.org/2247353005/diff/1/src/compiler/schedule.h
File src/compiler/schedule.h (right):

https://codereview.chromium.org/2247353005/diff/1/src/compiler/schedule.h#new...
src/compiler/schedule.h:99: void RemoveLast() { nodes_.pop_back(); }
oops, not really needed. thankfully.

https://codereview.chromium.org/2247353005/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2247353005/diff/1/src/isolate.cc#newcode1295
src/isolate.cc:1295: : raw_native_context();
I'm not sure about this:

For TFJ builtins, it looks like the frame should be more or less identical to
normal JS frames, however `maybe_context` is never a Context in the cctests
provided. I haven't checked if this would behave differently with proper TFJ
builtins or not yet.

Since stubs are basically internal, I think always using the currently entered
context is _probably_ fine though, but maybe not?

https://codereview.chromium.org/2247353005/diff/1/src/isolate.cc#newcode1300
src/isolate.cc:1300: offset = code_offset;
Without setting the offset, the stub is restarted from the beginning, and it
gets ugly and breaks things. I'm not actually 100% sure this does work, but it
does seem to get to the right spot, at least (in the two test cases).

https://codereview.chromium.org/2247353005/diff/1/test/cctest/test-code-stub-...
File test/cctest/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/2247353005/diff/1/test/cctest/test-code-stub-...
test/cctest/test-code-stub-assembler.cc:1394: m.Return(string);
The fact that this test passes is very surprising to me. An op like
`GetException()` or something would be very nice, and would read easier.

I haven't started working on something like that yet, as I'm not certain how it
would even work. I'd prefer that to defining a Variable and passing it to
BranchIfException() though.

[caitp, 2016-08-18 16:59:21]

PTAL --- I've moved most of the work out of RawMachineAssembler, as it seems
more appropriate to live in CodeAssembler / CodeStubAssembler.

Renamed `BranchIfException` to `BranchIfSuccess`, with a `GotoIfException`
convenience wrapper.

There's a bug with stackframe construction after resuming execution of a stub,
though, I'm not positive what is responsible for it.

Since that is the last (apparently) issue that I have with it, I think it's
ready for review.

https://codereview.chromium.org/2247353005/diff/40001/test/cctest/test-code-s...
File test/cctest/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/2247353005/diff/40001/test/cctest/test-code-s...
test/cctest/test-code-stub-assembler.cc:1412: m.GotoIfException(string,
&exception_handler2);
There's something weird with the latest refactoring.

When the stub / Runtime_ToString() throws on the line above this, the stackframe
for this test builtin is different (reported as an OPTIMIZED frame). This
crashes, because of an attempt to summarize it as a JS frame, with incorrect
`function` and `receiver` variables. The object at the Function offset is the
argument to Runtime_ToString() (to_string_tag).

So this test does not run to conclusion. I'm not sure if this is caused by the
way I resume execution from the first stub, or if the frame being looked at is
actually the runtime function, or what.

[Michael Starzinger, 2016-08-19 08:53:57]

Cool stuff, I like a lot where this is going. Thanks! Left comments on the
compiler pieces, pretty much just polishing there. Haven't yet looked at the
stack unwinding changes, will do so later today.

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/code-assem...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/code-assem...
src/compiler/code-assembler.cc:318: Label exception(this);
Let's mark this label as deferred, to move the handler block out of the
instruction stream.

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/code-assem...
src/compiler/code-assembler.cc:334: void CodeAssembler::GotoIfException(Node*
node, Label* if_exception,
It seems that users of the CodeAssembler will be mostly interested in this
method (i.e. fall-through on success and goto on exception). Is there any
use-case for the BranchIfSuccess method above?

If no then I would vote for merging BranchIfSuccess into this method completely
(or alternatively making it a private method).

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:751: void BranchIfSuccess(Node* call,
RawMachineLabel* if_success,
I like the fact that the RawMachineAssembler is offering just the bare-bones
interface to add the control projections to a call. This is nice. Can we rename
the method to something that does not contain the word "branch" though, to
clarify that they are continuations and not actual branches. I would vote for
calling the method just "Continuation" or "Continuations". WDYT?

[caitp, 2016-08-19 14:30:10]

https://codereview.chromium.org/2247353005/diff/40001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/2247353005/diff/40001/src/frames.cc#newcode882
src/frames.cc:882: int StubFrame::LookupExceptionHandlerInTable(
seems to be unnecessary, only seemed to come up due to the tests generating stub
code rather than JS frames.

[caitp, 2016-08-19 15:32:36]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 15:32:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2016-08-19 15:38:53]

> Cool stuff, I like a lot where this is going. Thanks! Left comments on the
compiler pieces, pretty much just polishing there. Haven't yet looked at the
stack unwinding changes, will do so later today.

I believe I've solved the unwinding problem, and removed most of the code that
was added for it.

All that seems to be needed for TFJ builtins, is to avoid the
marked_for_deoptimization() (which fails a DCHECK due to having an incorrect
code type).

If this needs to work for normal stubs as well, then a bit more is needed to
support that, but this covers all of my uses for this.

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/code-assem...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/code-assem...
src/compiler/code-assembler.cc:318: Label exception(this);
On 2016/08/19 08:53:56, Michael Starzinger wrote:
> Let's mark this label as deferred, to move the handler block out of the
> instruction stream.

Done.

https://codereview.chromium.org/2247353005/diff/40001/src/compiler/code-assem...
src/compiler/code-assembler.cc:334: void CodeAssembler::GotoIfException(Node*
node, Label* if_exception,
On 2016/08/19 08:53:56, Michael Starzinger wrote:
> It seems that users of the CodeAssembler will be mostly interested in this
> method (i.e. fall-through on success and goto on exception). Is there any
> use-case for the BranchIfSuccess method above?
> 
> If no then I would vote for merging BranchIfSuccess into this method
completely
> (or alternatively making it a private method).

I've merged them into just GotoIfException --- I just figured it would be good
to follow the apparent convention of Goto and Branch being available

[commit-bot: I haz the power, 2016-08-19 15:56:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 15:56:15]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-08-19 18:36:43]

On 2016/08/19 15:38:53, caitp wrote:
> > Cool stuff, I like a lot where this is going. Thanks! Left comments on the
> compiler pieces, pretty much just polishing there. Haven't yet looked at the
> stack unwinding changes, will do so later today.
> 
> I believe I've solved the unwinding problem, and removed most of the code that
> was added for it.
> 
> All that seems to be needed for TFJ builtins, is to avoid the
> marked_for_deoptimization() (which fails a DCHECK due to having an incorrect
> code type).
> 
> If this needs to work for normal stubs as well, then a bit more is needed to
> support that, but this covers all of my uses for this.

Having it support BUILTIN code kinds is definitely good enough for now.

[Michael Starzinger, 2016-08-19 18:37:44]

Awesome stuff! Just one final round of nits and then this is ready to go. Tests
also look good.

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
src/compiler/code-assembler.cc:328: exception_var->Bind(node);
nit: Technically the exception is the value output of the IfException node
instead of the call node. For a scheduled graph (as the RawMachineAssember is
building) where we are only dealing with calls that have at least one return
value, it shouldn't really matter all too much. But I would still go with using
the IfException node here (unless that causes unexpected troubles somewhere).

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
src/compiler/code-assembler.h:412: void GotoIfException(Node* node, Label*
if_exception,
nit: Lets move this up one function to not have it sit in between the branching
helpers. Also maybe add a "// Exception handling support." comment.

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:761: // Add success / exception successor
blocks to a block ending with a throwing
nit: What do you think about phrasing the comment as: "Add success / exception
successor blocks and ends the current basic block with a potentially throwing
call node." instead? Also add an empty newline before the comment for
readability.

[caitp, 2016-08-19 20:13:58]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 20:14:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2016-08-19 20:37:11]

nits addressed, thanks for the review.

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
src/compiler/code-assembler.cc:328: exception_var->Bind(node);
On 2016/08/19 18:37:43, Michael Starzinger wrote:
> nit: Technically the exception is the value output of the IfException node
> instead of the call node. For a scheduled graph (as the RawMachineAssember is
> building) where we are only dealing with calls that have at least one return
> value, it shouldn't really matter all too much. But I would still go with
using
> the IfException node here (unless that causes unexpected troubles somewhere).

passes the tests, so if this is what's expected, seems okay

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/code-assem...
src/compiler/code-assembler.h:412: void GotoIfException(Node* node, Label*
if_exception,
On 2016/08/19 18:37:43, Michael Starzinger wrote:
> nit: Lets move this up one function to not have it sit in between the
branching
> helpers. Also maybe add a "// Exception handling support." comment.

done

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/2247353005/diff/60001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:761: // Add success / exception successor
blocks to a block ending with a throwing
On 2016/08/19 18:37:43, Michael Starzinger wrote:
> nit: What do you think about phrasing the comment as: "Add success / exception
> successor blocks and ends the current basic block with a potentially throwing
> call node." instead? Also add an empty newline before the comment for
> readability.

sgtm, done

[commit-bot: I haz the power, 2016-08-19 20:48:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 20:48:55]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-08-22 07:26:27]

LGTM. Thanks!

[epertoso, 2016-08-22 07:54:50]

lgtm


Great!

[caitp, 2016-08-22 15:33:10]

On 2016/08/22 07:54:50, epertoso wrote:
> lgtm
> 
> 
> Great!

Sounds like folks are good with it, gonna give it a try

[caitp, 2016-08-22 15:33:16]

The CQ bit was checked by caitp@igalia.com

[commit-bot: I haz the power, 2016-08-22 15:33:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 15:58:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-22 15:58:16]

Description was changed from

==========
[builtins] support exception handling in TFJ builtins

BUG=v8:5264, v8:5268
R=mstarzinger@chromium.org, bmeurer@chromium.org, epertoso@chromium.org
==========

to

==========
[builtins] support exception handling in TFJ builtins

BUG=v8:5264, v8:5268
R=mstarzinger@chromium.org, bmeurer@chromium.org, epertoso@chromium.org

Committed: https://crrev.com/f93ac8968a64cd44d636d7093884d810cbbbef6f
Cr-Commit-Position: refs/heads/master@{#38792}
==========

[commit-bot: I haz the power, 2016-08-22 15:58:17]

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