Use Kernel frontend from run_vm_tests.

Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel parser snapshot. For example,

out/DebugX64/run_vm_tests --dfe=out/DebugX64/gen/kernel-service.dart.snapshot Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in debugger_test.cc to run with kernel frontend. There will be follow-up cls that enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
R=asiva@google.com

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

[aam, 2017-05-12 23:00:43]

Description was changed from

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests
--run_vm_test_with_kernel_snapshot=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

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

to

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests
--run_vm_test_with_kernel_snapshot=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the tests to use kernel frontend.

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

[aam, 2017-05-12 23:01:55]

Description was changed from

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests
--run_vm_test_with_kernel_snapshot=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the tests to use kernel frontend.

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

to

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests
--run_vm_test_with_kernel_snapshot=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

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

[aam, 2017-05-12 23:03:39]

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

[aam, 2017-05-12 23:03:39]

Hi Siva, 
 
  PTAL.

Thanks!

[aam, 2017-05-12 23:24:50]

Description was changed from

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests
--run_vm_test_with_kernel_snapshot=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

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

to

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests
--run_vm_test_with_kernel_snapshot=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
==========

[siva, 2017-05-15 04:34:27]

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
File runtime/bin/run_vm_tests.cc (right):

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:139: 
Will this code run for non kernel isolates too?

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:198: if (strstr(argv[arg_pos],
"--run_vm_test_with_kernel_snapshot") ==
Can we rename this option to --dfe same as the one we have for dart

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

https://codereview.chromium.org/2881953002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:3165: Dart_SourceFile source_files[]);
Dart_CompileToKernel and DartCompileSourcesToKernel seem to almost mean the
same,

Would 
Dart_CompileScriptUriToKernel
Dart_CompileSourcesToKernel
be more specific?

https://codereview.chromium.org/2881953002/diff/20001/runtime/vm/debugger_tes...
File runtime/vm/debugger_test.cc (right):

https://codereview.chromium.org/2881953002/diff/20001/runtime/vm/debugger_tes...
runtime/vm/debugger_test.cc:76: }
Can this be captures in TestCase::LoadTestScript so we don't have to repeat it
in every test file.

https://codereview.chromium.org/2881953002/diff/20001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.cc (right):

https://codereview.chromium.org/2881953002/diff/20001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:353: fileNamePairs[i * 2 + 1] = source_code;
(i * 2) + 1 for better readability?

[aam, 2017-05-15 23:49:14]

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
File runtime/bin/run_vm_tests.cc (right):

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:139: 
On 2017/05/15 at 04:34:26, siva wrote:
> Will this code run for non kernel isolates too?

Yes, if I understand correctly, this callback will be invoked for isolates
created via spawn() methods. But I can't find working tests yet that test
spawning.

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:198: if (strstr(argv[arg_pos],
"--run_vm_test_with_kernel_snapshot") ==
On 2017/05/15 at 04:34:26, siva wrote:
> Can we rename this option to --dfe same as the one we have for dart

--dfe option is picked up by VM, not run_vm_test one.

So, I tried to disambiguate this run_vm_test option, to be consumed by
run_vm_test, from other vm options like '--dfe'.

The way how currently rudimentary options parsing is done by run_vm_test is that
it only looks at a few at the beginning of the list, passes the rest to the vm.
If run_vm_test would rely on vm --dfe option, it should be able to pick it up
from anywhere in the list, right?

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

https://codereview.chromium.org/2881953002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:3165: Dart_SourceFile source_files[]);
On 2017/05/15 at 04:34:26, siva wrote:
> Dart_CompileToKernel and DartCompileSourcesToKernel seem to almost mean the
same,
> 
> Would 
> Dart_CompileScriptUriToKernel
> Dart_CompileSourcesToKernel
> be more specific?

Do you mean to to introduce new Dart_CompileScriptUriToKernel that would forward
to Dart_CompiletoKernel?
If we were to remove Dart_CompileToKernel, that would be breaking API change -
would that be worth it?

https://codereview.chromium.org/2881953002/diff/20001/runtime/vm/kernel_isola...
File runtime/vm/kernel_isolate.cc (right):

https://codereview.chromium.org/2881953002/diff/20001/runtime/vm/kernel_isola...
runtime/vm/kernel_isolate.cc:353: fileNamePairs[i * 2 + 1] = source_code;
On 2017/05/15 at 04:34:26, siva wrote:
> (i * 2) + 1 for better readability?

Done.

[aam, 2017-05-16 00:48:32]

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
File runtime/bin/run_vm_tests.cc (right):

https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:198: if (strstr(argv[arg_pos],
"--run_vm_test_with_kernel_snapshot") ==
On 2017/05/15 at 23:49:14, aam wrote:
> On 2017/05/15 at 04:34:26, siva wrote:
> > Can we rename this option to --dfe same as the one we have for dart
> 
> --dfe option is picked up by VM, not run_vm_test one.
> 
> So, I tried to disambiguate this run_vm_test option, to be consumed by
run_vm_test, from other vm options like '--dfe'.
> 
> The way how currently rudimentary options parsing is done by run_vm_test is
that it only looks at a few at the beginning of the list, passes the rest to the
vm. If run_vm_test would rely on vm --dfe option, it should be able to pick it
up from anywhere in the list, right?

Thanks for clarifying main.cc(embedder) '--dfe' versus vm '--use-dart-frontend'
options.
Now if we are to rename --run_vm_test_with_kernel_snapshot to '--dfe' should we
make it boolean option instead of string? If it is just a boolean option, then
run_vm_test will look for snapshot next to it. Something like
argv[0]/gen/kernel-service.dart.snapshot.

[aam, 2017-05-16 01:19:55]

On 2017/05/16 at 00:48:32, aam wrote:
>
https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
> File runtime/bin/run_vm_tests.cc (right):
> 
>
https://codereview.chromium.org/2881953002/diff/20001/runtime/bin/run_vm_test...
> runtime/bin/run_vm_tests.cc:198: if (strstr(argv[arg_pos],
"--run_vm_test_with_kernel_snapshot") ==
> On 2017/05/15 at 23:49:14, aam wrote:
> > On 2017/05/15 at 04:34:26, siva wrote:
> > > Can we rename this option to --dfe same as the one we have for dart
> > 
> > --dfe option is picked up by VM, not run_vm_test one.
> > 
> > So, I tried to disambiguate this run_vm_test option, to be consumed by
run_vm_test, from other vm options like '--dfe'.
> > 
> > The way how currently rudimentary options parsing is done by run_vm_test is
that it only looks at a few at the beginning of the list, passes the rest to the
vm. If run_vm_test would rely on vm --dfe option, it should be able to pick it
up from anywhere in the list, right?
> 
> Thanks for clarifying main.cc(embedder) '--dfe' versus vm
'--use-dart-frontend' options.
> Now if we are to rename --run_vm_test_with_kernel_snapshot to '--dfe' should
we make it boolean option instead of string? If it is just a boolean option,
then run_vm_test will look for snapshot next to it. Something like
argv[0]/gen/kernel-service.dart.snapshot.

It seems to be hard to have robust cross-platform mechanism of determining
directory where run_vm_test is located(what if you create and run a hard link to
run_vm_test, for example). So perhaps passing a string path to snapshot will be
better.

[aam, 2017-05-16 13:52:39]

Description was changed from

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests
--run_vm_test_with_kernel_snapshot=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
==========

to

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests --dfe=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
==========

[siva, 2017-05-18 00:02:39]

LGTM with some comments.

https://codereview.chromium.org/2881953002/diff/80001/runtime/bin/run_vm_test...
File runtime/bin/run_vm_tests.cc (right):

https://codereview.chromium.org/2881953002/diff/80001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:163: }
I wonder if all this code be just under if (is_kernel_isolate) {

https://codereview.chromium.org/2881953002/diff/80001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:203: ++arg_pos;
can we automatically add the --use-dart-frontend option here instead of
requiring it on the command line. Somewhat similar to how it is done in main.cc

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:287: }
The Concat function above may not be needed if you use OS::SCreate....

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:296: }
  Zone* zone = Thread::Current()->zone();

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:298: char* filename = Concat("file:///", lib_url);
char* filename = OS::SCreate(zone, "file:///", lib_url);

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:312: delete[] filename;
delete not needed as 'filename' would be zone allocated.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:314: EXPECT(compilation_result.status ==
Dart_KernelCompilationStatus_Ok)
The EXPECT above would also print an error message, I think we can get rid of
the expect and just rely on your explicit check below.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:317: return NULL;
should return a Dart_Handle not NULL

return Dart_ApiNewError(OS::SCreate(
    Thread::Current()->zone(), "Compilation failed %s",
compilation_result.error));

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:321: EXPECT(kernel_file != NULL);
Should this be ASSERT(kernel_file != NULL);

or 
if (kernel_file == NULL) {
    return Dart_ApiNewError(OS::SCreate(Thread::Current()->zone(), "front end
generated a NULL kernel file"));
}

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:323: EXPECT(kernel_program != NULL);
Ditto comment about this check here, should return a Dart_Handle

[aam, 2017-05-18 01:03:01]

Thank you for the comments, Siva!

PTAL

https://codereview.chromium.org/2881953002/diff/80001/runtime/bin/run_vm_test...
File runtime/bin/run_vm_tests.cc (right):

https://codereview.chromium.org/2881953002/diff/80001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:163: }
On 2017/05/18 at 00:02:38, siva wrote:
> I wonder if all this code be just under if (is_kernel_isolate) {

Done, I think that is fair assumption at this point.

https://codereview.chromium.org/2881953002/diff/80001/runtime/bin/run_vm_test...
runtime/bin/run_vm_tests.cc:203: ++arg_pos;
On 2017/05/18 at 00:02:38, siva wrote:
> can we automatically add the --use-dart-frontend option here instead of
requiring it on the command line. Somewhat similar to how it is done in main.cc

Sure, good idea. Done.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:287: }
On 2017/05/18 at 00:02:38, siva wrote:
> The Concat function above may not be needed if you use OS::SCreate....

Done.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:296: }
On 2017/05/18 at 00:02:38, siva wrote:
> Zone* zone = Thread::Current()->zone();

Done.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:298: char* filename = Concat("file:///", lib_url);
On 2017/05/18 at 00:02:39, siva wrote:
> char* filename = OS::SCreate(zone, "file:///", lib_url);

Done.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:312: delete[] filename;
On 2017/05/18 at 00:02:38, siva wrote:
> delete not needed as 'filename' would be zone allocated.

Awesome, done!

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:314: EXPECT(compilation_result.status ==
Dart_KernelCompilationStatus_Ok)
On 2017/05/18 at 00:02:38, siva wrote:
> The EXPECT above would also print an error message, I think we can get rid of
the expect and just rely on your explicit check below.

Right, done.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:317: return NULL;
On 2017/05/18 at 00:02:38, siva wrote:
> should return a Dart_Handle not NULL
> 
> return Dart_ApiNewError(OS::SCreate(
>     Thread::Current()->zone(), "Compilation failed %s",
compilation_result.error));

Oh, nice! Done.

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:321: EXPECT(kernel_file != NULL);
On 2017/05/18 at 00:02:38, siva wrote:
> Should this be ASSERT(kernel_file != NULL);
> 
> or 
> if (kernel_file == NULL) {
>     return Dart_ApiNewError(OS::SCreate(Thread::Current()->zone(), "front end
generated a NULL kernel file"));
> }

Yes, I like this better!

https://codereview.chromium.org/2881953002/diff/80001/runtime/vm/unit_test.cc...
runtime/vm/unit_test.cc:323: EXPECT(kernel_program != NULL);
On 2017/05/18 at 00:02:38, siva wrote:
> Ditto comment about this check here, should return a Dart_Handle

Done!

[aam, 2017-05-18 01:07:43]

Description was changed from

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests --dfe=out/DebugX64/gen/kernel-service.dart.snapshot
--use_dart_frontend Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
==========

to

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests --dfe=out/DebugX64/gen/kernel-service.dart.snapshot
Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
==========

[siva, 2017-05-18 04:04:02]

lgtm

[aam, 2017-05-18 04:26:01]

Description was changed from

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests --dfe=out/DebugX64/gen/kernel-service.dart.snapshot
Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
==========

to

==========
Use Kernel frontend from run_vm_tests.

With this change one can run run_vm_tests and pass to it reference to kernel
parser snapshot. For example,

out/DebugX64/run_vm_tests --dfe=out/DebugX64/gen/kernel-service.dart.snapshot
Debugger_RemoveBreakpoint

As this represents first step, this change enables only tests in
debugger_test.cc to run with kernel frontend. There will be follow-up cls that
enable all of the VM tests to use kernel frontend.

BUG=https://github.com/dart-lang/sdk/issues/28264
R=asiva@google.com

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

[aam, 2017-05-18 04:26:03]

Committed patchset #6 (id:100001) manually as
d91f228766a2c1708bec8f62bbe2756e4a529f5e (presubmit successful).