Use the return value of vm native methods to set the return value,

Use the return value of vm native methods to set the return value,
based on Siva's earlier suggestion (he actually suggested putting it
in the generated stub, which I haven't done).

Added SetReturnUnsafe and use it exactly one place so far.
Committed: https://code.google.com/p/dart/source/detail?r=11633

[turnidge, 2012-08-24 23:36:03]

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/array.cc
File lib/array.cc (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/array.cc#newcode58
lib/array.cc:58: return Object::null();
I could add some sort of macro for this like

RETURN_DART_VOID;

What do you think?

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/code_generator.cc
File vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/code_generator.cc#n...
vm/code_generator.cc:795: arguments.SetReturn(code);
Just something I noticed.  I didn't change DEFINE_RUNTIME_ENTRY in this change,
so it still uses SetReturn.

[Ivan Posva, 2012-08-27 18:20:37]

First round of comments, some of them we already discussed.

-Ivan

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/array.cc
File lib/array.cc (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/array.cc#newcode58
lib/array.cc:58: return Object::null();
On 2012/08/24 23:36:03, turnidge wrote:
> I could add some sort of macro for this like
> 
> RETURN_DART_VOID;
> 
> What do you think?

How many void returning native entries do we have? I doesn't feel as if it is a
common enough occurrence to warrant special handling.

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/error.cc
File lib/error.cc (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/error.cc#newcode72
lib/error.cc:72: return Object::null();
Add an UNREACHABLE before this line like above?

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_arguments.h
File vm/native_arguments.h (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_arguments.h#...
vm/native_arguments.h:71: void SetReturnUnsafe(RawObject* value) const;
Can we make this a private and do some friend magic instead of purely relying on
the comment?

[turnidge, 2012-08-28 18:08:21]

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_arguments.h
File vm/native_arguments.h (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_arguments.h#...
vm/native_arguments.h:71: void SetReturnUnsafe(RawObject* value) const;
On 2012/08/27 18:20:37, Ivan Posva wrote:
> Can we make this a private and do some friend magic instead of purely relying
on
> the comment?

I tried it but it got a little bit involved.

In order to declare the functions as friend declarations, we need access to the
BOOTSTRAP_NATIVE_LIST in bootstrap_natives.h.  Because that file also includes
the forward declarations of the native entries, it includes native_entry.h  And,
in turn, native_entry.h includes native_arguments.h in order to get the
definition of the NativeArguments type (it needs the full definition because it
is passed by value).

So, to resolve this, I would need to do some refactoring.  I would probably need
to split the BOOTSTRAP_NATIVE_LIST macro and the native entry forward
declarations into two separate headers.  I started to do this, but I decided I
should ping you back to see what you'd like me to do.

[siva, 2012-08-28 18:21:20]

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_entry.h
File vm/native_entry.h (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_entry.h#newc...
vm/native_entry.h:48: DN_Helper##name(arguments->isolate(), arguments));        
          \
I was also proposing that we set the return value in the stub code so that there
would be no need for this SetReturnUnsafe function.

These native functions would return Raw<type>* just like all regular VM
functions and the stub code has access to arguments area it would just store the
return value into that return area of arguments.

Only if the VERIFY_ON_TRANSITION FLAG is on we would need to  store the return
value in a handle do the verification and then return.

[turnidge, 2012-08-29 20:36:49]

Hi Ivan,

I think I've addressed your comments.  A fair number of new files were added to
the change.  Please take a look.

Todd

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/error.cc
File lib/error.cc (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/lib/error.cc#newcode72
lib/error.cc:72: return Object::null();
On 2012/08/27 18:20:37, Ivan Posva wrote:
> Add an UNREACHABLE before this line like above?

Done.

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_arguments.h
File vm/native_arguments.h (right):

https://chromiumcodereview.appspot.com/10874072/diff/1/vm/native_arguments.h#...
vm/native_arguments.h:71: void SetReturnUnsafe(RawObject* value) const;
As you suggested offline, I've created a BootstrapNatives class containing all
of the native entries.  That made the friend declaration quite a bit simpler.

On 2012/08/28 18:08:21, turnidge wrote:
> On 2012/08/27 18:20:37, Ivan Posva wrote:
> > Can we make this a private and do some friend magic instead of purely
relying
> on
> > the comment?
> 
> I tried it but it got a little bit involved.
> 
> In order to declare the functions as friend declarations, we need access to
the
> BOOTSTRAP_NATIVE_LIST in bootstrap_natives.h.  Because that file also includes
> the forward declarations of the native entries, it includes native_entry.h 
And,
> in turn, native_entry.h includes native_arguments.h in order to get the
> definition of the NativeArguments type (it needs the full definition because
it
> is passed by value).
> 
> So, to resolve this, I would need to do some refactoring.  I would probably
need
> to split the BOOTSTRAP_NATIVE_LIST macro and the native entry forward
> declarations into two separate headers.  I started to do this, but I decided I
> should ping you back to see what you'd like me to do.
>

[Ivan Posva, 2012-08-30 00:52:39]

LGTM -ip

https://chromiumcodereview.appspot.com/10874072/diff/25003/vm/bootstrap_nativ...
File vm/bootstrap_natives.h (right):

https://chromiumcodereview.appspot.com/10874072/diff/25003/vm/bootstrap_nativ...
vm/bootstrap_natives.h:199: class BootstrapNatives {
: public AllStatic