Replace the --platform vm option with --kernel-binaries option.

Replace the --platform vm_option with --kernel-binaries option.

This new flag value specifies the directory in which the VM should look
up the platform.dill file. A future change will require the VM to load
another kernel binary from disk. This binary will also live in the same
directory that --kernel-binaries specifies. This way, we avoid adding a
different flag for each of the different binaries.

R=asiva@google.com

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

[sivachandra, 2017-06-13 23:42:17]

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

[sivachandra, 2017-06-13 23:42:18]

This change is in response to the comment in
https://codereview.chromium.org/2925203002/ to reduce the number of command line
options.

[sivachandra, 2017-06-13 23:45:13]

Description was changed from

==========
Replace the --platform vm_option with --kernel-binaries option.

This new flag value specifies the directory in which the VM should look
up the platform.dill file. A future change will require the VM to load
another kernel binary from disk. This binary will also live in the same
directory that --kernel-binaries specifies. This way, we avoid adding a
different flag for each of the different binaries.
==========

to

==========
Replace the --platform vm_option with --kernel-binaries option.

This new flag value specifies the directory in which the VM should look
up the platform.dill file. A future change will require the VM to load
another kernel binary from disk. This binary will also live in the same
directory that --kernel-binaries specifies. This way, we avoid adding a
different flag for each of the different binaries.
==========

[siva, 2017-06-15 18:00:50]

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

https://codereview.chromium.org/2933203004/diff/1/runtime/bin/dfe.cc#newcode44
runtime/bin/dfe.cc:44: File::PathSeparator(), kPlatformBinaryName);
We normally use a pattern like this:

 intptr_t len = snprintf(NULL, 0, format, __VA_ARGS__);
 char* msg = reinterpret_cast<char*>(malloc(len + 1));
 snprintf(msg, len + 1, format, __VA_ARGS__);

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

https://codereview.chromium.org/2933203004/diff/1/runtime/bin/main.cc#newcode390
runtime/bin/main.cc:390: dfe.set_kernel_binaries_directory(dirname);
why change the name to kernel_binaries_directory? It is still setting a filename
right?

[sivachandra, 2017-06-15 20:47:52]

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

https://codereview.chromium.org/2933203004/diff/1/runtime/bin/dfe.cc#newcode44
runtime/bin/dfe.cc:44: File::PathSeparator(), kPlatformBinaryName);
On 2017/06/15 18:00:49, siva wrote:
> We normally use a pattern like this:
> 
>  intptr_t len = snprintf(NULL, 0, format, __VA_ARGS__);
>  char* msg = reinterpret_cast<char*>(malloc(len + 1));
>  snprintf(msg, len + 1, format, __VA_ARGS__);

Modified to use snprintf to evaluate the length, but using new char[len] instead
of malloc and reinterpret_cast. Any reason to favor malloc over new?

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

https://codereview.chromium.org/2933203004/diff/1/runtime/bin/main.cc#newcode390
runtime/bin/main.cc:390: dfe.set_kernel_binaries_directory(dirname);
On 2017/06/15 18:00:49, siva wrote:
> why change the name to kernel_binaries_directory? It is still setting a
filename
> right?

Yes, inside of DFE, a file name is being set. I have chosen to do it this way as
the method set_kernel_binaries_directory will soon be extended to set the file
name of vmservice_io.dill as well.

[siva, 2017-06-16 00:56:13]

lgtm

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

https://codereview.chromium.org/2933203004/diff/1/runtime/bin/dfe.cc#newcode44
runtime/bin/dfe.cc:44: File::PathSeparator(), kPlatformBinaryName);
On 2017/06/15 20:47:52, sivachandra wrote:
> On 2017/06/15 18:00:49, siva wrote:
> > We normally use a pattern like this:
> > 
> >  intptr_t len = snprintf(NULL, 0, format, __VA_ARGS__);
> >  char* msg = reinterpret_cast<char*>(malloc(len + 1));
> >  snprintf(msg, len + 1, format, __VA_ARGS__);
> 
> Modified to use snprintf to evaluate the length, but using new char[len]
instead
> of malloc and reinterpret_cast. Any reason to favor malloc over new?

new char[len] is fine.

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

https://codereview.chromium.org/2933203004/diff/1/runtime/bin/main.cc#newcode390
runtime/bin/main.cc:390: dfe.set_kernel_binaries_directory(dirname);
On 2017/06/15 20:47:52, sivachandra wrote:
> On 2017/06/15 18:00:49, siva wrote:
> > why change the name to kernel_binaries_directory? It is still setting a
> filename
> > right?
> 
> Yes, inside of DFE, a file name is being set. I have chosen to do it this way
as
> the method set_kernel_binaries_directory will soon be extended to set the file
> name of vmservice_io.dill as well.

In that case can you rename this using Camel case as it is not a simple
accessor, i.e void SetKernelBinaries(const char* dirname);

[sivachandra, 2017-06-16 19:22:36]

Description was changed from

==========
Replace the --platform vm_option with --kernel-binaries option.

This new flag value specifies the directory in which the VM should look
up the platform.dill file. A future change will require the VM to load
another kernel binary from disk. This binary will also live in the same
directory that --kernel-binaries specifies. This way, we avoid adding a
different flag for each of the different binaries.
==========

to

==========
Replace the --platform vm_option with --kernel-binaries option.

This new flag value specifies the directory in which the VM should look
up the platform.dill file. A future change will require the VM to load
another kernel binary from disk. This binary will also live in the same
directory that --kernel-binaries specifies. This way, we avoid adding a
different flag for each of the different binaries.

R=asiva@google.com

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

[sivachandra, 2017-06-16 19:22:37]

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