VM support for running Kernel binaries.

VM support for running Kernel binaries.

BUG=
R=asiva@google.com, fschneider@google.com

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

[Vyacheslav Egorov (Google), 2016-10-11 18:07:09]

vegorov@google.com changed reviewers:
+ asiva@google.com

[Vyacheslav Egorov (Google), 2016-10-11 18:07:09]

This is preliminary CL for running Kernel binaries so that people can start
evaluating it and sending their comments and concerns.

[Cutch, 2016-10-11 18:40:10]

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

[Cutch, 2016-10-11 18:40:12]

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/dartutils.cc#ne...
runtime/bin/dartutils.cc:497: const uint8_t*
DartUtils::SniffForMagicNumber(const uint8_t* text_buffer,
Alternatively this could take the magic pattern as an argument.

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/gen_snapshot.cc
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:490: void *file = DartUtils::OpenFile(script_name,
false);
Why is this change here? You're not respecting the resolved url or going through
the loader. The rest of the code is the same as LoadUrlContents..

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:1349:
SetupStubNativeResolversForPrecompilation(entry_points);
This is common to both paths. Why not move it out of the branches?

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

https://codereview.chromium.org/2411823003/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2810: * Loads a dart application which was compiled
to a dill file
\param buffer A buffer which contains the contents of a dill file.
\param buffer_len Length of the passed in buffer.

\return If no error occurs, the ....

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:117: if (graph != NULL) return graph;
ASSERT(graph != NULL); ? Or are we supposed to fallback to the old front end?

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1734: // kImplicitStaticFinalGetter is used for both
implicit static getters
Could this be factored into a DilParser::ParseStaticFieldInitializer so that it
matches the Parser::ParseStaticFieldInitializer structure?

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

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:5412: // TODO(kustermann): Memory leak!
please fix before committing...

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.cc
File runtime/vm/dil.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.cc#newcode730
runtime/vm/dil.cc:730: }
two blank lines between functions (here and elsewhere)

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h
File runtime/vm/dil.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h#newcode127
runtime/vm/dil.h:127: namespace dil {
we don't use a lot of nested namespaces in the VM codebase.  Is there some
reason why this code couldn't exist inside the dart namespace?

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc
File runtime/vm/dil_binary.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc#ne...
runtime/vm/dil_binary.cc:29: explicit InputFile(const char* filename)
The code for InputFile and OutputFile should probably live inside bin/

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc#ne...
runtime/vm/dil_binary.cc:134: static const uint32_t kMagicProgramFile =
0x90ABCDEFu;
might be nice if this was expressed in the same way as it is in gen_snapshot {
0x90, ... }

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc#ne...
runtime/vm/dil_binary.cc:301: 
two blank lines..

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_main.cc
File runtime/vm/dil_main.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_main.cc#newc...
runtime/vm/dil_main.cc:10: int main(int argc, char** argv) {
This should be in bin/

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/hash_map.h
File runtime/vm/hash_map.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/hash_map.h#newco...
runtime/vm/hash_map.h:369: class ProperPointerKeyValueTrait {
define "Proper" or better yet, rename this class.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:872: intptr_t dil_function_;
can this be folded into another field? Maybe "data_"  or "owner_" ?

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:968: intptr_t dil_field_;
similarly...

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object_snaps...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:824: field.set_dil_field(0);
please define a named constant for this sentinel and use it when comparing
elsewhere, i.e. (dil_field() != kInvalidDilIndex)

[Florian Schneider, 2016-10-11 19:50:32]

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

[Florian Schneider, 2016-10-11 19:50:33]

First round of comments

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:90: bool UseDilFrontEndFor(ParsedFunction*
parsed_function) {
I think this should be constant a compile-time constant, either always false, or
always true. It would simplify the code in many places.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:93: (function.kind() ==
RawFunction::kNoSuchMethodDispatcher) ||
In AOT we don't generate this kind of function.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:94: (function.kind() ==
RawFunction::kInvokeFieldDispatcher);
This kind is generated in AOT for certain fields (.call and fields that have a
function-type type annotation -- likely used as callback). It should be
supported in both front-ends.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2604: __ movq(CTX, Address(RBP,
closure_parameter->index() * kWordSize));
Saving/restoring the context should explicitly be done at the flow-graph level.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:872: intptr_t dil_function_;
Please refactor the code so that there are no back-pointers from heap objecs to
the AST. These won't be needed at runtime, but I don't see the reason they are
needed at compile-time either.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
File runtime/vm/redundancy_elimination.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
runtime/vm/redundancy_elimination.cc:3359: if
(!catch_entry->exception_var().is_captured()) {
Why do these variables now become captured?

[Kevin Millikin (Google), 2016-10-12 12:11:46]

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

[Kevin Millikin (Google), 2016-10-12 12:11:47]

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1734: // kImplicitStaticFinalGetter is used for both
implicit static getters
On 2016/10/11 18:40:11, Cutch wrote:
> Could this be factored into a DilParser::ParseStaticFieldInitializer so that
it
> matches the Parser::ParseStaticFieldInitializer structure?

There is nothing to parse, we just need to build the flow graph.  We should
probably find a way to share some code here, but I expect the
Binary->Kernel->Flow-graph translation to have some churn over the next quarter
or two so it might be premature to worry about it.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:872: intptr_t dil_function_;
On 2016/10/11 19:50:33, Florian Schneider wrote:
> Please refactor the code so that there are no back-pointers from heap objecs
to
> the AST. These won't be needed at runtime, but I don't see the reason they are
> needed at compile-time either.

It's the "token position".

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
File runtime/vm/redundancy_elimination.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
runtime/vm/redundancy_elimination.cc:3359: if
(!catch_entry->exception_var().is_captured()) {
On 2016/10/11 19:50:33, Florian Schneider wrote:
> Why do these variables now become captured?

The parser introduces a pair of exception (also, stack trace) variables.  One is
never captured, the other is possibly captured, and we end up emitting:

:exception_var = exception register  // never captured
e = :exception_var  // this is possibly captured

In the Kernel compiler we eliminated the redundant variable, but that means the
one variable we have might be captured (there is also code in
CatchBlockEntryInstr::EmitNativeCode to correctly store into the context in case
it is captured).

[Florian Schneider, 2016-10-12 16:17:37]

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h
File runtime/vm/dil.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h#newcode1
runtime/vm/dil.h:1: // Copyright (c) 2016, the Dart project authors.  Please see
the AUTHORS file
Please make sure that nothing from dil*.h/.cc ends up linked into
dart_precompiled_runtime to keep snapshot size from growing.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2604: __ movq(CTX, Address(RBP,
closure_parameter->index() * kWordSize));
On 2016/10/11 19:50:33, Florian Schneider wrote:
> Saving/restoring the context should explicitly be done at the flow-graph
level.

I wonder if we ever end up restoring the context twice. Once into CTX here, and
again into current_context_var later.

If it's not a strict code size reduction, I'd prefer CatchBlockEntryInstr
unchanged.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2625: CTX,
Is restoring the context above only needed if this variable is captured?
i.e. is exception_var().is_captured() equivalent to
should_restore_closure_context()?

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:872: intptr_t dil_function_;
On 2016/10/12 12:11:47, Kevin Millikin (Google) wrote:
> On 2016/10/11 19:50:33, Florian Schneider wrote:
> > Please refactor the code so that there are no back-pointers from heap objecs
> to
> > the AST. These won't be needed at runtime, but I don't see the reason they
are
> > needed at compile-time either.
> 
> It's the "token position".

The uses of this reinterpret_cast this into some form of Node*.

I'm asking to avoid making objects in raw_object.h any bigger.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
File runtime/vm/redundancy_elimination.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
runtime/vm/redundancy_elimination.cc:3359: if
(!catch_entry->exception_var().is_captured()) {
On 2016/10/12 12:11:47, Kevin Millikin (Google) wrote:
> On 2016/10/11 19:50:33, Florian Schneider wrote:
> > Why do these variables now become captured?
> 
> The parser introduces a pair of exception (also, stack trace) variables.  One
is
> never captured, the other is possibly captured, and we end up emitting:
> 
> :exception_var = exception register  // never captured
> e = :exception_var  // this is possibly captured
> 
> In the Kernel compiler we eliminated the redundant variable, but that means
the
> one variable we have might be captured (there is also code in
> CatchBlockEntryInstr::EmitNativeCode to correctly store into the context in
case
> it is captured).

I see. For this you needed to add code to restore the context in the
CatchBlockEntry. Then the context is restored to :current_context_var again
later (dil_to_il.cc:2029)

The optimization may not be worth doing, if more code is generated in the catch
block entry instead. So unless the savings are substantial, I'd just not do this
optimization.

[Vyacheslav Egorov (Google), 2016-10-12 16:45:57]

I have addressed some of the comments. Please send more.

I am looking into two things right now:

- ensuring that dart_precompiled_runtime does not pull in any Kernel stuff.

- ensuring that we compile on Windows (which means changing dil_binary and
making file reading platform independent).

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/dartutils.cc#ne...
runtime/bin/dartutils.cc:497: const uint8_t*
DartUtils::SniffForMagicNumber(const uint8_t* text_buffer,
On 2016/10/11 18:40:11, Cutch wrote:
> Alternatively this could take the magic pattern as an argument.

Refactored this code.

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/gen_snapshot.cc
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:490: void *file = DartUtils::OpenFile(script_name,
false);
On 2016/10/11 18:40:11, Cutch wrote:
> Why is this change here? You're not respecting the resolved url or going
through
> the loader. The rest of the code is the same as LoadUrlContents..

Historical. There was some DIL file handling here at some point - then this
change was reverted but not cleanly/entirely... Reverted back to
LoadUrlContents.

https://codereview.chromium.org/2411823003/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:1349:
SetupStubNativeResolversForPrecompilation(entry_points);
On 2016/10/11 18:40:11, Cutch wrote:
> This is common to both paths. Why not move it out of the branches?

Done. Though the code became more obscure now.

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

https://codereview.chromium.org/2411823003/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:2810: * Loads a dart application which was compiled
to a dill file
On 2016/10/11 18:40:11, Cutch wrote:
> \param buffer A buffer which contains the contents of a dill file.
> \param buffer_len Length of the passed in buffer.
> 
> \return If no error occurs, the ....

Done.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:90: bool UseDilFrontEndFor(ParsedFunction*
parsed_function) {
On 2016/10/11 19:50:32, Florian Schneider wrote:
> I think this should be constant a compile-time constant, either always false,
or
> always true. It would simplify the code in many places.

I can't be a compile time constant right now - this CL only brings enough
changes to run in *hybrid* mode where core libraries are boostraped from source,
while app itself can be either Source or Kernel. 

CL that brings enough changes to run purely Kernel is larger - we will bring
those changes in separately.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:93: (function.kind() ==
RawFunction::kNoSuchMethodDispatcher) ||
On 2016/10/11 19:50:32, Florian Schneider wrote:
> In AOT we don't generate this kind of function.

Kernel is not AOT only. It supports JIT mode as well (and the plan is that JIT
mode from Kernel would eventually replace the JIT mode from the source).

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:94: (function.kind() ==
RawFunction::kInvokeFieldDispatcher);
On 2016/10/11 19:50:32, Florian Schneider wrote:
> This kind is generated in AOT for certain fields (.call and fields that have a
> function-type type annotation -- likely used as callback). It should be
> supported in both front-ends.

Well, in reality front-end-s should have nothing to do with these kind of
functions as they have no source. The current setup with Parser synthesizing a
fake AST for them is a bizarre historical artifact. They should just be built by
some auxiliary graph builder somewhere.

In fact that's what DIL "front-end" ends up doing. Instead of FakeAST -> Graph
-> ... dance it justs directly builds the flow graph - which seems to be a much
cleaner solution.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:117: if (graph != NULL) return graph;
On 2016/10/11 18:40:11, Cutch wrote:
> ASSERT(graph != NULL); ? Or are we supposed to fallback to the old front end?

Done.

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

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:5412: // TODO(kustermann): Memory leak!
On 2016/10/11 18:40:11, Cutch wrote:
> please fix before committing...

This is unlikely to cause anybody any issues.

Right now this is only a leak if you create and destroy isolates each running
Kernel code in a loop. I am not aware about any production code that does that. 

There are no memory implications for pure-source VM mode.

It will certainly be addressed once we start working on bridging the parity gap
between Kernel and Source front-ends, e.g. for hot reload from Kernel it's
essential not to leak unused Kernel binaries.

Does this sound reasonable to you?

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.cc
File runtime/vm/dil.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.cc#newcode730
runtime/vm/dil.cc:730: }
On 2016/10/11 18:40:11, Cutch wrote:
> two blank lines between functions (here and elsewhere)

Done.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h
File runtime/vm/dil.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h#newcode1
runtime/vm/dil.h:1: // Copyright (c) 2016, the Dart project authors.  Please see
the AUTHORS file
On 2016/10/12 16:17:36, Florian Schneider wrote:
> Please make sure that nothing from dil*.h/.cc ends up linked into
> dart_precompiled_runtime to keep snapshot size from growing.

I will check that.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h#newcode127
runtime/vm/dil.h:127: namespace dil {
On 2016/10/11 18:40:11, Cutch wrote:
> we don't use a lot of nested namespaces in the VM codebase.  Is there some
> reason why this code couldn't exist inside the dart namespace?

There are naming conflicts, things like dil::String and dart::String are
different things. 

Google C++ style guide encourages use of namespaces to avoid name clashes.
(https://google.github.io/styleguide/cppguide.html#Namespaces).

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc
File runtime/vm/dil_binary.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc#ne...
runtime/vm/dil_binary.cc:29: explicit InputFile(const char* filename)
On 2016/10/11 18:40:12, Cutch wrote:
> The code for InputFile and OutputFile should probably live inside bin/

I am looking at porting this code into bin/ or platform/ and rewriting it to
support Windows.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc#ne...
runtime/vm/dil_binary.cc:134: static const uint32_t kMagicProgramFile =
0x90ABCDEFu;
On 2016/10/11 18:40:12, Cutch wrote:
> might be nice if this was expressed in the same way as it is in gen_snapshot {
> 0x90, ... }
> 

Checking it is much nicer when it's a single number though.

Maybe we should consider expressing magic in gen_snapshot as uint32_t as well?

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_binary.cc#ne...
runtime/vm/dil_binary.cc:301: 
On 2016/10/11 18:40:12, Cutch wrote:
> two blank lines..

Done.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_main.cc
File runtime/vm/dil_main.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil_main.cc#newc...
runtime/vm/dil_main.cc:10: int main(int argc, char** argv) {
On 2016/10/11 18:40:12, Cutch wrote:
> This should be in bin/

Removed this file - it's just a benchmark.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/hash_map.h
File runtime/vm/hash_map.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/hash_map.h#newco...
runtime/vm/hash_map.h:369: class ProperPointerKeyValueTrait {
On 2016/10/11 18:40:12, Cutch wrote:
> define "Proper" or better yet, rename this class.

Renamed to RawPointer which seems to be a better name.

(PointerKeyValueTrait is the one which is incorrectly named... Naming it
correctly is hard though... It should have been called something like
HashableKeyValueTrait because it expects T to define Hashcode and Equals).

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2604: __ movq(CTX, Address(RBP,
closure_parameter->index() * kWordSize));
On 2016/10/11 19:50:33, Florian Schneider wrote:
> Saving/restoring the context should explicitly be done at the flow-graph
level.

This particular restoration can't be done on the flow-graph level because
exception/stack variables management below needs access to the current context.

Moving this stuff to the flow graph level would require us to introduce
artificial non-captured exception_var() and stacktrace_var() - which are later
copied into actual captured stack trace and exception var. This seems somewhat
convoluted. In the Kernel front-end we have just a single exception/stacktrace
var without anything artificial. But as a result they can be captured (e.g. by
await within catch). This code deals with that.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2625: CTX,
On 2016/10/12 16:17:36, Florian Schneider wrote:
> Is restoring the context above only needed if this variable is captured?
> i.e. is exception_var().is_captured() equivalent to
> should_restore_closure_context()?

Not entirely. If any of the artificial variables are captured - context needs to
be restored. 

Also having a separate flag allows for more assertions (e.g. we can check that
we are not capturing something when we don't expect it).

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:872: intptr_t dil_function_;
On 2016/10/11 18:40:12, Cutch wrote:
> can this be folded into another field? Maybe "data_"  or "owner_" ?

It can't be folded into owner_, because DIL functions need to have correct
owner_ set up.

In the end it will be folded into token_pos_ + end_token_pos_

Right now RawRunction is approximately ~13 words. Adding one word is <10%
increase of an instance size. In dart2js before it starts compiling anything
RawFunction object occupies 181.7 KB out of 12 MB heap so it's ~2% with this
change. Folding this field away would save .2% of the heap - which does not
amount to much. (note: if I take snapshot of dart2js while it is compiling the
number would be even smaller).

I think temporary .2% increase in memory consumption is fine. In the end this
field will take place of token_pos_ as noted before so this .2% increase will
disappear.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:872: intptr_t dil_function_;
On 2016/10/12 12:11:47, Kevin Millikin (Google) wrote:
> On 2016/10/11 19:50:33, Florian Schneider wrote:
> > Please refactor the code so that there are no back-pointers from heap objecs
> to
> > the AST. These won't be needed at runtime, but I don't see the reason they
are
> > needed at compile-time either.
> 
> It's the "token position".

As Kevin points out these are no different from token_pos_. There needs to be a
way to find AST somehow. I am gonna make it NOT_IN_PRECOMPILED though.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:872: intptr_t dil_function_;
On 2016/10/12 16:17:36, Florian Schneider wrote:
> On 2016/10/12 12:11:47, Kevin Millikin (Google) wrote:
> > On 2016/10/11 19:50:33, Florian Schneider wrote:
> > > Please refactor the code so that there are no back-pointers from heap
objecs
> > to
> > > the AST. These won't be needed at runtime, but I don't see the reason they
> are
> > > needed at compile-time either.
> > 
> > It's the "token position".
> 
> The uses of this reinterpret_cast this into some form of Node*.
> 
> I'm asking to avoid making objects in raw_object.h any bigger.

Please see move replies above. I made it NOT_IN_PRECOMPILED to reduce the size
of precompiled runtime. Growth in JIT mode is neglectable (see numbers above),
especially if you keep in mind that this pointer will become a new token
position.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:968: intptr_t dil_field_;
On 2016/10/11 18:40:12, Cutch wrote:
> similarly...

see above

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object_snaps...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:824: field.set_dil_field(0);
On 2016/10/11 18:40:12, Cutch wrote:
> please define a named constant for this sentinel and use it when comparing
> elsewhere, i.e. (dil_field() != kInvalidDilIndex)

Done.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
File runtime/vm/redundancy_elimination.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/redundancy_elimi...
runtime/vm/redundancy_elimination.cc:3359: if
(!catch_entry->exception_var().is_captured()) {
> 
> I see. For this you needed to add code to restore the context in the
> CatchBlockEntry. Then the context is restored to :current_context_var again
> later (dil_to_il.cc:2029)
> 

It's a different context level being restored there. The code at CatchBlockEntry
restores raw closure context (context_level == 0) so that LoadLocal were able to
load CurrentCatchContext() from the context. 

This whole machinery exists for the sake of async functions - it's the only case
when auxiliary variables like catch_context_x could be captured - e.g. if you
have await inside the try { ... }.

If you would prefer we can walk you through our implementation of async over a
VC.

In general it is somewhat simpler then VMs and has less auxiliary variables -
but we had to add the code to CatchBlockEntry to achieve that.

[Florian Schneider, 2016-10-12 17:30:46]

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2604: __ movq(CTX, Address(RBP,
closure_parameter->index() * kWordSize));
On 2016/10/12 16:45:57, Vyacheslav Egorov (Google) wrote:
> On 2016/10/11 19:50:33, Florian Schneider wrote:
> > Saving/restoring the context should explicitly be done at the flow-graph
> level.
> 
> This particular restoration can't be done on the flow-graph level because
> exception/stack variables management below needs access to the current
context.
> 
> Moving this stuff to the flow graph level would require us to introduce
> artificial non-captured exception_var() and stacktrace_var() - which are later
> copied into actual captured stack trace and exception var. This seems somewhat
> convoluted. In the Kernel front-end we have just a single exception/stacktrace
> var without anything artificial. But as a result they can be captured (e.g. by
> await within catch). This code deals with that. 

In general I'm all for removing extra compiler-generated locals, if the result
is smaller generated code. Here I see code added, that's why I'm wondering if
this is actually an improvement and/or a bug fix of broken behavior in the
existing implementation.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2618: __ movq(Address(RBP, context_index
* kWordSize), CTX);
I wonder if this store is dead code?

[rmacnak, 2016-10-13 00:56:38]

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

[rmacnak, 2016-10-13 00:56:39]

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/bin/dart...
File runtime/bin/dartutils.h (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/bin/dart...
runtime/bin/dartutils.h:41: bool TryReadKernel(const char* script_uri,
General question: Why does the embedder code call this "Kernel" but the vm code
calls this "DIL"?

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/compi...
File runtime/vm/compiler.cc (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/compi...
runtime/vm/compiler.cc:1728: StackZone _(thread);
StackZone stack_zone(thread);
Zone* zone = stack_zone.GetZone();

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil.h
File runtime/vm/dil.h (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil.h...
runtime/vm/dil.h:8: #include <stdint.h>
Remove. Provided by globals.h

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil.h...
runtime/vm/dil.h:273: memcpy(buffer_, utf8, length);  // NOLINT
memmove, lint

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_b...
File runtime/vm/dil_binary.cc (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_b...
runtime/vm/dil_binary.cc:40: int fd = TEMP_FAILURE_RETRY(open(filename_,
O_RDONLY));
IO belongs in the embedder, not the VM.

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_b...
runtime/vm/dil_binary.cc:495: fprintf(stderr, "@%ld %s\n", offset_, str);
OS::PrintErr("@%" Pd64 %s\n", ...)

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_b...
runtime/vm/dil_binary.cc:678: fprintf(stderr, "@%ld %s\n", offset_, str);
OS::PrintErr

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_r...
File runtime/vm/dil_reader.h (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_r...
runtime/vm/dil_reader.h:8: #include <map>
We don't use stl in the VM.

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_t...
File runtime/vm/dil_to_il.h (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/dil_t...
runtime/vm/dil_to_il.h:246: virtual void VisitDefaultDartType(DartType* node) {
UNREACHABLE(); }
Don't implement virtual functions in headers. I believe this is a error with the
compiler flags used by Mojo.

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/raw_o...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/raw_o...
runtime/vm/raw_object.h:220: kNoDilNode = 0
NULL

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/raw_o...
runtime/vm/raw_object.h:876: NOT_IN_PRECOMPILED(intptr_t dil_function_);
void* ?

I think it would make more sense to have a alternate version of Script that
pointed to some DIL instead of a TokenStream, rather than make any heap objects
bigger.

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/raw_o...
runtime/vm/raw_object.h:972: intptr_t dil_field_;
void* ?

[Kevin Millikin (Google), 2016-10-13 04:29:41]

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/bin/dart...
File runtime/bin/dartutils.h (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/bin/dart...
runtime/bin/dartutils.h:41: bool TryReadKernel(const char* script_uri,
On 2016/10/13 00:56:39, rmacnak wrote:
> General question: Why does the embedder code call this "Kernel" but the vm
code
> calls this "DIL"?

It mostly historical.  We called it DIL (Dart Intermediate Language) until we
came up with the name Kernel which is better.

At one point I started changing references to DIL but mkustermann@ argued
against doing it at that time.  This is probably the time.  We do like the
namespace name dil because it's so short.  (We've toyed with the ides that we
could have a name for the IR's tree representation (Kernel) and another name for
its binary format (DIL), but I don't really think that's an important
distinction.)

[Kevin Millikin (Google), 2016-10-13 04:38:35]

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/raw_o...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/vm/raw_o...
runtime/vm/raw_object.h:876: NOT_IN_PRECOMPILED(intptr_t dil_function_);
On 2016/10/13 00:56:39, rmacnak wrote:
> void* ?
> 
> I think it would make more sense to have a alternate version of Script that
> pointed to some DIL instead of a TokenStream, rather than make any heap
objects
> bigger.

That's a good idea.  We do  have that. jenj@ is currently changing our treatment
of scripts as part of adding source positions for stack traces, debugging, and
profiling and a new script kind is part of those changes.

We have postponed shrinking instances by reusing fields, but now is probably a
good time to do that.

[Cutch, 2016-10-13 13:59:12]

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

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:5412: // TODO(kustermann): Memory leak!
On 2016/10/12 16:45:56, Vyacheslav Egorov (Google) wrote:
> On 2016/10/11 18:40:11, Cutch wrote:
> > please fix before committing...
> 
> This is unlikely to cause anybody any issues.
> 
> Right now this is only a leak if you create and destroy isolates each running
> Kernel code in a loop. I am not aware about any production code that does
that. 
> 
> There are no memory implications for pure-source VM mode.
> 
> It will certainly be addressed once we start working on bridging the parity
gap
> between Kernel and Source front-ends, e.g. for hot reload from Kernel it's
> essential not to leak unused Kernel binaries.
> 
> Does this sound reasonable to you?

What makes fixing this memory leak so difficult that we can't do it before
merging?

[Cutch, 2016-10-13 14:01:47]

On 2016/10/13 04:29:41, Kevin Millikin (Google) wrote:
>
https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/bin/dart...
> File runtime/bin/dartutils.h (right):
> 
>
https://chromiumcodereview.appspot.com/2411823003/diff/20001/runtime/bin/dart...
> runtime/bin/dartutils.h:41: bool TryReadKernel(const char* script_uri,
> On 2016/10/13 00:56:39, rmacnak wrote:
> > General question: Why does the embedder code call this "Kernel" but the vm
> code
> > calls this "DIL"?
> 
> It mostly historical.  We called it DIL (Dart Intermediate Language) until we
> came up with the name Kernel which is better.
> 

Calling it "Kernel" is one of the most confusing naming choices I've ever come
across.

"A kernel is the central part of an operating system."

[Vyacheslav Egorov (Google), 2016-10-13 14:37:58]

I addressed most of the comments. 

Please take another look. 

- I have ensured that no Kernel stuff gets pulled into precompiled snapshot. 

- I have removed platform dependent code from the kernel_binary.cc

- Have renamed DIL to Kernel everywhere to ensure consistency.

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

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:5412: // TODO(kustermann): Memory leak!
On 2016/10/13 13:59:12, Cutch wrote:
> On 2016/10/12 16:45:56, Vyacheslav Egorov (Google) wrote:
> > On 2016/10/11 18:40:11, Cutch wrote:
> > > please fix before committing...
> > 
> > This is unlikely to cause anybody any issues.
> > 
> > Right now this is only a leak if you create and destroy isolates each
running
> > Kernel code in a loop. I am not aware about any production code that does
> that. 
> > 
> > There are no memory implications for pure-source VM mode.
> > 
> > It will certainly be addressed once we start working on bridging the parity
> gap
> > between Kernel and Source front-ends, e.g. for hot reload from Kernel it's
> > essential not to leak unused Kernel binaries.
> > 
> > Does this sound reasonable to you?
> 
> What makes fixing this memory leak so difficult that we can't do it before
> merging?

I can change your question into: What makes this leak so important that we can't
do it after merge? 

I did not say it is difficult. 

It requires figuring out how we want to represent and store Kernel binary within
Dart VM heap, e.g. it can be gigantic typed array, then GC will manage it for
us. It can be some external storage that is tied to isolate, etc.

I just don't want to invest time in this refactoring before we merge, because
nobody will be affected by this leak.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2604: __ movq(CTX, Address(RBP,
closure_parameter->index() * kWordSize));
> In general I'm all for removing extra compiler-generated locals, if the result
> is smaller generated code. Here I see code added, that's why I'm wondering if
> this is actually an improvement and/or a bug fix of broken behavior in the
> existing implementation.

As I tried to explain before the code is not added. It's moved around to a more
reasonable place.

Please see VM's AwaitTransformer::VisitAwaitNode. It builds the following code:

  //   :saved_try_ctx_var = :await_saved_try_ctx_var_y;

(Where :saved_try_ctx_var is stack local and :await_saved_try_ctx_var_y is
variable in the context).

Instead of having one special context variable that is manually synced with the
local variable after each await point, we simply allowed to capture
:saved_try_ctx_var. However to access it later we need to have correct context
in the :current_context when we enter catch. This is what this code is doing. It
sets :current_context to 0 level context (closure context) - the same way the
entry does.


Futhermore the VM current implementation is even more convoluted. For some
reason it saves current and outer try context (which does not make much sense to
me and which our implementation does not need to do AFAIK - precisely due to
this simplification).

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2618: __ movq(Address(RBP, context_index
* kWordSize), CTX);
On 2016/10/12 17:30:46, Florian Schneider wrote:
> I wonder if this store is dead code?

It is not. Subsequent code will use :current_context to load :saved_try_ctx_var.

https://codereview.chromium.org/2411823003/diff/20001/runtime/bin/dartutils.h
File runtime/bin/dartutils.h (right):

https://codereview.chromium.org/2411823003/diff/20001/runtime/bin/dartutils.h...
runtime/bin/dartutils.h:41: bool TryReadKernel(const char* script_uri,
Renamed all DIL to Kernel.

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1728: StackZone _(thread);
On 2016/10/13 00:56:39, rmacnak wrote:
> StackZone stack_zone(thread);
> Zone* zone = stack_zone.GetZone();

Done.

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

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil.h#newcode8
runtime/vm/dil.h:8: #include <stdint.h>
On 2016/10/13 00:56:39, rmacnak wrote:
> Remove. Provided by globals.h

Done.

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil.h#newcod...
runtime/vm/dil.h:273: memcpy(buffer_, utf8, length);  // NOLINT
On 2016/10/13 00:56:39, rmacnak wrote:
> memmove, lint

Done.

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil_binary.cc
File runtime/vm/dil_binary.cc (right):

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil_binary.c...
runtime/vm/dil_binary.cc:40: int fd = TEMP_FAILURE_RETRY(open(filename_,
O_RDONLY));
On 2016/10/13 00:56:39, rmacnak wrote:
> IO belongs in the embedder, not the VM.

Deleted this code.

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil_binary.c...
runtime/vm/dil_binary.cc:495: fprintf(stderr, "@%ld %s\n", offset_, str);
On 2016/10/13 00:56:39, rmacnak wrote:
> OS::PrintErr("@%" Pd64 %s\n", ...)

Done.

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil_binary.c...
runtime/vm/dil_binary.cc:678: fprintf(stderr, "@%ld %s\n", offset_, str);
On 2016/10/13 00:56:39, rmacnak wrote:
> OS::PrintErr

Done.

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

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil_reader.h...
runtime/vm/dil_reader.h:8: #include <map>
On 2016/10/13 00:56:39, rmacnak wrote:
> We don't use stl in the VM.

It's not completely true: assert.h includes sstream and string.

GC used to use multimap at some point (does not anymore though).

I can see some reasons for not using STL (e.g. our longjmp-s can lead to memory
leaks), some platforms might not have a good STL implementation, etc.

We can eradicate <map> & <vector> after landing. 

[Once you realize that our embedders (e.g. Flutter) are more likely to use STL
than not use it the decision to not use STL ourselves and instead hand roll
various crippled alternatives does not look very sound. We live in 2016 but
still use Napoleon's C++ from 1812]

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

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/dil_to_il.h#...
runtime/vm/dil_to_il.h:246: virtual void VisitDefaultDartType(DartType* node) {
UNREACHABLE(); }
On 2016/10/13 00:56:39, rmacnak wrote:
> Don't implement virtual functions in headers. I believe this is a error with
the
> compiler flags used by Mojo.

$ cd runtime/vm
$ git grep "virtual .* {" -- *.h | wc -l
997

If these flags would be set for Dart VM we would have a lot of problems.

I am not gonna fix this now given that existing VM code violates it majorly.

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

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:220: kNoDilNode = 0
On 2016/10/13 00:56:39, rmacnak wrote:
> NULL

Good point. Done.

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:876: NOT_IN_PRECOMPILED(intptr_t dil_function_);
I made it void*.

We can try to fold it into Script later.

As said multiple times before - increase in heap consumption is negligible and
it will go away later.

https://codereview.chromium.org/2411823003/diff/20001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:972: intptr_t dil_field_;
On 2016/10/13 00:56:39, rmacnak wrote:
> void* ?

Done. Also made it NOT_IN_PRECOMPILED.

[siva, 2016-10-13 23:02:59]

lgtm with some comments.

https://codereview.chromium.org/2411823003/diff/40001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/bin/dartutils.c...
runtime/bin/dartutils.cc:77: // Do not free buffer if we this is a kernel file -
kernel_file will be
'if we this' doesn't sound right.

Maybe add a comment that a free of the buffer is done after Dart_LoadKernel is
done.

https://codereview.chromium.org/2411823003/diff/40001/runtime/bin/dartutils.c...
runtime/bin/dartutils.cc:615: UNREACHABLE();
Why UNREACHABLE here and not
result = DartUtils::NewError("%s is not a valid UTF-8 script",
resolved_script_url);

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

https://codereview.chromium.org/2411823003/diff/40001/runtime/include/dart_ap...
runtime/include/dart_api.h:2813: * \param length Length of the passed in buffer.
buffer_len not length

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5419: // TODO(kustermann): Memory leak!
In general we try to avoid TODO with individual names, could you file a tracking
issue and add the issue number here in the TODO.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5426: library.set_debuggable(false);
I presume this is temporary as we would want to debug libraries loaded this way
too.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:765: parsed_function,
alignment?

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

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:221: int length() { return length_; }
int length() const { return length_; }

I have noticed that 'const' is not used on all such accessor functions in
classes in these new files.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:373: virtual void VisitChildren(Visitor* visitor) = 0;
private:
  Node() {}

over here? Some other classes in this file are also missing constructors.

also DISALLOW_COPY_AND_ASSIGN(Node);

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:2889: return kernel::Program::ReadFrom(&reader);
I am wondering why this goes from binary format to the C++ structures in
kernel.[h|cc] here and then in KernelReader::ReadProgram() the C++ structures
are converted to the VM heap objects.

Would it make sense to generate the VM heap objects directly here? Kind of
similar to way it is done in the current AST

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

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:26: 
All Handle functions in this file do not seem to be using the Z parameter, would
be nice to have them all use Z.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:702: // TODO(kustermann): Are these correct?
Is this comment resolved?

[siva, 2016-10-13 23:15:58]

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5419: // TODO(kustermann): Memory leak!
On 2016/10/13 23:02:59, siva wrote:
> In general we try to avoid TODO with individual names, could you file a
tracking
> issue and add the issue number here in the TODO.

I have created https://github.com/dart-lang/sdk/issues/27588 for this.

[Florian Schneider, 2016-10-13 23:47:10]

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_x64.cc:2604: __ movq(CTX, Address(RBP,
closure_parameter->index() * kWordSize));
On 2016/10/13 14:37:57, Vyacheslav Egorov (Google) wrote:
> > In general I'm all for removing extra compiler-generated locals, if the
result
> > is smaller generated code. Here I see code added, that's why I'm wondering
if
> > this is actually an improvement and/or a bug fix of broken behavior in the
> > existing implementation.
> 
> As I tried to explain before the code is not added. It's moved around to a
more
> reasonable place.
> 
> Please see VM's AwaitTransformer::VisitAwaitNode. It builds the following
code:
> 
>   //   :saved_try_ctx_var = :await_saved_try_ctx_var_y;
> 
> (Where :saved_try_ctx_var is stack local and :await_saved_try_ctx_var_y is
> variable in the context).
> 
> Instead of having one special context variable that is manually synced with
the
> local variable after each await point, we simply allowed to capture
> :saved_try_ctx_var. However to access it later we need to have correct context
> in the :current_context when we enter catch. This is what this code is doing.
It
> sets :current_context to 0 level context (closure context) - the same way the
> entry does.
> 
> 
> Futhermore the VM current implementation is even more convoluted. For some
> reason it saves current and outer try context (which does not make much sense
to
> me and which our implementation does not need to do AFAIK - precisely due to
> this simplification).

Can they be captured in a non-async function?

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1143: const Field& field =
Field::Handle(owner.LookupFieldAllowPrivate(name));
Could you stick the Field into the Function::data_ to avoid having to do a
lookup here and below? The check for is_instance would go away as well.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:2014: CurrentException()->is_captured() ||
CurrentStackTrace()->is_captured() ||
This suggests that the exception and the stacktrace can be captured
individually, and not both or neither.

The assertion in CatchBlockEntryInstr::should_restore_closure_context() asserts
that either both or none are captured.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h#new...
runtime/vm/scopes.h:59: bool is_forced_stack() const { return is_forced_stack_;
}
What does is_forced_stack() mean?

[Kevin Millikin (Google), 2016-10-14 05:18:34]

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5426: library.set_debuggable(false);
On 2016/10/13 23:02:59, siva wrote:
> I presume this is temporary as we would want to debug libraries loaded this
way
> too.

Yes, this is temporary.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:2889: return kernel::Program::ReadFrom(&reader);
On 2016/10/13 23:02:59, siva wrote:
> I am wondering why this goes from binary format to the C++ structures in
> kernel.[h|cc] here and then in KernelReader::ReadProgram() the C++ structures
> are converted to the VM heap objects.
> 
> Would it make sense to generate the VM heap objects directly here? Kind of
> similar to way it is done in the current AST

Yes.  It should be possible to translate the binary directly to the flow graph
without creating C++ AST classes.

Right now it's convenient to have the C++ structures because it's a two-pass
translation Kernel->flow graph, where the first pass computes captured variables
for context and closure allocation.  We are implementing closure conversion on
Kernel and then there will be a one-pass transformation directly from the binary
to the flow graph.

[Vyacheslav Egorov (Google), 2016-10-14 11:55:27]

Thank you for your comments!

Given that I have an LGTM I am planning to land this (likely outside of working
hours for both AAR and MTV, e.g. Saturday). 

If anybody has outstanding issues - please contact me.

https://codereview.chromium.org/2411823003/diff/40001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/bin/dartutils.c...
runtime/bin/dartutils.cc:77: // Do not free buffer if we this is a kernel file -
kernel_file will be
On 2016/10/13 23:02:58, siva wrote:
> 'if we this' doesn't sound right.
> 
> Maybe add a comment that a free of the buffer is done after Dart_LoadKernel is
> done.

Done.

https://codereview.chromium.org/2411823003/diff/40001/runtime/bin/dartutils.c...
runtime/bin/dartutils.cc:615: UNREACHABLE();
On 2016/10/13 23:02:58, siva wrote:
> Why UNREACHABLE here and not
> result = DartUtils::NewError("%s is not a valid UTF-8 script",
> resolved_script_url);
> 

This code can load snapshots so I think the original idea was to handle loading
Kernel here too. So it should be unimplemented. 

However this is actually a dead code. John killed dart counterparts of it so
nobody now calls these builtins (see https://codereview.chromium.org/1998963003/
- it removes dart code, but does not kill this C++ part). 

I will send a separate CL to kill these builtins.

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

https://codereview.chromium.org/2411823003/diff/40001/runtime/include/dart_ap...
runtime/include/dart_api.h:2813: * \param length Length of the passed in buffer.
On 2016/10/13 23:02:58, siva wrote:
> buffer_len not length

Fixed here and above, from where it was copied.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5419: // TODO(kustermann): Memory leak!
On 2016/10/13 23:15:58, siva wrote:
> On 2016/10/13 23:02:59, siva wrote:
> > In general we try to avoid TODO with individual names, could you file a
> tracking
> > issue and add the issue number here in the TODO.
> 
> I have created https://github.com/dart-lang/sdk/issues/27588 for this.

I will convert all new TODO's to this style.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1143: const Field& field =
Field::Handle(owner.LookupFieldAllowPrivate(name));
On 2016/10/13 23:47:10, Florian Schneider wrote:
> Could you stick the Field into the Function::data_ to avoid having to do a
> lookup here and below? The check for is_instance would go away as well.

Great idea! Added a TODO.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:765: parsed_function,
On 2016/10/13 23:02:59, siva wrote:
> alignment?

Done. Was screwed up by search and replace s/dil/kernel/

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

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:373: virtual void VisitChildren(Visitor* visitor) = 0;
On 2016/10/13 23:02:59, siva wrote:
> private:
>   Node() {}
> 
> over here? Some other classes in this file are also missing constructors.
> 
> also DISALLOW_COPY_AND_ASSIGN(Node);

Can't be private - subclasses need it.

Made it protected. 

(This class has pure-virtual methods and thus naturally can't be constructed.)

Walked through the Kernel AST hierarchy.

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

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:26: 
On 2016/10/13 23:02:59, siva wrote:
> All Handle functions in this file do not seem to be using the Z parameter,
would
> be nice to have them all use Z.

Done.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:702: // TODO(kustermann): Are these correct?
On 2016/10/13 23:02:59, siva wrote:
> Is this comment resolved?

Done.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:2014: CurrentException()->is_captured() ||
CurrentStackTrace()->is_captured() ||
On 2016/10/13 23:47:10, Florian Schneider wrote:
> This suggests that the exception and the stacktrace can be captured
> individually, and not both or neither.
> 
> The assertion in CatchBlockEntryInstr::should_restore_closure_context()
asserts
> that either both or none are captured.

Fair point. Thanks! Fixed.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h#new...
runtime/vm/scopes.h:59: bool is_forced_stack() const { return is_forced_stack_;
}
On 2016/10/13 23:47:10, Florian Schneider wrote:
> What does is_forced_stack() mean?

Added a comment. 

It is used to prevent capturing local variable indiscriminately in
CaptureLocalVariables - which captures all local variables in the scope chain
between current scope and target scope.

CaptureLocalVariables(...) actually contains a black list:

(variable->name().raw() == Symbols::StackTraceVar().raw()) ||
          (variable->name().raw() == Symbols::ExceptionVar().raw()) ||
          (variable->name().raw() == Symbols::SavedTryContextVar().raw())

We thought that manually adding variables to the black list is tedious - so
instead we introduced a bit on the variable itself.

I just noticed that I accidentally omitted the code from CaptureLocalVariables
that uses this field. Thanks not spotting.

[Florian Schneider, 2016-10-14 21:26:48]

Few more comments to clean things up. Otherwise lgtm.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h
File runtime/vm/dil.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h#newcode1
runtime/vm/dil.h:1: // Copyright (c) 2016, the Dart project authors.  Please see
the AUTHORS file
On 2016/10/12 16:45:56, Vyacheslav Egorov (Google) wrote:
> On 2016/10/12 16:17:36, Florian Schneider wrote:
> > Please make sure that nothing from dil*.h/.cc ends up linked into
> > dart_precompiled_runtime to keep snapshot size from growing.
> 
> I will check that.

I noticed an 16K size increase for the stripped dart_precompiled_runtime binary.
I wonder if something from the front-end is still pulled into the runtime
accidentally.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:2967: #if 0
Is the code below needed?

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h#new...
runtime/vm/scopes.h:59: bool is_forced_stack() const { return is_forced_stack_;
}
On 2016/10/14 11:55:27, Vyacheslav Egorov (Google) wrote:
> On 2016/10/13 23:47:10, Florian Schneider wrote:
> > What does is_forced_stack() mean?
> 
> Added a comment. 
> 
> It is used to prevent capturing local variable indiscriminately in
> CaptureLocalVariables - which captures all local variables in the scope chain
> between current scope and target scope.
> 
> CaptureLocalVariables(...) actually contains a black list:
> 
> (variable->name().raw() == Symbols::StackTraceVar().raw()) ||
>           (variable->name().raw() == Symbols::ExceptionVar().raw()) ||
>           (variable->name().raw() == Symbols::SavedTryContextVar().raw())
> 
> We thought that manually adding variables to the black list is tedious - so
> instead we introduced a bit on the variable itself.
> 
> I just noticed that I accidentally omitted the code from CaptureLocalVariables
> that uses this field. Thanks not spotting. 

That's why I asked: I noticed that it was unused. Having both, a black-list and
this flag is not good. Please add a TODO to clean this up.

https://codereview.chromium.org/2411823003/diff/60001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2411823003/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:11: #if 0
For stuf like this we do

#ifdef DEBUG, and add a debug-mode only --trace... flag (see flag_list.h)

Or remove code if not needed anymore.

[Vyacheslav Egorov (Google), 2016-10-15 20:48:51]

Description was changed from

==========
VM support for running Kernel binaries.

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

to

==========
VM support for running Kernel binaries.

BUG=
R=asiva@google.com, fschneider@google.com

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

[Vyacheslav Egorov (Google), 2016-10-15 20:48:53]

Committed patchset #5 (id:80001) manually as
f67ce210689987a20fae57345a68cc40511cba4f (presubmit successful).

[Vyacheslav Egorov (Google), 2016-10-15 20:50:47]

Thanks! Landing it now.

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h
File runtime/vm/dil.h (right):

https://codereview.chromium.org/2411823003/diff/1/runtime/vm/dil.h#newcode1
runtime/vm/dil.h:1: // Copyright (c) 2016, the Dart project authors.  Please see
the AUTHORS file
On 2016/10/14 21:26:47, Florian Schneider wrote:
> On 2016/10/12 16:45:56, Vyacheslav Egorov (Google) wrote:
> > On 2016/10/12 16:17:36, Florian Schneider wrote:
> > > Please make sure that nothing from dil*.h/.cc ends up linked into
> > > dart_precompiled_runtime to keep snapshot size from growing.
> > 
> > I will check that.
> 
> I noticed an 16K size increase for the stripped dart_precompiled_runtime
binary.
> I wonder if something from the front-end is still pulled into the runtime
> accidentally.

binary size analysis does not show anything from Kernel sources, similary
"explain" tool does not show anything:

https://gist.github.com/mraleph/0ed37eefa67c834e381dd50cce4437d6

I will make an issue to follow up on this after landing.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:2967: #if 0
On 2016/10/14 21:26:47, Florian Schneider wrote:
> Is the code below needed?

Removed.

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/2411823003/diff/40001/runtime/vm/scopes.h#new...
runtime/vm/scopes.h:59: bool is_forced_stack() const { return is_forced_stack_;
}
On 2016/10/14 21:26:47, Florian Schneider wrote:
> On 2016/10/14 11:55:27, Vyacheslav Egorov (Google) wrote:
> > On 2016/10/13 23:47:10, Florian Schneider wrote:
> > > What does is_forced_stack() mean?
> > 
> > Added a comment. 
> > 
> > It is used to prevent capturing local variable indiscriminately in
> > CaptureLocalVariables - which captures all local variables in the scope
chain
> > between current scope and target scope.
> > 
> > CaptureLocalVariables(...) actually contains a black list:
> > 
> > (variable->name().raw() == Symbols::StackTraceVar().raw()) ||
> >           (variable->name().raw() == Symbols::ExceptionVar().raw()) ||
> >           (variable->name().raw() == Symbols::SavedTryContextVar().raw())
> > 
> > We thought that manually adding variables to the black list is tedious - so
> > instead we introduced a bit on the variable itself.
> > 
> > I just noticed that I accidentally omitted the code from
CaptureLocalVariables
> > that uses this field. Thanks not spotting. 
> 
> That's why I asked: I noticed that it was unused. Having both, a black-list
and
> this flag is not good. Please add a TODO to clean this up.

Added TODO

https://codereview.chromium.org/2411823003/diff/60001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2411823003/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:11: #if 0
On 2016/10/14 21:26:48, Florian Schneider wrote:
> For stuf like this we do
> 
> #ifdef DEBUG, and add a debug-mode only --trace... flag (see flag_list.h)
> 
> Or remove code if not needed anymore.

Done.

https://codereview.chromium.org/2411823003/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:11: #if 0
On 2016/10/14 21:26:48, Florian Schneider wrote:
> For stuf like this we do
> 
> #ifdef DEBUG, and add a debug-mode only --trace... flag (see flag_list.h)
> 
> Or remove code if not needed anymore.

Done.