Add Dart_PropagateError.

Add Dart_PropagateError.

This function can be used in native functions to properly pass all
errors up the stack.

Set the long jump base in the Compiler instead of outside of the
compiler.  A bunch of errors that used to be propagated through the
sticky_error are now propagated through return values.  This includes
all of the DartEntry and DartLibraryCall functions.

In particular, we no longer use the long jump to cross dart frames.
Instead errors are propagated across dart frames using the same
mechanism that we use for unhandled exceptions.  I've added assertions
to make sure that we only use the long jump when it is "safe".
Committed: https://code.google.com/p/dart/source/detail?r=3815

[turnidge, 2012-01-27 01:20:14]

Here's another change for you to review.  Lower priority than the native ports
review I sent you.

[siva, 2012-01-31 00:52:34]

http://codereview.chromium.org/9169102/diff/11003/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:153: * If the error handle represents an unhandled
exception, this
if the handle or error handle?

http://codereview.chromium.org/9169102/diff/11003/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/lib/isolate.cc#newc...
runtime/lib/isolate.cc:102: if (result.IsInstance()) {
Why not
if (!result.IsError()) {
  PortMap::SetLive(port_id);
}
would be more readable.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode444
runtime/vm/ast.cc:444: kNoArgumentNames));
Does result have to be a ZoneHandle? We are not returning this handle from the
function.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode451
runtime/vm/ast.cc:451: ASSERT(result.IsInstance());
Why is this assert needed here doesn't the ^= assignment below have the same
assertion?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode452
runtime/vm/ast.cc:452: Instance& value = Instance::ZoneHandle();
why not call it field_value instead of value to retain the old code

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode454
runtime/vm/ast.cc:454: if (!value.IsNull()) {
The NULL check can be done before we create an Instance handle that way we save
on a handle creation.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:434: const Error& error =
Error::Handle(Compiler::CompileFunction(function));
Would it make sense to have a version of CompileFunction which calls
Exceptions::PropagateError(error) if an error has occurred?

That way this boiler plate error check code does not have to be repeated every
time.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:454: }
ASSERT(result.IsNull());

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:723: // TODO(turnidge): Is this the desired
behavior?
Yes this is the desired behaviour.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1367: Dart_Handle* result) {
Why does this have to take the result as parameter? why not just return
retval.raw() for both the error and success cases.

Also is the isolate parameter needed?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1414: return Api::NewLocalHandle(element);
Should just be:
element = GetListAt(instance, indexobj, function);
return Api::NewLocalHandle(element);

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1426: Dart_Handle* result) {
Why does this need the isolate parameter?

Also could we return Dart_Handle and not have to pass result as a parameter?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1473: SetListAt(isolate, instance, index_obj,
value_obj, function, &result);
return SetListAt(instance, index_obj, value_obj, function);

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1526: }
Ditto comment.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1581: SetListAt(isolate, instance, indexobj,
valueobj, function, &result);
Ditto.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl_te...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl_te...
runtime/vm/dart_api_impl_test.cc:131: }
Missing blank lines?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc#ne...
runtime/vm/exceptions.cc:133: void Exceptions::PropagateError(const Error&
error) {
We need an assertion here that this is called only from code which was called
from dart code. That is there are some dart frames above the current frame.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc#ne...
runtime/vm/exceptions.cc:150: CPU::JumpToErrorHandler(handler_pc, handler_sp,
handler_fp, error);
Will we get a stack trace for these error cases, probably not but it might be
useful.

I am beginning to wonder whether we should treat compile error as exceptions
similar to unhandled exceptions.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/longjump.cc
File runtime/vm/longjump.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/longjump.cc#newc...
runtime/vm/longjump.cc:28: jumpbuf_addr < isolate->top_exit_frame_info());
This code seems to assume that the stack always grows from high to low, it is
probably true for the targets we port to but it still feels weird.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7070: const Object& constructor_result =
Object::ZoneHandle(
Why does this have to be a ZoneHandle?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7324: const Object& constructor_result =
Object::ZoneHandle(
Ditto...

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7543: Object& result =
Object::ZoneHandle(Compiler::ExecuteOnce(seq));
Ditto.

[turnidge, 2012-01-31 21:56:31]

Hi Siva,

Ready for another look.

Todd

http://codereview.chromium.org/9169102/diff/11003/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:153: * If the error handle represents an unhandled
exception, this
On 2012/01/31 00:52:34, asiva wrote:
> if the handle or error handle?

Rewrote the comment a bit.

http://codereview.chromium.org/9169102/diff/11003/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/lib/isolate.cc#newc...
runtime/lib/isolate.cc:102: if (result.IsInstance()) {
On 2012/01/31 00:52:34, asiva wrote:
> Why not
> if (!result.IsError()) {
>   PortMap::SetLive(port_id);
> }
> would be more readable.

Done.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode444
runtime/vm/ast.cc:444: kNoArgumentNames));
On 2012/01/31 00:52:34, asiva wrote:
> Does result have to be a ZoneHandle? We are not returning this handle from the
> function.

Done.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode451
runtime/vm/ast.cc:451: ASSERT(result.IsInstance());
On 2012/01/31 00:52:34, asiva wrote:
> Why is this assert needed here doesn't the ^= assignment below have the same
> assertion?
 Removed.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode452
runtime/vm/ast.cc:452: Instance& value = Instance::ZoneHandle();
On 2012/01/31 00:52:34, asiva wrote:
> why not call it field_value instead of value to retain the old code

Done.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/ast.cc#newcode454
runtime/vm/ast.cc:454: if (!value.IsNull()) {
On 2012/01/31 00:52:34, asiva wrote:
> The NULL check can be done before we create an Instance handle that way we
save
> on a handle creation.

Done.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:434: const Error& error =
Error::Handle(Compiler::CompileFunction(function));
On 2012/01/31 00:52:34, asiva wrote:
> Would it make sense to have a version of CompileFunction which calls
> Exceptions::PropagateError(error) if an error has occurred?
> 
> That way this boiler plate error check code does not have to be repeated every
> time.

I sort of find the current version less magical.  The caller has to make a
conscious choice about how to handle the error rather than deciding to call the
"void" CompileFunction in a strange context and not considering how to handle
the error.

Is it okay to leave it like this for now and change it later if it is
problematic?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:454: }
On 2012/01/31 00:52:34, asiva wrote:
> ASSERT(result.IsNull());

Why should the result be null?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:723: // TODO(turnidge): Is this the desired
behavior?
On 2012/01/31 00:52:34, asiva wrote:
> Yes this is the desired behaviour.

Thanks.  Updated comment.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1367: Dart_Handle* result) {
On 2012/01/31 00:52:34, asiva wrote:
> Why does this have to take the result as parameter? why not just return
> retval.raw() for both the error and success cases.
> 
> Also is the isolate parameter needed?

Got rid of GetListAt.  It made more trouble than it was worth.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1414: return Api::NewLocalHandle(element);
On 2012/01/31 00:52:34, asiva wrote:
> Should just be:
> element = GetListAt(instance, indexobj, function);
> return Api::NewLocalHandle(element);

Reworked this.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1426: Dart_Handle* result) {
On 2012/01/31 00:52:34, asiva wrote:
> Why does this need the isolate parameter?
> 
> Also could we return Dart_Handle and not have to pass result as a parameter?

Got rid of SetListAt as well.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1473: SetListAt(isolate, instance, index_obj,
value_obj, function, &result);
On 2012/01/31 00:52:34, asiva wrote:
> return SetListAt(instance, index_obj, value_obj, function);

Code has changed.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1526: }
On 2012/01/31 00:52:34, asiva wrote:
> Ditto comment.

Code changed.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1581: SetListAt(isolate, instance, indexobj,
valueobj, function, &result);
On 2012/01/31 00:52:34, asiva wrote:
> Ditto.

Code changed.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl_te...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/dart_api_impl_te...
runtime/vm/dart_api_impl_test.cc:131: }
On 2012/01/31 00:52:34, asiva wrote:
> Missing blank lines?

Added lines.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc#ne...
runtime/vm/exceptions.cc:133: void Exceptions::PropagateError(const Error&
error) {
On 2012/01/31 00:52:34, asiva wrote:
> We need an assertion here that this is called only from code which was called
> from dart code. That is there are some dart frames above the current frame.

Added.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc#ne...
runtime/vm/exceptions.cc:150: CPU::JumpToErrorHandler(handler_pc, handler_sp,
handler_fp, error);
It might be helpful to have a stack trace in these cases, yes.  That's a nice
idea.

Can I plan on changing this later?  I plan on doing more work around
isolate-death and having a death-stack-trace could be part of that work.

For this CL, the semantics of an Error are the same as they were before -- skip
dart frames and do not run finally blocks.  If we wanted to start running
finally blocks on compilation errors, would it involve spec changes?

On 2012/01/31 00:52:34, asiva wrote:
> Will we get a stack trace for these error cases, probably not but it might be
> useful.
> 
> I am beginning to wonder whether we should treat compile error as exceptions
> similar to unhandled exceptions.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/longjump.cc
File runtime/vm/longjump.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/longjump.cc#newc...
runtime/vm/longjump.cc:28: jumpbuf_addr < isolate->top_exit_frame_info());
On 2012/01/31 00:52:34, asiva wrote:
> This code seems to assume that the stack always grows from high to low, it is
> probably true for the targets we port to but it still feels weird.

I also make this assumption in the stack overflow handler in code_generator.cc.

Do you want me to document this?

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7070: const Object& constructor_result =
Object::ZoneHandle(
On 2012/01/31 00:52:34, asiva wrote:
> Why does this have to be a ZoneHandle?

I was cribbing code here.  Fixed.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7324: const Object& constructor_result =
Object::ZoneHandle(
On 2012/01/31 00:52:34, asiva wrote:
> Ditto...

Fixed.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:7543: Object& result =
Object::ZoneHandle(Compiler::ExecuteOnce(seq));
On 2012/01/31 00:52:34, asiva wrote:
> Ditto.

Fixed.  Also changed for Error below.

[siva, 2012-02-01 19:09:38]

LGTM

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:434: const Error& error =
Error::Handle(Compiler::CompileFunction(function));
On 2012/01/31 21:56:31, turnidge wrote:
> On 2012/01/31 00:52:34, asiva wrote:
> > Would it make sense to have a version of CompileFunction which calls
> > Exceptions::PropagateError(error) if an error has occurred?
> > 
> > That way this boiler plate error check code does not have to be repeated
every
> > time.
> 
> I sort of find the current version less magical.  The caller has to make a
> conscious choice about how to handle the error rather than deciding to call
the
> "void" CompileFunction in a strange context and not considering how to handle
> the error.
> 
> Is it okay to leave it like this for now and change it later if it is
> problematic?

Sure.

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:454: }
On 2012/01/31 21:56:31, turnidge wrote:
> On 2012/01/31 00:52:34, asiva wrote:
> > ASSERT(result.IsNull());
> 
> Why should the result be null?


You are right the result does not have to be NULL, I was thinking of the compile
case where it either succeeds or fails, but this is used more for checking the
result of a dart function invocation which can return some other result object

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/exceptions.cc#ne...
runtime/vm/exceptions.cc:150: CPU::JumpToErrorHandler(handler_pc, handler_sp,
handler_fp, error);
On 2012/01/31 21:56:31, turnidge wrote:
> It might be helpful to have a stack trace in these cases, yes.  That's a nice
> idea.
> 
> Can I plan on changing this later?  I plan on doing more work around
> isolate-death and having a death-stack-trace could be part of that work.
> 
> For this CL, the semantics of an Error are the same as they were before --
skip
> dart frames and do not run finally blocks.  If we wanted to start running
> finally blocks on compilation errors, would it involve spec changes?
> 
> On 2012/01/31 00:52:34, asiva wrote:
> > Will we get a stack trace for these error cases, probably not but it might
be
> > useful.
> > 
> > I am beginning to wonder whether we should treat compile error as exceptions
> > similar to unhandled exceptions.
> 


Yes I agree we should plan on any enhancements to error reporting in another CL
not this one,

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/longjump.cc
File runtime/vm/longjump.cc (right):

http://codereview.chromium.org/9169102/diff/11003/runtime/vm/longjump.cc#newc...
runtime/vm/longjump.cc:28: jumpbuf_addr < isolate->top_exit_frame_info());
On 2012/01/31 21:56:31, turnidge wrote:
> On 2012/01/31 00:52:34, asiva wrote:
> > This code seems to assume that the stack always grows from high to low, it
is
> > probably true for the targets we port to but it still feels weird.
> 
> I also make this assumption in the stack overflow handler in
code_generator.cc.
> 
> Do you want me to document this?

Maybe a comment stating that this code assumes the stack grows from high t low
would be fine.

http://codereview.chromium.org/9169102/diff/17001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

http://codereview.chromium.org/9169102/diff/17001/runtime/vm/exceptions.cc#ne...
runtime/vm/exceptions.cc:133: void Exceptions::PropagateError(const Error&
error) {
Not for this CL but maybe changing the signature of this to
Exceptions::PropagateError(const Object& obj);
might simplify the invocation points which always
do a
Error& error = Error::Handle();
error ^ result.raw();
PropogateError(error);