Add an option to dump core when an uncaught exception is thrown.

Add an option to dump core when an uncaught exception is thrown.


BUG=

Committed: https://code.google.com/p/v8/source/detail?r=14185

[dave.pacheco, 2013-03-25 20:34:20]

This is a change I originally mailed v8-discuss about in February:
https://groups.google.com/forum/?fromgroups=#!topic/v8-users/cJw2-SoAEWo

Slava suggested I should just submit this review with Jakob as a reviewer. 
Thanks in advance!

[Yang, 2013-03-27 14:18:31]

On 2013/03/25 20:34:20, dave.pacheco wrote:
> This is a change I originally mailed v8-discuss about in February:
> https://groups.google.com/forum/?fromgroups=#%21topic/v8-users/cJw2-SoAEWo
> 
> Slava suggested I should just submit this review with Jakob as a reviewer. 
> Thanks in advance!

I'm not sure why this change is necessary.

To give the output you added in this CL, you could, as embedder, simply add a
message handler. The message object would contain everything from error object
to stack trace.

If you want to look at the stack at the point where the uncaught exception is
thrown, you could just attach gdb and break at the point where you added your
code.

The problem with this patch is that the stack trace it prints is always from the
throw site. So if an error object is caught and rethrown, the original stack
trace is lost, so the output may be misleading.

[Yang, 2013-03-27 14:46:42]

On 2013/03/27 14:18:31, Yang wrote:
> On 2013/03/25 20:34:20, dave.pacheco wrote:
> > This is a change I originally mailed v8-discuss about in February:
> > https://groups.google.com/forum/?fromgroups=#%2521topic/v8-users/cJw2-SoAEWo
> > 
> > Slava suggested I should just submit this review with Jakob as a reviewer. 
> > Thanks in advance!
> 
> I'm not sure why this change is necessary.
> 
> To give the output you added in this CL, you could, as embedder, simply add a
> message handler. The message object would contain everything from error object
> to stack trace.
> 
> If you want to look at the stack at the point where the uncaught exception is
> thrown, you could just attach gdb and break at the point where you added your
> code.
> 
> The problem with this patch is that the stack trace it prints is always from
the
> throw site. So if an error object is caught and rethrown, the original stack
> trace is lost, so the output may be misleading.

Thanks Slava for explaining the use case to me!

I think what you want to do is to use MessageHandler::DefaultMessageReport and
V8_Fatal. 
The former takes care of formatting, the latter takes care of recursive
exceptions and a detailed javascript stack trace.

[Yang, 2013-03-27 14:50:40]

On 2013/03/27 14:46:42, Yang wrote:
> On 2013/03/27 14:18:31, Yang wrote:
> > On 2013/03/25 20:34:20, dave.pacheco wrote:
> > > This is a change I originally mailed v8-discuss about in February:
> > >
https://groups.google.com/forum/?fromgroups=#%252521topic/v8-users/cJw2-SoAEWo
> > > 
> > > Slava suggested I should just submit this review with Jakob as a reviewer.

> > > Thanks in advance!
> > 
> > I'm not sure why this change is necessary.
> > 
> > To give the output you added in this CL, you could, as embedder, simply add
a
> > message handler. The message object would contain everything from error
object
> > to stack trace.
> > 
> > If you want to look at the stack at the point where the uncaught exception
is
> > thrown, you could just attach gdb and break at the point where you added
your
> > code.
> > 
> > The problem with this patch is that the stack trace it prints is always from
> the
> > throw site. So if an error object is caught and rethrown, the original stack
> > trace is lost, so the output may be misleading.
> 
> Thanks Slava for explaining the use case to me!
> 
> I think what you want to do is to use MessageHandler::DefaultMessageReport and
> V8_Fatal. 
> The former takes care of formatting, the latter takes care of recursive
> exceptions and a detailed javascript stack trace.

I guess PrintCurrentStackTrace does print a cleaner stack trace. You could set
FLAG_stack_trace_on_abort to false before calling V8_Fatal... :/

[dave.pacheco, 2013-03-27 19:03:56]

On Wed, Mar 27, 2013 at 7:50 AM, <yangguo@chromium.org> wrote:

> On 2013/03/27 14:46:42, Yang wrote:
>
>> On 2013/03/27 14:18:31, Yang wrote:
>> > On 2013/03/25 20:34:20, dave.pacheco wrote:
>> > > This is a change I originally mailed v8-discuss about in February:
>> > >
>>
> https://groups.google.com/**forum/?fromgroups=#%**
>
252521topic/v8-users/cJw2-**SoAEWo<https://groups.google.com/forum/?fromgroups=#%252521topic/v8-users/cJw2-SoAEWo>
>
>  > >
>> > > Slava suggested I should just submit this review with Jakob as a
>> reviewer.
>>
>
>  > > Thanks in advance!
>> >
>> > I'm not sure why this change is necessary.
>> >
>> > To give the output you added in this CL, you could, as embedder, simply
>> add
>>
> a
>
>> > message handler. The message object would contain everything from error
>>
> object
>
>> > to stack trace.
>> >
>> > If you want to look at the stack at the point where the uncaught
>> exception
>>
> is
>
>> > thrown, you could just attach gdb and break at the point where you added
>>
> your
>
>> > code.
>> >
>> > The problem with this patch is that the stack trace it prints is always
>> from
>> the
>> > throw site. So if an error object is caught and rethrown, the original
>> stack
>> > trace is lost, so the output may be misleading.
>>
>
>  Thanks Slava for explaining the use case to me!
>>
>
>  I think what you want to do is to use MessageHandler::**DefaultMessageReport
>> and
>> V8_Fatal.
>> The former takes care of formatting, the latter takes care of recursive
>> exceptions and a detailed javascript stack trace.
>>
>
> I guess PrintCurrentStackTrace does print a cleaner stack trace. You could
> set
> FLAG_stack_trace_on_abort to false before calling V8_Fatal... :/
>
>
https://codereview.chromium.**org/13071005/<https://codereview.chromium.org/1...
>


Thanks for taking a look.  I looked at V8_Fatal at first, but it reports an
enormous amount of output, most of it appropriate only for someone familiar
with V8 internals.  The intent of this message is to explain to a
JavaScript developer that their pure-JS program threw an uncaught exception
-- there's no reason to reference V8 internal details at all here.

For comparison, here's a test program ("toplevel.js") and some sample
output using node v0.10, which is a little behind the current V8:
https://gist.github.com/davepacheco/0e7d2c0d1d2a14a267a6

output1.txt is the output for the code as submitted for review.
output2.txt is the output for the code as submitted, but with 'OS::Abort'
replaced with 'FATAL("aborting due to fatal exception")' (which uses
V8_Fatal).
output3.txt is the output for the code as submitted, but with
'PrintCurrentStackTrace(stderr);' replaced with
"MessageHandler::DefaultMessageReport(location, message_obj)".

I still prefer output1.txt as the best summary for a JavaScript developer.

I appreciate your taking a look at the review!

-- Dave

[Yang, 2013-04-02 10:42:30]

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

https://codereview.chromium.org/13071005/diff/1/src/isolate.cc#newcode1308
src/isolate.cc:1308: }
I was actually suggesting to replace the entire above if-block with
MessageHandler::DefaultMessageReport(this, location, message_obj);

That would print

test.js:86: Uncaught Error: my sample error

FROM:
baz (test.js:13:9)
bar (test.js:8:2)
foo (test.js:3:5)
test.js:16:1


As opposed to

UNCAUGHT EXCEPTION: 0x3f716a49 <an Error>
EXCEPTION MESSAGE: my sample error
FROM:
baz (test.js:13:9)
bar (test.js:8:2)
foo (test.js:3:5)
test.js:16:1


I just prefer using existing code that essentially prints the same content
(unless you insist on the Error object's address, in which case you could keep
the ShortPrint).

[dave.pacheco, 2013-04-04 20:30:53]

Thanks for clarifying.  I'm on the fence about not having the pointer in
the message any more, since that's usually the first thing to start with in
the debugger, but I'm okay with leaving it out.

I just tried this out, and the formatting does look a bit better, but it's
printing the wrong line number for the error.  To test it, I backported the
fix to Node 0.10.3, which is based on version 3.14.5.8.  Here's the patch I
used so that I could compare the two suggested outputs:

+
+      // If the abort-on-uncaught-exception flag is specified, abort on any
+      // exception not caught by JavaScript, even when an external handler
is
+      // present.  This flag is intended for use by JavaScript developers,
so
+      // print a user-friendly stack trace (not an internal one).
+      if (fatal_exception_depth == 0 &&
+          FLAG_abort_on_uncaught_exception &&
+          (report_exception || can_be_caught_externally)) {
+        fatal_exception_depth++;
+
+        // option 1
+        fprintf(stderr, "-------------\n");
+        fprintf(stderr, "UNCAUGHT EXCEPTION: ");
+        exception->ShortPrint(stderr);
+
+        if (IsErrorObject(exception_handle)) {
+          Handle<String> key = factory()->LookupAsciiSymbol("message");
+          MaybeObject *maybeMessage =
+            JSObject::cast(*exception_handle)->GetProperty(*key);
+          Object *messageObject;
+          if (maybeMessage->ToObject(&messageObject)) {
+            String *message = String::cast(messageObject);
+            fprintf(stderr, "\nEXCEPTION MESSAGE: ");
+            message->PrintOn(stderr);
+          }
+        }
+
+        // option 2
+        fprintf(stderr, "\n-------------\n");
+        MessageHandler::DefaultMessageReport(location, message_obj);
+        fprintf(stderr, "-------------\n");
+
+        fprintf(stderr, "\nFROM:\n");
+        PrintCurrentStackTrace(stderr);
+        OS::Abort();
+      }


and here's my script:

     1  function foo()
     2  {
     3          bar();
     4  }
     5
     6  function bar()
     7  {
     8          baz();
     9  }
    10
    11  function baz()
    12  {
    13          throw (new Error('my sample error'));
    14  }
    15
    16  foo();


and here's the output:

$ ./node --abort-on-uncaught-exception toplevel.js
-------------
UNCAUGHT EXCEPTION: b875bf1d <an Error>
EXCEPTION MESSAGE: my sample error
-------------
/home/dap/tests/toplevel.js:143: Uncaught Error: my sample error
-------------

FROM:
baz (/home/dap/tests/toplevel.js:13:9)
bar (/home/dap/tests/toplevel.js:8:2)
foo (/home/dap/tests/toplevel.js:3:2)
Object.<anonymous> (/home/dap/tests/toplevel.js:16:1)
Module._compile (module.js:456:26)
Object.Module._extensions..js (module.js:474:10)
Module.load (module.js:356:32)
Function.Module._load (module.js:312:12)
Function.Module.runMain (module.js:497:10)
startup (node.js:119:16)
node.js:901:3
Abort (core dumped)

I don't know why DefaultMessageReport reports line 143 for this 16-line
script, but I'd rather print nothing than the wrong thing (especially since
the stack below it includes the correct line number).  Thoughts?

Thanks,
Dave


On Tue, Apr 2, 2013 at 3:42 AM, <yangguo@chromium.org> wrote:

>
>
https://codereview.chromium.**org/13071005/diff/1/src/**isolate.cc<https://co...
> File src/isolate.cc (right):
>
> https://codereview.chromium.**org/13071005/diff/1/src/**
>
isolate.cc#newcode1308<https://codereview.chromium.org/13071005/diff/1/src/isolate.cc#newcode1308>
> src/isolate.cc:1308: }
> I was actually suggesting to replace the entire above if-block with
> MessageHandler::**DefaultMessageReport(this, location, message_obj);
>
> That would print
>
> test.js:86: Uncaught Error: my sample error
>
> FROM:
> baz (test.js:13:9)
> bar (test.js:8:2)
> foo (test.js:3:5)
> test.js:16:1
>
>
> As opposed to
>
> UNCAUGHT EXCEPTION: 0x3f716a49 <an Error>
> EXCEPTION MESSAGE: my sample error
> FROM:
> baz (test.js:13:9)
> bar (test.js:8:2)
> foo (test.js:3:5)
> test.js:16:1
>
>
> I just prefer using existing code that essentially prints the same
> content (unless you insist on the Error object's address, in which case
> you could keep the ShortPrint).
>
>
https://codereview.chromium.**org/13071005/<https://codereview.chromium.org/1...
>

[Yang, 2013-04-05 12:21:27]

On 2013/04/04 20:30:53, dave.pacheco wrote:
> Thanks for clarifying.  I'm on the fence about not having the pointer in
> the message any more, since that's usually the first thing to start with in
> the debugger, but I'm okay with leaving it out.
> 
> I just tried this out, and the formatting does look a bit better, but it's
> printing the wrong line number for the error.  To test it, I backported the
> fix to Node 0.10.3, which is based on version 3.14.5.8.  Here's the patch I
> used so that I could compare the two suggested outputs:
> 
> +
> +      // If the abort-on-uncaught-exception flag is specified, abort on any
> +      // exception not caught by JavaScript, even when an external handler
> is
> +      // present.  This flag is intended for use by JavaScript developers,
> so
> +      // print a user-friendly stack trace (not an internal one).
> +      if (fatal_exception_depth == 0 &&
> +          FLAG_abort_on_uncaught_exception &&
> +          (report_exception || can_be_caught_externally)) {
> +        fatal_exception_depth++;
> +
> +        // option 1
> +        fprintf(stderr, "-------------\n");
> +        fprintf(stderr, "UNCAUGHT EXCEPTION: ");
> +        exception->ShortPrint(stderr);
> +
> +        if (IsErrorObject(exception_handle)) {
> +          Handle<String> key = factory()->LookupAsciiSymbol("message");
> +          MaybeObject *maybeMessage =
> +            JSObject::cast(*exception_handle)->GetProperty(*key);
> +          Object *messageObject;
> +          if (maybeMessage->ToObject(&messageObject)) {
> +            String *message = String::cast(messageObject);
> +            fprintf(stderr, "\nEXCEPTION MESSAGE: ");
> +            message->PrintOn(stderr);
> +          }
> +        }
> +
> +        // option 2
> +        fprintf(stderr, "\n-------------\n");
> +        MessageHandler::DefaultMessageReport(location, message_obj);
> +        fprintf(stderr, "-------------\n");
> +
> +        fprintf(stderr, "\nFROM:\n");
> +        PrintCurrentStackTrace(stderr);
> +        OS::Abort();
> +      }
> 
> 
> and here's my script:
> 
>      1  function foo()
>      2  {
>      3          bar();
>      4  }
>      5
>      6  function bar()
>      7  {
>      8          baz();
>      9  }
>     10
>     11  function baz()
>     12  {
>     13          throw (new Error('my sample error'));
>     14  }
>     15
>     16  foo();
> 
> 
> and here's the output:
> 
> $ ./node --abort-on-uncaught-exception toplevel.js
> -------------
> UNCAUGHT EXCEPTION: b875bf1d <an Error>
> EXCEPTION MESSAGE: my sample error
> -------------
> /home/dap/tests/toplevel.js:143: Uncaught Error: my sample error
> -------------
> 
> FROM:
> baz (/home/dap/tests/toplevel.js:13:9)
> bar (/home/dap/tests/toplevel.js:8:2)
> foo (/home/dap/tests/toplevel.js:3:2)
> Object.<anonymous> (/home/dap/tests/toplevel.js:16:1)
> Module._compile (module.js:456:26)
> Object.Module._extensions..js (module.js:474:10)
> Module.load (module.js:356:32)
> Function.Module._load (module.js:312:12)
> Function.Module.runMain (module.js:497:10)
> startup (node.js:119:16)
> node.js:901:3
> Abort (core dumped)
> 
> I don't know why DefaultMessageReport reports line 143 for this 16-line
> script, but I'd rather print nothing than the wrong thing (especially since
> the stack below it includes the correct line number).  Thoughts?
> 
> Thanks,
> Dave
> 
> 
> On Tue, Apr 2, 2013 at 3:42 AM, <mailto:yangguo@chromium.org> wrote:
> 
> >
> >
>
https://codereview.chromium.**org/13071005/diff/1/src/**isolate.cc%3Chttps://...>
> > File src/isolate.cc (right):
> >
> > https://codereview.chromium.**org/13071005/diff/1/src/**
> >
>
isolate.cc#newcode1308<https://codereview.chromium.org/13071005/diff/1/src/isolate.cc#newcode1308>
> > src/isolate.cc:1308: }
> > I was actually suggesting to replace the entire above if-block with
> > MessageHandler::**DefaultMessageReport(this, location, message_obj);
> >
> > That would print
> >
> > test.js:86: Uncaught Error: my sample error
> >
> > FROM:
> > baz (test.js:13:9)
> > bar (test.js:8:2)
> > foo (test.js:3:5)
> > test.js:16:1
> >
> >
> > As opposed to
> >
> > UNCAUGHT EXCEPTION: 0x3f716a49 <an Error>
> > EXCEPTION MESSAGE: my sample error
> > FROM:
> > baz (test.js:13:9)
> > bar (test.js:8:2)
> > foo (test.js:3:5)
> > test.js:16:1
> >
> >
> > I just prefer using existing code that essentially prints the same
> > content (unless you insist on the Error object's address, in which case
> > you could keep the ShortPrint).
> >
> >
>
https://codereview.chromium.**org/13071005/%3Chttps://codereview.chromium.org...>
> >

I think 143 refers to the source position (index in the source string), which
has not been converted to column and line numbers yet. I agree that this is
probably misleading.

How about printing the char* result of

*MessageHandler::GetLocalizedMessage(message_obj)

to stderr instead and ignore the location since the stack trace is sufficient?

[dave.pacheco, 2013-04-08 17:06:49]

On Fri, Apr 5, 2013 at 5:21 AM, <yangguo@chromium.org> wrote:

> On 2013/04/04 20:30:53, dave.pacheco wrote:
>
>> Thanks for clarifying.  I'm on the fence about not having the pointer in
>> the message any more, since that's usually the first thing to start with
>> in
>> the debugger, but I'm okay with leaving it out.
>>
>
>  I just tried this out, and the formatting does look a bit better, but it's
>> printing the wrong line number for the error.  To test it, I backported
>> the
>> fix to Node 0.10.3, which is based on version 3.14.5.8.
>
>
> I think 143 refers to the source position (index in the source string),
> which
> has not been converted to column and line numbers yet. I agree that this is
> probably misleading.
>
> How about printing the char* result of
>
> *MessageHandler::**GetLocalizedMessage(message_**obj)
>
> to stderr instead and ignore the location since the stack trace is
> sufficient?
>


I tried this, and the results look pretty good:

Uncaught Error: my sample error

FROM
baz (/home/dap/tests/toplevel.js:13:9)
bar (/home/dap/tests/toplevel.js:8:2)
foo (/home/dap/tests/toplevel.js:3:2)
Object.<anonymous> (/home/dap/tests/toplevel.js:16:1)
Module._compile (module.js:456:26)
Object.Module._extensions..js (module.js:474:10)
Module.load (module.js:356:32)
Function.Module._load (module.js:312:12)
Function.Module.runMain (module.js:497:10)
startup (node.js:119:16)
node.js:901:3


I'll upload a new patch shortly.  Thanks again!

-- Dave

[dave.pacheco, 2013-04-08 18:08:13]

I've uploaded a new patch set based on the latest round of feedback.  Thanks.

[Yang, 2013-04-09 11:30:02]

On 2013/04/08 18:08:13, dave.pacheco wrote:
> I've uploaded a new patch set based on the latest round of feedback.  Thanks.

LGTM. Will commit.

[Yang, 2013-04-09 11:31:18]

Committed patchset #2 manually as r14185 (presubmit successful).