Heartbeat implementation of dart:mirrors.

Heartbeat implementation of dart:mirrors.

Add the skeleton for the dart:mirrors library in all the appropriate
places.  The only thing we can do right now is ask an Isolate for its
debugName.  Add a few tests.
Committed: https://code.google.com/p/dart/source/detail?r=5370

[turnidge, 2012-02-17 01:42:10]

Ivan and/or Siva -- please take a look.

Gilad -- please check to see if this is looking sane.

[gbracha, 2012-02-17 02:02:36]

lgtm

[ahe, 2012-02-17 08:09:12]

DBC

https://chromiumcodereview.appspot.com/9420038/diff/6002/lib/mirrors/mirrors....
File lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/lib/mirrors/mirrors....
lib/mirrors/mirrors.dart:11: interface IsolateMirror {
I'd like to implement these interfaces in Leg as well. Could we move this to a
shared location?

[Ivan Posva, 2012-02-17 08:21:49]

https://chromiumcodereview.appspot.com/9420038/diff/6002/lib/mirrors/mirrors....
File lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/lib/mirrors/mirrors....
lib/mirrors/mirrors.dart:11: interface IsolateMirror {
On 2012/02/17 08:09:12, ahe wrote:
> I'd like to implement these interfaces in Leg as well. Could we move this to a
> shared location?

Peter, when I talked with Todd yesterday his plan was to get a first cut of the
API going and then we move these into the right places. We are planning to do
more cleanup in the layout of libraries regardless.

[ahe, 2012-02-17 09:13:20]

DBC

https://chromiumcodereview.appspot.com/9420038/diff/6002/lib/mirrors/mirrors....
File lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/lib/mirrors/mirrors....
lib/mirrors/mirrors.dart:11: interface IsolateMirror {
On 2012/02/17 08:21:50, Ivan Posva wrote:
> On 2012/02/17 08:09:12, ahe wrote:
> > I'd like to implement these interfaces in Leg as well. Could we move this to
a
> > shared location?
> 
> Peter, when I talked with Todd yesterday his plan was to get a first cut of
the
> API going and then we move these into the right places. We are planning to do
> more cleanup in the layout of libraries regardless.

Actually, I think it already is in a shared location. Sorry for adding
confusion.

Regardless, if Todd would rather stub this out in runtime, that is no problem
short term.

[siva, 2012-02-18 01:25:55]

http://codereview.chromium.org/9420038/diff/6002/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/lib/mirrors.cc#newco...
runtime/lib/mirrors.cc:31: }
Integer& value = Integer::Handle();

over here instead of the current location.

http://codereview.chromium.org/9420038/diff/6002/runtime/lib/mirrors.cc#newco...
runtime/lib/mirrors.cc:46: writer.WriteObject(message.raw());
Can message be any random object or be restricted to just the integral types and
list/map of integral types. These messages can be potentially posted to Isolates
which are not necessarily running from the same source base and hence it may not
be possible to send any random Dart object.

http://codereview.chromium.org/9420038/diff/6002/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/lib/mirrors_impl.dar...
runtime/lib/mirrors_impl.dart:38: static void processCommand(message, replyTo) {
What is our convention with respect to parameters in library code? Do we
normally specify types for them or leave them untyped. This file seems to do
both, sometimes there are types and in this case it is untyped.

http://codereview.chromium.org/9420038/diff/6002/runtime/tests/vm/src/Isolate...
File runtime/tests/vm/src/IsolateMirrorBusyTest.dart (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/tests/vm/src/Isolate...
runtime/tests/vm/src/IsolateMirrorBusyTest.dart:9: #import('dart:mirrors');
Since mirrors are part of the corelib at least in the current implementation is
this import needed. I am not sure if it is a good thing that this works.
It gets really confusing if somebody uses a prefix here.
e.g: #import('dart:mirrors', prefix:'core_mirror');

http://codereview.chromium.org/9420038/diff/6002/runtime/tests/vm/src/Isolate...
File runtime/tests/vm/src/IsolateMirrorIdleTest.dart (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/tests/vm/src/Isolate...
runtime/tests/vm/src/IsolateMirrorIdleTest.dart:9: #import('dart:mirrors');
Ditto comment about importing mirrors.

http://codereview.chromium.org/9420038/diff/6002/runtime/tests/vm/src/Isolate...
File runtime/tests/vm/src/IsolateMirrorSelfTest.dart (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/tests/vm/src/Isolate...
runtime/tests/vm/src/IsolateMirrorSelfTest.dart:9: #import('dart:mirrors');
Ditto.

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/bootstrap.cc
File runtime/vm/bootstrap.cc (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/bootstrap.cc#newc...
runtime/vm/bootstrap.cc:65: return error.raw();
Instead of defining functions to return RawError* can we just define them to
return RawObject* that way we won't have to go through this hoop of creating yet
another handle, assigning and returning, we could just do:
if (result.IsError()) {
  return result.raw();
}
Library& lib = Library::Handle();
lib ^= result.raw();
.....
return Object::null();

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

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:888: instance ^= reader.ReadObject();
Ditto comment about any object being send in these OOB messages.

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:890: }
This function is there in dart_api_impl.cc as well probably needs to be
refactored into snapshot.h or something.

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:915: Message::kOOBPriority);
Can one do a DNS style attack here just bombarding the isolate with OOB
messages?

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

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:666: ASSERT(result.IsNull());
There seems to be some duplication here and in code_generator.cc.

Why not have a
RawObject* DartLibraryCalls::HandleMessage(Message* message, bool
oob_messages_only);
which can be then used at both places.

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:386: }
Ditto comment about code being duplicated here.

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:426: ASSERT(result.IsNull());
Ditto comment about code being duplicated here.

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9420038/diff/6002/runtime/vm/object.cc#newcode659
runtime/vm/object.cc:659: // Load other libraries, such as dart:mirrors.
Load and compile?

[turnidge, 2012-03-07 20:00:14]

I have synced and made this CL sane again.  I have changed the Bootstrap code to
more closely resemble the code that got added for dart:isolate.

I also took care of an issue in Dart_CreateIsolate that was preventing compiler
errors from being printed.

Please take another look.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors....
runtime/lib/mirrors.cc:31: }
On 2012/02/18 01:25:55, asiva wrote:
> Integer& value = Integer::Handle();
> 
> over here instead of the current location.

Are you suggesting this to save a handle allocation in the case that PortGetId
fails?  That will never happen if the system is properly configured.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors....
runtime/lib/mirrors.cc:46: writer.WriteObject(message.raw());
On 2012/02/18 01:25:55, asiva wrote:
> Can message be any random object or be restricted to just the integral types
and
> list/map of integral types. These messages can be potentially posted to
Isolates
> which are not necessarily running from the same source base and hence it may
not
> be possible to send any random Dart object.

My thought was that message would be a pretty vanilla object.  In this case, an
array of strings or other simple objects.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors_...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors_...
runtime/lib/mirrors_impl.dart:38: static void processCommand(message, replyTo) {
On 2012/02/18 01:25:55, asiva wrote:
> What is our convention with respect to parameters in library code? Do we
> normally specify types for them or leave them untyped. This file seems to do
> both, sometimes there are types and in this case it is untyped.

Don't know.  It seems like I should add types where I can, so I have.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/tests/vm/src...
File runtime/tests/vm/src/IsolateMirrorBusyTest.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/tests/vm/src...
runtime/tests/vm/src/IsolateMirrorBusyTest.dart:9: #import('dart:mirrors');
On 2012/02/18 01:25:55, asiva wrote:
> Since mirrors are part of the corelib at least in the current implementation
is
> this import needed. I am not sure if it is a good thing that this works.
> It gets really confusing if somebody uses a prefix here.
> e.g: #import('dart:mirrors', prefix:'core_mirror');

Has your opinion on this changed after the recent dart:isolate cl?

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/bootstrap.cc
File runtime/vm/bootstrap.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/bootstrap...
runtime/vm/bootstrap.cc:65: return error.raw();
On 2012/02/18 01:25:55, asiva wrote:
> Instead of defining functions to return RawError* can we just define them to
> return RawObject* that way we won't have to go through this hoop of creating
yet
> another handle, assigning and returning, we could just do:
> if (result.IsError()) {
>   return result.raw();
> }
> Library& lib = Library::Handle();
> lib ^= result.raw();
> .....
> return Object::null();

This function went away in refactor.  Point taken though.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/code_gene...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/code_gene...
runtime/vm/code_generator.cc:890: }
On 2012/02/18 01:25:55, asiva wrote:
> This function is there in dart_api_impl.cc as well probably needs to be
> refactored into snapshot.h or something.

Agreed.  As the next part of the ThreadPool stuff I am working on, I intend to
consolidate the duplicated message handling code.  Not sure if it makes sense to
do it beforehand, since it will all move anyways.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/code_gene...
runtime/vm/code_generator.cc:915: Message::kOOBPriority);
On 2012/02/18 01:25:55, asiva wrote:
> Can one do a DNS style attack here just bombarding the isolate with OOB
> messages?

You can only send OOB messages when you can already send normal messages.  So we
might argue that a hostile isolate can already cause some troubles.  Now the
fact that these are OOB maybe makes things worse though.

What do you think?

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:666: ASSERT(result.IsNull());
On 2012/02/18 01:25:55, asiva wrote:
> There seems to be some duplication here and in code_generator.cc.
> 
> Why not have a
> RawObject* DartLibraryCalls::HandleMessage(Message* message, bool
> oob_messages_only);
> which can be then used at both places.

Yes.  There is duplication.  Can I clean it up during my ThreadPool work?  I'll
be completely rewhacking message send/receive then.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/object.cc...
runtime/vm/object.cc:659: // Load other libraries, such as dart:mirrors.
On 2012/02/18 01:25:55, asiva wrote:
> Load and compile?

Changed this code to look more like how it is done for dart:isolates.

[ahe, 2012-03-08 20:14:44]

DBC

https://chromiumcodereview.appspot.com/9420038/diff/13021/lib/mirrors/mirrors...
File lib/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/13021/lib/mirrors/mirrors...
lib/mirrors/mirrors.dart:16: Future<IsolateMirror> isolateMirrorOf(SendPort
port) {
Any given isolate may have multiple mirror systems in play at the same time. For
example, an IDE may have a mirror system for talking to a remote VM and a mirror
system to analyze the source code being edited. It may also be using the mirror
system of the current VM to load extensions. This is three different mirror
systems in the same isolate. So which one can claim the top-level method?

How about putting methods like this in an interface?

[ahe, 2012-03-08 20:16:16]

Otherwise, Dart code LGTM. I totally understand if you don't want to get bogged
down in API design right now, so perhaps just add a TODO to revisit this API.

[siva, 2012-03-08 22:24:01]

lgtm

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors....
runtime/lib/mirrors.cc:31: }
In that case should you just be asserting that result is not an Error?

On 2012/03/07 20:00:14, turnidge wrote:
> On 2012/02/18 01:25:55, asiva wrote:
> > Integer& value = Integer::Handle();
> > 
> > over here instead of the current location.
> 
> Are you suggesting this to save a handle allocation in the case that PortGetId
> fails?  That will never happen if the system is properly configured.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors....
runtime/lib/mirrors.cc:46: writer.WriteObject(message.raw());
On 2012/03/07 20:00:14, turnidge wrote:
> On 2012/02/18 01:25:55, asiva wrote:
> > Can message be any random object or be restricted to just the integral types
> and
> > list/map of integral types. These messages can be potentially posted to
> Isolates
> > which are not necessarily running from the same source base and hence it may
> not
> > be possible to send any random Dart object.
> 
> My thought was that message would be a pretty vanilla object.  In this case,
an
> array of strings or other simple objects.

Ok in that case this is another place I need to remember to change when I
implement cross isolate message type.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors_...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors_...
runtime/lib/mirrors_impl.dart:38: static void processCommand(message, replyTo) {
On 2012/03/07 20:00:14, turnidge wrote:
> On 2012/02/18 01:25:55, asiva wrote:
> > What is our convention with respect to parameters in library code? Do we
> > normally specify types for them or leave them untyped. This file seems to do
> > both, sometimes there are types and in this case it is untyped.
> 
> Don't know.  It seems like I should add types where I can, so I have.


I think the convention agreed upon (though not strictly followed) was to fully
type all the interfaces but try and not specify types in the implementation
code.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/tests/vm/src...
File runtime/tests/vm/src/IsolateMirrorBusyTest.dart (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/tests/vm/src...
runtime/tests/vm/src/IsolateMirrorBusyTest.dart:9: #import('dart:mirrors');
On 2012/03/07 20:00:14, turnidge wrote:
> On 2012/02/18 01:25:55, asiva wrote:
> > Since mirrors are part of the corelib at least in the current implementation
> is
> > this import needed. I am not sure if it is a good thing that this works.
> > It gets really confusing if somebody uses a prefix here.
> > e.g: #import('dart:mirrors', prefix:'core_mirror');
> 
> Has your opinion on this changed after the recent dart:isolate cl?

We are treating 'dart:mirrors' as an external library now and not part of
corelib so this import is ok.

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:666: ASSERT(result.IsNull());
On 2012/03/07 20:00:14, turnidge wrote:
> On 2012/02/18 01:25:55, asiva wrote:
> > There seems to be some duplication here and in code_generator.cc.
> > 
> > Why not have a
> > RawObject* DartLibraryCalls::HandleMessage(Message* message, bool
> > oob_messages_only);
> > which can be then used at both places.
> 
> Yes.  There is duplication.  Can I clean it up during my ThreadPool work? 
I'll
> be completely rewhacking message send/receive then.

Ok (maybe add a TODO at the duplication points).

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
File runtime/vm/bootstrap.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
runtime/vm/bootstrap.cc:76: }
InitLibrary is a static function in this file and I don't see any references to
it, dead code?

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
File runtime/vm/bootstrap_natives.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
runtime/vm/bootstrap_natives.cc:68:
reinterpret_cast<Dart_NativeEntryResolver>(NativeLookup));
Did you change Library::CoreLibrary(), Library::CoreImplLibrary etc. to this
expanded form to avoid access to isolate?

In that case why not also change Library::IsolateLibrary() also below?

[turnidge, 2012-03-08 22:50:50]

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/lib/mirrors....
runtime/lib/mirrors.cc:31: }
On reflection, I was wrong when I said never.

This case can occur if we happen to be in the PortGetId call when another
isolate sends us a (terminating) interrupt.  So it is just very very rare.

In any case, I'll move the handle.

On 2012/03/08 22:24:02, asiva wrote:
> In that case should you just be asserting that result is not an Error?
> 
> On 2012/03/07 20:00:14, turnidge wrote:
> > On 2012/02/18 01:25:55, asiva wrote:
> > > Integer& value = Integer::Handle();
> > > 
> > > over here instead of the current location.
> > 
> > Are you suggesting this to save a handle allocation in the case that
PortGetId
> > fails?  That will never happen if the system is properly configured.
>

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/6002/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:666: ASSERT(result.IsNull());
On 2012/03/08 22:24:02, asiva wrote:
> On 2012/03/07 20:00:14, turnidge wrote:
> > On 2012/02/18 01:25:55, asiva wrote:
> > > There seems to be some duplication here and in code_generator.cc.
> > > 
> > > Why not have a
> > > RawObject* DartLibraryCalls::HandleMessage(Message* message, bool
> > > oob_messages_only);
> > > which can be then used at both places.
> > 
> > Yes.  There is duplication.  Can I clean it up during my ThreadPool work? 
> I'll
> > be completely rewhacking message send/receive then.
> 
> Ok (maybe add a TODO at the duplication points).

Done.

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
File runtime/vm/bootstrap.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
runtime/vm/bootstrap.cc:76: }
On 2012/03/08 22:24:02, asiva wrote:
> InitLibrary is a static function in this file and I don't see any references
to
> it, dead code?

Done.

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
File runtime/vm/bootstrap_natives.cc (right):

https://chromiumcodereview.appspot.com/9420038/diff/13021/runtime/vm/bootstra...
runtime/vm/bootstrap_natives.cc:68:
reinterpret_cast<Dart_NativeEntryResolver>(NativeLookup));
On 2012/03/08 22:24:02, asiva wrote:
> Did you change Library::CoreLibrary(), Library::CoreImplLibrary etc. to this
> expanded form to avoid access to isolate?
> 
> In that case why not also change Library::IsolateLibrary() also below?
> 

This is debris from before I added Library::MirrorsLibrary.  Fixed.