[builtins] Construct builtin frame in String/Number ctors

[builtins] Construct builtin frame in String/Number ctors

BUG=v8:5173
R=bmeurer@chromium.org

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

[jgruber, 2016-07-04 13:17:44]

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

[commit-bot: I haz the power, 2016-07-04 13:17:52]

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

[jgruber, 2016-07-04 13:22:50]

Could you please take a brief look at the x64-specific code before I port all
architectures? I commented the relevant parts.

[jgruber, 2016-07-04 13:25:45]

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:1895: // TODO(jgruber): Is a builtin frame needed here?
Just double-checking - do we want a builtin frame here?

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:1968: __
TailCallRuntime(Runtime::kSymbolDescriptiveString);
No builtin frame here because SymbolDescriptiveString only throws when arg is
not a symbol (and we check before the call).

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:2050: __ Push(rbx);  // the first argument
As above.

[commit-bot: I haz the power, 2016-07-04 13:38:40]

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

[commit-bot: I haz the power, 2016-07-04 13:38:41]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8451)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Benedikt Meurer, 2016-07-04 16:45:37]

LGTM with comments, thanks. Questions addressed.

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:1895: // TODO(jgruber): Is a builtin frame needed here?
Yes because of new.target this is actually observable.

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:1968: __
TailCallRuntime(Runtime::kSymbolDescriptiveString);
On 2016/07/04 13:25:44, jgruber wrote:
> No builtin frame here because SymbolDescriptiveString only throws when arg is
> not a symbol (and we check before the call).

Acknowledged.

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:2050: __ Push(rbx);  // the first argument
Yes, this is observable (thx to new.target...)

https://codereview.chromium.org/2118283003/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-4815.js (right):

https://codereview.chromium.org/2118283003/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-4815.js:70: // Assembler builtins.
Can you move these to regress-5173? This way it's easier to associate with the
appropriate bug.

[jgruber, 2016-07-05 10:52:17]

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

[commit-bot: I haz the power, 2016-07-05 10:52:25]

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-07-05 11:08:16]

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

[commit-bot: I haz the power, 2016-07-05 11:08:18]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8492)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[jgruber, 2016-07-05 12:30:26]

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

[commit-bot: I haz the power, 2016-07-05 12:30:34]

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-07-05 12:47:51]

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

[commit-bot: I haz the power, 2016-07-05 12:47:51]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8501)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[jgruber, 2016-07-05 15:22:57]

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

[commit-bot: I haz the power, 2016-07-05 15:23:07]

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-07-05 15:40:28]

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

[commit-bot: I haz the power, 2016-07-05 15:40:28]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8518)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[jgruber, 2016-07-06 07:19:23]

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

[commit-bot: I haz the power, 2016-07-06 07:19:28]

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-07-06 07:38:18]

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

[commit-bot: I haz the power, 2016-07-06 07:38:20]

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...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/4488)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[jgruber, 2016-07-06 08:33:24]

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

[commit-bot: I haz the power, 2016-07-06 08:33:32]

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-07-06 08:52:02]

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

[commit-bot: I haz the power, 2016-07-06 08:52:03]

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

[jgruber, 2016-07-06 09:24:40]

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

[commit-bot: I haz the power, 2016-07-06 09:24:51]

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-07-06 09:50:46]

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

[commit-bot: I haz the power, 2016-07-06 09:50:47]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

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

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

[commit-bot: I haz the power, 2016-07-06 11:37:33]

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-07-06 12:14:10]

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

[commit-bot: I haz the power, 2016-07-06 12:14:11]

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

[jgruber, 2016-07-06 13:05:32]

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

[commit-bot: I haz the power, 2016-07-06 13:05:42]

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

[jgruber, 2016-07-06 13:12:01]

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

[commit-bot: I haz the power, 2016-07-06 13:12:16]

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-07-06 13:40:30]

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

[commit-bot: I haz the power, 2016-07-06 13:40:31]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-07-07 11:16:50]

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

[commit-bot: I haz the power, 2016-07-07 11:17: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-07-07 11:43:02]

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

[commit-bot: I haz the power, 2016-07-07 11:43:03]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-07-07 11:44:56]

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

[commit-bot: I haz the power, 2016-07-07 11:45:08]

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

[jgruber, 2016-07-07 11:50:05]

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

[commit-bot: I haz the power, 2016-07-07 11:50:08]

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

[jgruber, 2016-07-07 11:50:52]

This turned out to be much more involved than I initially thought, due to all
the different architectures involved. All ports should now be complete. 

We still have the issue of 'new Number(x)' and 'new String(x)' not being
detected as constructors during stack trace formatting. E.g.:

out/x64/Debug/d8 <<< 'new Number({ [Symbol.toPrimitive]: () => FAIL })'
[...]                                         ^
ReferenceError: FAIL is not defined
    at Object.[Symbol.toPrimitive] ((d8):1:42)
    at Function.Number (<anonymous>)
    at (d8):1:1

I'll handle that in an upcoming commit.

PTAL.

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:1895: // TODO(jgruber): Is a builtin frame needed here?
On 2016/07/04 16:45:36, Benedikt Meurer wrote:
> Yes because of new.target this is actually observable.

Done.

https://codereview.chromium.org/2118283003/diff/1/src/x64/builtins-x64.cc#new...
src/x64/builtins-x64.cc:2050: __ Push(rbx);  // the first argument
On 2016/07/04 16:45:36, Benedikt Meurer wrote:
> Yes, this is observable (thx to new.target...)

Done.

https://codereview.chromium.org/2118283003/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-4815.js (right):

https://codereview.chromium.org/2118283003/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-4815.js:70: // Assembler builtins.
On 2016/07/04 16:45:37, Benedikt Meurer wrote:
> Can you move these to regress-5173? This way it's easier to associate with the
> appropriate bug.

Done.

[commit-bot: I haz the power, 2016-07-07 12:17:32]

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

[commit-bot: I haz the power, 2016-07-07 12:17:33]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-07-07 19:02:53]

Very nice, thanks again. Still LGTM.

[jgruber, 2016-07-08 06:33:08]

The CQ bit was checked by jgruber@chromium.org

[commit-bot: I haz the power, 2016-07-08 06:33:17]

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-08 06:35:24]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-07-08 06:38:29]

Description was changed from

==========
[builtins] Construct builtin frame in String/Number ctors

BUG=v8:5173
R=bmeurer@chromium.org
==========

to

==========
[builtins] Construct builtin frame in String/Number ctors

BUG=v8:5173
R=bmeurer@chromium.org

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

[commit-bot: I haz the power, 2016-07-08 06:38:31]

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