Add an API function to get a debugger stack trace from an error handle.

Add an API function to get a debugger stack trace from an error handle.

BUG=http://dartbug.com/14322
R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30323

[rmacnak, 2013-10-30 16:52:44]

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15435: // How to inform API users?
frame_count++ if we indicate the gap. See comment below.

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15485: // How to inform API users?
Function named '...' at -1, -1?

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15503: *function_name =
function.QualifiedUserVisibleName();
Might allocate.

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15521: *function_name =
function.QualifiedUserVisibleName();
Might allocate.

[siva, 2013-10-31 17:13:26]

As discussed offline, I am wondering if it makes sense to something like what
the debugger does by creating a C++ stack trace object which can be handed out
through the API and one could query individual frames using this object. This
would ensure we don't keep iterating the frames for every call.

https://codereview.chromium.org/51793002/diff/50001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/51793002/diff/50001/runtime/include/dart_api....
runtime/include/dart_api.h:286: * \param handle A Stacktrace.
maybe call it handle instead of stacktrace.

https://codereview.chromium.org/51793002/diff/50001/runtime/include/dart_api....
runtime/include/dart_api.h:297: * \param handle A Stacktrace.
Ditto comment.

https://codereview.chromium.org/51793002/diff/50001/runtime/include/dart_api....
runtime/include/dart_api.h:299: *     position 0.
position of the frame in the stacktrace?

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15376: // Check for null function object, which indicates a
stack trace overflow.
A null function object doesn't necessarily mean a stack trace overflow, the
original comment tries to convey that.

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15435: // How to inform API users?
On 2013/10/30 16:52:45, Ryan Macnak wrote:
> frame_count++ if we indicate the gap. See comment below.

Since you are handing out actual frames maybe the API implementation should not
try and truncate stuff. The embedder code using the API could try and figure out
how it wants to handle long stack traces.
We should probably do the truncating stuff in the dartium bindings code.

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15437: } else if (function.is_visible() ||
FLAG_verbose_stacktrace) {
We probably don't want the verbose_stacktrace flag to influence this, we should
only show visible frames.

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15485: // How to inform API users?
On 2013/10/30 16:52:45, Ryan Macnak wrote:
> Function named '...' at -1, -1?

Ditto comment as above, the API implementation should not bother with this.

[rmacnak, 2013-11-07 21:31:11]

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15376: // Check for null function object, which indicates a
stack trace overflow.
On 2013/10/31 17:13:26, siva wrote:
> A null function object doesn't necessarily mean a stack trace overflow, the
> original comment tries to convey that.

Reverted.

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15435: // How to inform API users?
On 2013/10/31 17:13:26, siva wrote:
> On 2013/10/30 16:52:45, Ryan Macnak wrote:
> > frame_count++ if we indicate the gap. See comment below.
> 
> Since you are handing out actual frames maybe the API implementation should
not
> try and truncate stuff. The embedder code using the API could try and figure
out
> how it wants to handle long stack traces.
> We should probably do the truncating stuff in the dartium bindings code.

The truncation for StackOverflow happens before this code runs. Otherwise, this
should be a full trace.

https://codereview.chromium.org/51793002/diff/50001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:15437: } else if (function.is_visible() ||
FLAG_verbose_stacktrace) {
On 2013/10/31 17:13:26, siva wrote:
> We probably don't want the verbose_stacktrace flag to influence this, we
should
> only show visible frames.

Removed.

https://codereview.chromium.org/51793002/diff/290001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/51793002/diff/290001/runtime/include/dart_api...
runtime/include/dart_api.h:202: typedef struct _Dart_ExceptionStackTrace*
Dart_ExceptionStackTrace;
This is a bad name considering the Dart_CurrentStacktrace below.

This one should be called Dart_StackTrace, and the one in the debugger should be
renamed Dart_Stack as it is a token for querying a live stack, not a trace.

https://codereview.chromium.org/51793002/diff/290001/runtime/include/dart_api...
runtime/include/dart_api.h:287: DART_EXPORT Dart_Handle Dart_ErrorGetStacktrace(
Note this no longer provides access to dart:core.Stacktrace object.  (It can
still be retrieved using GetField on the exception, but it doesn't provided any
information beyond what can be gotten here.)

https://codereview.chromium.org/51793002/diff/290001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/51793002/diff/290001/runtime/vm/debugger.h#ne...
runtime/vm/debugger.h:312: ExceptionStackTrace* BuildExceptionStackTrace();
Ditto bad name.

https://codereview.chromium.org/51793002/diff/290001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/51793002/diff/290001/runtime/vm/object.h#newc...
runtime/vm/object.h:6075: class ExceptionStackTrace;
Move to top of file?

https://codereview.chromium.org/51793002/diff/290001/runtime/vm/object.h#newc...
runtime/vm/object.h:6108: ExceptionStackTrace* BuildExceptionStackTrace() const;
Ditto bad name.

[rmacnak, 2013-11-07 21:31:29]

Goes with https://codereview.chromium.org/55433007/.

[siva, 2013-11-10 05:39:12]

As discussed offline with Mathias we should merge the debugger stack trace API
functions with your changes that way we won't have the conflicts you mention.

[rmacnak, 2013-11-12 00:05:16]

+Mathias

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:153: EXPECT_EQ(51, frame_count);
Broken after refactoring to share with the debugger. Claims to have a stack
trace of length 0. Strange because the equivalent use in Dartium still works.

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1285: return (stack_trace_ != NULL) ? stack_trace_ :
CollectStackTrace();
This does not appear to get cleared by resuming execution, only by other
debugger functions.

[siva, 2013-11-12 16:00:42]

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
File runtime/include/dart_debugger_api.h (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:370: * \return A handle to the True object
if no error occurs.
why a handle to the True object and not Success like all other API functions.

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:426: intptr_t* library_id);
I don't find any use cases of this API function anywhere other than in some
debugger tests, why is this version needed?

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:443: * \return A handle to the True object
if no error occurs.
Ditto comment regarding returning Success.

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:450: intptr_t* column_number);
If we decide to get rid of the other version of ActivationFrameInfo function
then this can be named Dart_ActivationFrameInfo

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:153: EXPECT_EQ(51, frame_count);
On 2013/11/12 00:05:16, Ryan Macnak wrote:
> Broken after refactoring to share with the debugger. Claims to have a stack
> trace of length 0. Strange because the equivalent use in Dartium still works.

If you print the stacktrace object returned what do you see?

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1285: return (stack_trace_ != NULL) ? stack_trace_ :
CollectStackTrace();
On 2013/11/12 00:05:16, Ryan Macnak wrote:
> This does not appear to get cleared by resuming execution, only by other
> debugger functions.

Sounds like a bug

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1288: DebuggerStackTrace* Debugger::UncachedStackTrace()
{
why not call it CurrentStackTrace() ?

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1308: // Missing frames in preallocated StackOverflow or
OutOfMemory.
Add an ASSERT here? or should the condition you are testing above be an assert?

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger_api_...
File runtime/vm/debugger_api_impl.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger_api_...
runtime/vm/debugger_api_impl.cc:248: return Api::NewError("This error is not an
unhandled exception error.");
Why are you not getting the stack trace corresponding to the error object? All
error objects have a stack trace.

You could unwrap into an error object:

const Error& error = Api::UnwrapErrorHandle(isolate, handle);
if (error.IsNull()) {
  return Api::NewError("Can only ......");
}
...
...

[rmacnak, 2013-11-12 22:32:40]

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
File runtime/include/dart_debugger_api.h (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:370: * \return A handle to the True object
if no error occurs.
On 2013/11/12 16:00:43, siva wrote:
> why a handle to the True object and not Success like all other API functions.

It does return Success. Copied from doc for Dart_GetStackTrace, which is also
out of sync. Updated both.

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:426: intptr_t* library_id);
On 2013/11/12 16:00:43, siva wrote:
> I don't find any use cases of this API function anywhere other than in some
> debugger tests, why is this version needed?

Indeed, even the debugger tests ignore the position info. Removed.

https://codereview.chromium.org/51793002/diff/410001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:450: intptr_t* column_number);
On 2013/11/12 16:00:43, siva wrote:
> If we decide to get rid of the other version of ActivationFrameInfo function
> then this can be named Dart_ActivationFrameInfo

Done.

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:153: EXPECT_EQ(51, frame_count);
On 2013/11/12 16:00:43, siva wrote:
> On 2013/11/12 00:05:16, Ryan Macnak wrote:
> > Broken after refactoring to share with the debugger. Claims to have a stack
> > trace of length 0. Strange because the equivalent use in Dartium still
works.

Woops, this comment was supposed to be attached to the very similar
CurrentStacktraceInfo test below.
 
> If you print the stacktrace object returned what do you see?

It's empty. Presumably the stack is unwound to hand the Error handle back to the
C++ code, so it shouldn't have worked in the earlier iteration. I've refactored
the test so it runs in a native called by the Dart code and the stack is still
live. This mimics its use in Dartium.

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1288: DebuggerStackTrace* Debugger::UncachedStackTrace()
{
On 2013/11/12 16:00:43, siva wrote:
> why not call it CurrentStackTrace() ?

Done.

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:1308: // Missing frames in preallocated StackOverflow or
OutOfMemory.
On 2013/11/12 16:00:43, siva wrote:
> Add an ASSERT here? or should the condition you are testing above be an
assert?

Should be an assert.

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger_api_...
File runtime/vm/debugger_api_impl.cc (right):

https://codereview.chromium.org/51793002/diff/410001/runtime/vm/debugger_api_...
runtime/vm/debugger_api_impl.cc:248: return Api::NewError("This error is not an
unhandled exception error.");
On 2013/11/12 16:00:43, siva wrote:
> Why are you not getting the stack trace corresponding to the error object? All
> error objects have a stack trace.
> 
> You could unwrap into an error object:
> 
> const Error& error = Api::UnwrapErrorHandle(isolate, handle);
> if (error.IsNull()) {
>   return Api::NewError("Can only ......");
> }
> ...
> ...

This matches the behavior of Dart_ErrorGetStacktrace. It would be strange for
this path to be exercised. Someone would have to catch an error, and then pass
it into a native. I'd rather not add such functionality as part of this CL.

[hausner, 2013-11-12 23:24:39]

DBC

https://codereview.chromium.org/51793002/diff/490001/runtime/include/dart_deb...
File runtime/include/dart_debugger_api.h (right):

https://codereview.chromium.org/51793002/diff/490001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:372: DART_EXPORT Dart_Handle
Dart_GetStackTraceFromError(Dart_Handle handle,
Name the parameter error?

https://codereview.chromium.org/51793002/diff/490001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:410: * \col_number receives the line number
in the script, or -1 if column
column number

https://codereview.chromium.org/51793002/diff/490001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/51793002/diff/490001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl.cc:4253: library.set_debuggable(true);
Don't change the default please. Only the toplevel library (aka script) should
be debuggable by default. It is the debug client's job to implement a policy as
to what else should be debuggable.

https://codereview.chromium.org/51793002/diff/490001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/51793002/diff/490001/runtime/vm/debugger.h#ne...
runtime/vm/debugger.h:314: DebuggerStackTrace* CurrentStackTrace();
Add one or two lines of comment to describe what the functions do?

[rmacnak, 2013-11-14 20:00:18]

Sync'd with the change to debugger frame filtering.

Removed removal from Dartium's dart:html so this can be committed independently.

https://codereview.chromium.org/51793002/diff/490001/runtime/include/dart_deb...
File runtime/include/dart_debugger_api.h (right):

https://codereview.chromium.org/51793002/diff/490001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:372: DART_EXPORT Dart_Handle
Dart_GetStackTraceFromError(Dart_Handle handle,
On 2013/11/12 23:24:39, hausner wrote:
> Name the parameter error?

Done.

https://codereview.chromium.org/51793002/diff/490001/runtime/include/dart_deb...
runtime/include/dart_debugger_api.h:410: * \col_number receives the line number
in the script, or -1 if column
On 2013/11/12 23:24:39, hausner wrote:
> column number

Done.

https://codereview.chromium.org/51793002/diff/490001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/51793002/diff/490001/runtime/vm/debugger.h#ne...
runtime/vm/debugger.h:314: DebuggerStackTrace* CurrentStackTrace();
On 2013/11/12 23:24:39, hausner wrote:
> Add one or two lines of comment to describe what the functions do?

Done.

[siva, 2013-11-15 23:03:27]

LGTM with one comment.

https://codereview.chromium.org/51793002/diff/820001/runtime/vm/debugger_api_...
File runtime/vm/debugger_api_impl.cc (right):

https://codereview.chromium.org/51793002/diff/820001/runtime/vm/debugger_api_...
runtime/vm/debugger_api_impl.cc:248: return Api::NewError("This error is not an
unhandled exception error.");
We should probably just have
if (obj.IsError()) {
  ...
} else {
  return Api::newError(...);
]

[rmacnak, 2013-11-15 23:42:31]

Committed patchset #17 manually as r30323 (presubmit successful).