[builtins] Add receiver to builtin exit frames

[builtins] Add receiver to builtin exit frames

Stack trace generation requires access to the receiver; and while the
receiver is already on the stack, we cannot determine its position
during stack trace generation (it's stored in argv[0], and argc is only
stored in a callee-saved register).

This patch grants access to the receiver by pushing argc onto builtin
exit frames as an extra argument. Compared to simply pushing the
receiver, this requires an additional dereference during stack trace
generation, but one fewer during builtin calls.

BUG=v8:4815
Committed: https://crrev.com/f59a23356b437e369d75445177fea838ee5bccc7
Cr-Commit-Position: refs/heads/master@{#37500}

[jgruber, 2016-06-29 10:46:30]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 10:46:34]

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

[jgruber, 2016-06-29 10:48:44]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 10:48:54]

Dry run: 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-06-29 10:54:24]

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

[commit-bot: I haz the power, 2016-06-29 10:54:26]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[jgruber, 2016-06-29 12:03:49]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 12:04:00]

Dry run: 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-06-29 12:31:16]

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

[commit-bot: I haz the power, 2016-06-29 12:31:17]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-06-30 07:21:37]

jgruber@chromium.org changed reviewers:
+ bmeurer@chromium.org, yangguo@chromium.org

[jgruber, 2016-06-30 08:38:20]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 08:38:27]

Dry run: 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-06-30 09:06:57]

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

[commit-bot: I haz the power, 2016-06-30 09:06:57]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-06-30 12:34:23]

lgtm. Please wait for Benedikt's review.

https://codereview.chromium.org/2106883003/diff/60001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2106883003/diff/60001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:44: __ SmiTag(r0);
Since we now make the distinction between builtin-exit and normal exit, couldn't
we move part of this adaptor into the CEntryStub? The difference being that we
generate CEntryStub only once, but an adaptor for every builtin.

https://codereview.chromium.org/2106883003/diff/60001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2106883003/diff/60001/src/builtins.cc#newcode67
src/builtins.cc:67: kNewTargetOffset = 0,
I think we usually use constants and explicitly spell out their values.

https://codereview.chromium.org/2106883003/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2106883003/diff/60001/src/isolate.cc#newcode436
src/isolate.cc:436: // CallSite::IsConstructor does not correctly detect builtin
Isn't this comment outdated now that you actually fixed IsConstructor to be
correct?

[jgruber, 2016-06-30 13:33:59]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 13:34:08]

Dry run: 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-06-30 14:03:01]

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

[commit-bot: I haz the power, 2016-06-30 14:03:02]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-06-30 14:40:29]

https://codereview.chromium.org/2106883003/diff/60001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2106883003/diff/60001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:44: __ SmiTag(r0);
On 2016/06/30 12:34:23, Yang wrote:
> Since we now make the distinction between builtin-exit and normal exit,
couldn't
> we move part of this adaptor into the CEntryStub? The difference being that we
> generate CEntryStub only once, but an adaptor for every builtin.

Not without larger changes, since HandleApiCall* builtins don't use BUILTIN_EXIT
frames, but still expect extra arguments on the stack.  CEntryStub can't tell
the difference between HandleApiCall* and other C builtins.

If we really want to do this, we'd have to revert
https://codereview.chromium.org/2074063002 and go from there.

https://codereview.chromium.org/2106883003/diff/60001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2106883003/diff/60001/src/builtins.cc#newcode67
src/builtins.cc:67: kNewTargetOffset = 0,
On 2016/06/30 12:34:23, Yang wrote:
> I think we usually use constants and explicitly spell out their values.

Done.

https://codereview.chromium.org/2106883003/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2106883003/diff/60001/src/isolate.cc#newcode436
src/isolate.cc:436: // CallSite::IsConstructor does not correctly detect builtin
On 2016/06/30 12:34:23, Yang wrote:
> Isn't this comment outdated now that you actually fixed IsConstructor to be
> correct?

Done.

[jgruber, 2016-06-30 14:41:36]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 14:41:41]

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

[jgruber, 2016-06-30 14:45:47]

https://codereview.chromium.org/2106883003/diff/60001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2106883003/diff/60001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:44: __ SmiTag(r0);
On 2016/06/30 12:34:23, Yang wrote:
> Since we now make the distinction between builtin-exit and normal exit,
couldn't
> we move part of this adaptor into the CEntryStub? The difference being that we
> generate CEntryStub only once, but an adaptor for every builtin.

I guess we could also pass in whether we're calling from GenerateAdaptor, but
that seems like a bit of a hack.

[commit-bot: I haz the power, 2016-06-30 15:39:40]

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

[commit-bot: I haz the power, 2016-06-30 15:39:41]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-07-01 08:38:36]

LGTM, thanks.

[Yang, 2016-07-04 06:45:29]

On 2016/07/01 08:38:36, Benedikt Meurer wrote:
> LGTM, thanks.

lgtm.

[jgruber, 2016-07-04 12:19:32]

The CQ bit was checked by jgruber@chromium.org

[commit-bot: I haz the power, 2016-07-04 12:19:39]

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-07-04 12:44:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-04 12:46:58]

Description was changed from

==========
[builtins] Add receiver to builtin exit frames

Stack trace generation requires access to the receiver; and while the
receiver is already on the stack, we cannot determine its position
during stack trace generation (it's stored in argv[0], and argc is only
stored in a callee-saved register).

This patch grants access to the receiver by pushing argc onto builtin
exit frames as an extra argument. Compared to simply pushing the
receiver, this requires an additional dereference during stack trace
generation, but one fewer during builtin calls.

BUG=v8:4815
==========

to

==========
[builtins] Add receiver to builtin exit frames

Stack trace generation requires access to the receiver; and while the
receiver is already on the stack, we cannot determine its position
during stack trace generation (it's stored in argv[0], and argc is only
stored in a callee-saved register).

This patch grants access to the receiver by pushing argc onto builtin
exit frames as an extra argument. Compared to simply pushing the
receiver, this requires an additional dereference during stack trace
generation, but one fewer during builtin calls.

BUG=v8:4815

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

[commit-bot: I haz the power, 2016-07-04 12:46:59]

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