Move FormatStackTrace to C++

Move FormatStackTrace to C++

BUG=

Committed: https://crrev.com/6c53efc7c434f62ca0cde62dc4697410fa21904f
Cr-Commit-Position: refs/heads/master@{#38212}

[jgruber, 2016-07-29 10:06:53]

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

[commit-bot: I haz the power, 2016-07-29 10:07:04]

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-29 10:21:54]

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

[commit-bot: I haz the power, 2016-07-29 10:21:55]

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...)

[jgruber, 2016-07-29 10:31:03]

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

[commit-bot: I haz the power, 2016-07-29 10:31:15]

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-29 10:53:55]

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

[commit-bot: I haz the power, 2016-07-29 10:53:56]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-07-29 11:29:15]

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

[commit-bot: I haz the power, 2016-07-29 11:29:20]

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-29 11:51:00]

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

[commit-bot: I haz the power, 2016-07-29 11:51:01]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-07-29 11:54:36]

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

[jgruber, 2016-07-29 11:54:37]

https://codereview.chromium.org/2191293002/diff/40001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2191293002/diff/40001/src/isolate.h#newcode129
src/isolate.h:129: #define RETURN_RESULT(isolate, call, T)           \
This is related to RETURN_RESULT_OR_FAILURE as ASSIGN_RETURN_ON_EXCEPTION is
related to ASSIGN_RETURN_FAILURE_ON_EXCEPTION.

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc#newcode601
src/messages.cc:601: // TODO(jgruber): Handle recursive calls.
Is this still needed? And if so, how do we want to track it? Setting a flag on
isolate?

[jgruber, 2016-07-29 11:56:06]

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc#newcode601
src/messages.cc:601: // TODO(jgruber): Handle recursive calls.
On 2016/07/29 11:54:37, jgruber wrote:
> Is this still needed? And if so, how do we want to track it? Setting a flag on
> isolate?

Recursion handling was introduced in a3f16f8e650.

[Yang, 2016-08-01 06:49:18]

LGTM with comments.

https://codereview.chromium.org/2191293002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2191293002/diff/40001/src/isolate.cc#newcode439
src/isolate.cc:439: Handle<Object> HoleToUndefined(Isolate* isolate,
Handle<Object> in) {
Let's call this TheHoleToUndefined.

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc#newcode516
src/messages.cc:516: for (int i = 1; i < raw_stack_len; i += 4) {
I wonder whether allocating CallSite objects is more expensive than the internal
array. If it's not significantly more expensive, maybe we should just allocate
CallSite objects in CaptureSimpleStackTrace directly and avoid this conversion
here.

But let's not do that in this CL.

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc#newcode601
src/messages.cc:601: // TODO(jgruber): Handle recursive calls.
On 2016/07/29 11:54:37, jgruber wrote:
> Is this still needed? And if so, how do we want to track it? Setting a flag on
> isolate?

I thought we had a test case covering this. Does this work?

Error.prepareStackTrace = function() {
  return new Error().stack;
}
new Error().stack;

https://codereview.chromium.org/2191293002/diff/40001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2191293002/diff/40001/src/messages.h#newcode101
src/messages.h:101: MaybeHandle<Object> ConstructCallSite(Isolate* isolate,
Can we make this a static method of the CallSite class?

https://codereview.chromium.org/2191293002/diff/40001/src/messages.h#newcode108
src/messages.h:108: MaybeHandle<String> CallSiteToString(Isolate* isolate,
Handle<Object> recv);
same here.

[jgruber, 2016-08-01 10:32:46]

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

[commit-bot: I haz the power, 2016-08-01 10:32:50]

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-08-01 10:36:01]

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

[commit-bot: I haz the power, 2016-08-01 10:36:03]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[jgruber, 2016-08-01 10:37:30]

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

[commit-bot: I haz the power, 2016-08-01 10:37: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-08-01 10:52:56]

https://codereview.chromium.org/2191293002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2191293002/diff/40001/src/isolate.cc#newcode439
src/isolate.cc:439: Handle<Object> HoleToUndefined(Isolate* isolate,
Handle<Object> in) {
On 2016/08/01 06:49:18, Yang wrote:
> Let's call this TheHoleToUndefined.

Done.

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc#newcode516
src/messages.cc:516: for (int i = 1; i < raw_stack_len; i += 4) {
On 2016/08/01 06:49:18, Yang wrote:
> I wonder whether allocating CallSite objects is more expensive than the
internal
> array. If it's not significantly more expensive, maybe we should just allocate
> CallSite objects in CaptureSimpleStackTrace directly and avoid this conversion
> here.
> 
> But let's not do that in this CL.

Acknowledged.

https://codereview.chromium.org/2191293002/diff/40001/src/messages.cc#newcode601
src/messages.cc:601: // TODO(jgruber): Handle recursive calls.
On 2016/08/01 06:49:18, Yang wrote:
> On 2016/07/29 11:54:37, jgruber wrote:
> > Is this still needed? And if so, how do we want to track it? Setting a flag
on
> > isolate?
> 
> I thought we had a test case covering this. Does this work?
> 
> Error.prepareStackTrace = function() {
>   return new Error().stack;
> }
> new Error().stack;
> 

It worked, but more or less accidentally since the second stack overflow
exception was swallowed. Adding a flag to isolate as discussed.

https://codereview.chromium.org/2191293002/diff/40001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2191293002/diff/40001/src/messages.h#newcode101
src/messages.h:101: MaybeHandle<Object> ConstructCallSite(Isolate* isolate,
On 2016/08/01 06:49:18, Yang wrote:
> Can we make this a static method of the CallSite class?

Moved to all-static ErrorUtils class as discussed.

https://codereview.chromium.org/2191293002/diff/40001/src/messages.h#newcode108
src/messages.h:108: MaybeHandle<String> CallSiteToString(Isolate* isolate,
Handle<Object> recv);
On 2016/08/01 06:49:18, Yang wrote:
> same here.

Ditto.

[commit-bot: I haz the power, 2016-08-01 11:14:34]

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

[commit-bot: I haz the power, 2016-08-01 11:14:35]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-08-01 12:31:20]

lgtm

https://codereview.chromium.org/2191293002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2191293002/diff/80001/src/isolate.h#newcode1306
src/isolate.h:1306: bool formatting_stack_trace_;
You can add this to ISOLATE_INIT_LIST

https://codereview.chromium.org/2191293002/diff/80001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2191293002/diff/80001/src/messages.cc#newcode931
src/messages.cc:931: ASSIGN_RETURN_ON_EXCEPTION(isolate, pos_int32,
Object::ToInt32(isolate, pos),
Can we check this for being a Smi as well and omit the ToInt32?

[jgruber, 2016-08-01 14:01:08]

https://codereview.chromium.org/2191293002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2191293002/diff/80001/src/isolate.h#newcode1306
src/isolate.h:1306: bool formatting_stack_trace_;
On 2016/08/01 12:31:20, Yang wrote:
> You can add this to ISOLATE_INIT_LIST

Done.

https://codereview.chromium.org/2191293002/diff/80001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2191293002/diff/80001/src/messages.cc#newcode931
src/messages.cc:931: ASSIGN_RETURN_ON_EXCEPTION(isolate, pos_int32,
Object::ToInt32(isolate, pos),
On 2016/08/01 12:31:20, Yang wrote:
> Can we check this for being a Smi as well and omit the ToInt32?

Done.

[jgruber, 2016-08-01 14:01:17]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-08-01 14:01:18]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2191293002/#ps100001
(title: "Add formatting_stack_trace to ISOLATE_INIT_LIST")

[commit-bot: I haz the power, 2016-08-01 14:01:26]

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-01 14:27:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-01 14:28:41]

Description was changed from

==========
Move FormatStackTrace to C++

BUG=
==========

to

==========
Move FormatStackTrace to C++

BUG=

Committed: https://crrev.com/6c53efc7c434f62ca0cde62dc4697410fa21904f
Cr-Commit-Position: refs/heads/master@{#38212}
==========

[commit-bot: I haz the power, 2016-08-01 14:28:42]

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