Read platform.dill in the VM.

Read platform.dill in the VM.

1. A --platform flag is added to dart to give a path to a Kernel
   binary for the platform libraries (as produced by building the
   runtime_kernel target).

2. This binary is used for bootstrapping.  Since it contains libraries
   other then the VM's bootstrap libraries, they are also loaded.

3. The frontend does not send any library with a dart: import URI
   scheme.  Note that it does not (yet) prune the canonical name
   table, which will contain a lot of unnecessary names used for
   internal linkages in the platform libraries.

4. There is a single dependency in the platform libraries on the
   script: _getMainClosure in dart:_builtin.  This is patched after
   the script is loaded.

BUG=
R=ahe@google.com, kustermann@google.com, vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/385f8fb054e85d642d7570291ad02eef1af34b8f

[Kevin Millikin (Google), 2017-03-31 10:05:41]

kmillikin@google.com changed reviewers:
+ kustermann@google.com, vegorov@google.com

[Kevin Millikin (Google), 2017-03-31 10:05:42]

https://codereview.chromium.org/2786083002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2786083002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:155: class FrontEndBinaryPrinter extends
BinaryPrinter {
This is hacky and I'm sure there is a better way.  I'll figure out what we
really want to do for now before committing.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:149: // There is a function _getMainClosure in
dart:_builtin that returns the
I'm aware that this is ugly.  It would be better to either get rid of
_getMainClosure and just look up main in the root library, or else make
_getMainClosure a native so it's OK to have a 'magical' implementation.

[ahe, 2017-03-31 10:13:08]

ahe@google.com changed reviewers:
+ ahe@google.com

[ahe, 2017-03-31 10:13:10]

lgtm

https://codereview.chromium.org/2786083002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2786083002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:155: class FrontEndBinaryPrinter extends
BinaryPrinter {
I think this would be a better fit for the "kernel" subdirectory.

https://codereview.chromium.org/2786083002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:155: class FrontEndBinaryPrinter extends
BinaryPrinter {
How about renaming this FilteringBinaryPrinter and add this method:

bool includeLibrary(Library library) => !library.importUri.isScheme('dart');

https://codereview.chromium.org/2786083002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2786083002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:270:
continuation.transformProgram(program);
Can this be moved to otherTransformations?

https://codereview.chromium.org/2786083002/diff/20001/tools/testing/dart/comp...
File tools/testing/dart/compiler_configuration.dart (right):

https://codereview.chromium.org/2786083002/diff/20001/tools/testing/dart/comp...
tools/testing/dart/compiler_configuration.dart:217:
args.add('--platform=${buildDir}/patched_sdk/platform.dill');
How about creating an outline of the platform file and passing that to Fasta?

[ahe, 2017-03-31 10:13:40]

I pressed LGTM too quickly: I only looked at the Dart code. Someone else should
review the C++ changes.

[Kevin Millikin (Google), 2017-04-05 09:11:46]

Slava or Martin, do you want to look at the VM changes here?

[kustermann, 2017-04-05 11:47:07]

lgtm

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:892: return NULL;
Maybe 'delete isolate_data' here.

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:931: kernel_platform != NULL
Consider adding another case for 'kernel_program != NULL': If it still works, we
might want to keep support for it and add a test. It's easy to remove the
support if we have a reason to break it.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:981: void set_body(Statement* body) { body_ = body; }
This only works if body_ is NULL before (Child<Statement>::operator= will assert
it), I guess this is always the case?

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:155:
Function::Handle(builtin_library.LookupFunctionAllowPrivate(
Handle(Z,

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:159: CanonicalName* name = CanonicalName::NewRoot();
This will leak [name], right?

Please add a comment about it.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:161: main_library->name()->size()));
This "new String()" will never be freed afaik! (also for the other cases below).

Please add a comment about it.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:169: procedure->function()->set_body(new
ReturnStatement(new StaticGet(name)));
I assume the function has already a body (like nsm call) and you replace it with
the static tearoff, right?

In that case: set_body() will hit an ASSERT because it uses "Child<Statement>
body_" and

                                                    
 138 template <typename T>
 ...
 139 class Child {
 147   T*& operator=(T* value) {
 148     ASSERT(pointer_ == NULL);
 149     return pointer_ = value;
 150   }

[Vyacheslav Egorov (Google), 2017-04-05 12:21:23]

lgtm 

I think we need to rigorously document which combinations of --platform, --dfe
and input files (.dart / .dill input scripts) we support and ensure that these
configurations are tested. This is a multidimensional matrix:

1) --platform present/absent.
2) --dfe present/absent
3) input script is Dart, input script is linked Kernel binary, input script is
unlinked Kernel binary

I think for situations when input script is fully linked Kernel binary we should
consider bootstrapping from it, instead of dropping libraries to the floor.

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:349: use_platform_binary = true;
Should not this platform binary be also passed down to DFE isolate and
subsequently Fasta to ensure that we are compiling and bootstrapping against the
same SDK?

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:876: IsolateData* isolate_data =
Was there any reason to move isolate_data creation up here? Moving it down to
where it was declared before allows to avoid memory leak.

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:912: return NULL;
This leaks isolate data and kernel_platform.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:149: // There is a function _getMainClosure in
dart:_builtin that returns the
On 2017/03/31 10:05:42, Kevin Millikin (Google) wrote:
> I'm aware that this is ugly.  It would be better to either get rid of
> _getMainClosure and just look up main in the root library, or else make
> _getMainClosure a native so it's OK to have a 'magical' implementation.

We would also need to come up with solution for the future in which the flow
graph builder is streaming from Binary and AST nodes do not exist anymore.

https://codereview.chromium.org/2786083002/diff/20001/tools/testing/dart/comp...
File tools/testing/dart/compiler_configuration.dart (right):

https://codereview.chromium.org/2786083002/diff/20001/tools/testing/dart/comp...
tools/testing/dart/compiler_configuration.dart:631:
args.add('--dfe=utils/kernel-service/kernel-service.dart');
this needs to be modified as well.

[Kevin Millikin (Google), 2017-04-25 18:04:32]

Description was changed from

==========
Read platform.dill in the VM.

1. A --platform flag is added to dart to give a path to a Kernel
   binary for the platform libraries (as produced by building the
   runtime_kernel target).

2. This binary is used for bootstrapping.  Since it contains libraries
   other then the VM's bootstrap libraries, they are also loaded.

3. The frontend does not send any library with a dart: import URI
   scheme.  Note that it does not (yet) prune the canonical name
   table, which will contain a lot of unnecessary names used for
   internal linkages in the platform libraries.

4. There is a single dependency in the platform libraries on the
   script: _getMainClosure in dart:_builtin.  This is patched after
   the script is loaded.

BUG=
==========

to

==========
Read platform.dill in the VM.

1. A --platform flag is added to dart to give a path to a Kernel
   binary for the platform libraries (as produced by building the
   runtime_kernel target).

2. This binary is used for bootstrapping.  Since it contains libraries
   other then the VM's bootstrap libraries, they are also loaded.

3. The frontend does not send any library with a dart: import URI
   scheme.  Note that it does not (yet) prune the canonical name
   table, which will contain a lot of unnecessary names used for
   internal linkages in the platform libraries.

4. There is a single dependency in the platform libraries on the
   script: _getMainClosure in dart:_builtin.  This is patched after
   the script is loaded.

BUG=
R=ahe@google.com, kustermann@google.com, vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/385f8fb054e85d642d7570291ad02eef1af34b8f
==========

[Kevin Millikin (Google), 2017-04-25 18:04:33]

Committed patchset #3 (id:40001) manually as
385f8fb054e85d642d7570291ad02eef1af34b8f (presubmit successful).

[Kevin Millikin (Google), 2017-04-25 18:20:30]

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:349: use_platform_binary = true;
On 2017/04/05 12:21:23, Vyacheslav Egorov (Google) wrote:
> Should not this platform binary be also passed down to DFE isolate and
> subsequently Fasta to ensure that we are compiling and bootstrapping against
the
> same SDK?

Yes, let's add that in a separate change.

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:876: IsolateData* isolate_data =
On 2017/04/05 12:21:23, Vyacheslav Egorov (Google) wrote:
> Was there any reason to move isolate_data creation up here? Moving it down to
> where it was declared before allows to avoid memory leak.

I used it earlier in an intermediate state, but you are right that I can and
should move it back where it was.  Done.

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:892: return NULL;
On 2017/04/05 11:47:07, kustermann wrote:
> Maybe 'delete isolate_data' here.

Thank you.  Well spotted.

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:912: return NULL;
On 2017/04/05 12:21:23, Vyacheslav Egorov (Google) wrote:
> This leaks isolate data and kernel_platform.

Thank you.  Well spotted.  I delete kernel_platform here now.

https://codereview.chromium.org/2786083002/diff/20001/runtime/bin/main.cc#new...
runtime/bin/main.cc:931: kernel_platform != NULL
On 2017/04/05 11:47:07, kustermann wrote:
> Consider adding another case for 'kernel_program != NULL': If it still works,
we
> might want to keep support for it and add a test. It's easy to remove the
> support if we have a reason to break it.

Good suggestion, left for a separate change.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:981: void set_body(Statement* body) { body_ = body; }
On 2017/04/05 11:47:07, kustermann wrote:
> This only works if body_ is NULL before (Child<Statement>::operator= will
assert
> it), I guess this is always the case?

Yes.  We could assert, but since operator= does it isn't necessary.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:149: // There is a function _getMainClosure in
dart:_builtin that returns the
On 2017/04/05 12:21:23, Vyacheslav Egorov (Google) wrote:
> On 2017/03/31 10:05:42, Kevin Millikin (Google) wrote:
> > I'm aware that this is ugly.  It would be better to either get rid of
> > _getMainClosure and just look up main in the root library, or else make
> > _getMainClosure a native so it's OK to have a 'magical' implementation.
> 
> We would also need to come up with solution for the future in which the flow
> graph builder is streaming from Binary and AST nodes do not exist anymore.

I know, and those changes started landing before this one.  I've updated this to
work by special casing StaticGet what doesn't come from the binary.

Agreed we need a better solution for the long term.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:155:
Function::Handle(builtin_library.LookupFunctionAllowPrivate(
On 2017/04/05 11:47:07, kustermann wrote:
> Handle(Z, 

Done.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:161: main_library->name()->size()));
On 2017/04/05 11:47:07, kustermann wrote:
> This "new String()" will never be freed afaik! (also for the other cases
below).
> 
> Please add a comment about it.

Comment added that we are leaking the whole body.

https://codereview.chromium.org/2786083002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:169: procedure->function()->set_body(new
ReturnStatement(new StaticGet(name)));
On 2017/04/05 11:47:07, kustermann wrote:
> I assume the function has already a body (like nsm call) and you replace it
with
> the static tearoff, right?
> 
> In that case: set_body() will hit an ASSERT because it uses "Child<Statement>
> body_" and
> 
>                                                     
>  138 template <typename T>
>  ...
>  139 class Child {
>  147   T*& operator=(T* value) {
>  148     ASSERT(pointer_ == NULL);
>  149     return pointer_ = value;
>  150   }
> 

It doesn't have a body, it's treated as if it were external by special casing in
the front end.

https://codereview.chromium.org/2786083002/diff/20001/tools/testing/dart/comp...
File tools/testing/dart/compiler_configuration.dart (right):

https://codereview.chromium.org/2786083002/diff/20001/tools/testing/dart/comp...
tools/testing/dart/compiler_configuration.dart:217:
args.add('--platform=${buildDir}/patched_sdk/platform.dill');
On 2017/03/31 10:13:09, ahe wrote:
> How about creating an outline of the platform file and passing that to Fasta?

That's an excellent suggestion.  Let's discuss how to do it.

https://codereview.chromium.org/2786083002/diff/20001/tools/testing/dart/comp...
tools/testing/dart/compiler_configuration.dart:631:
args.add('--dfe=utils/kernel-service/kernel-service.dart');
On 2017/04/05 12:21:23, Vyacheslav Egorov (Google) wrote:
> this needs to be modified as well.

Done.