Enable generation of instruction buffer on precompilation

Enable generation of instruction buffer on precompilation

R=rmacnak@google.com

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

[Chinmay, 2015-10-21 20:31:27]

chinmaygarde@google.com changed reviewers:
+ johnmccutchan@google.com, rmacnak@google.com

[Chinmay, 2015-10-21 20:31:27]

These are the required updates to the snapshotter to make usable precompiled
builds on iOS. This is just an initial take, let me know if you want me to slice
this into separate chunks.

[Cutch, 2015-10-21 22:00:08]

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

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:542: int argument_count,
align arguments

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:636: static bool ParseEntryPointsManifestSingleLine(
This should take an char** error which indicates what went wrong on failure
which can be printed by the caller.

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:703: entries = -1;
This might leak some |entry_item| allocated above in the error case.

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:749: if (parsed_entry_count != entry_count) {
You're calling the same function twice with the same inputs. How could this
happen?

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:750: free(entries);
This leaks pointers inside of |entries|. Call CleanupEntryPointsCollection
instead of just free.

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:766: if (entries == nullptr &&
IsSnapshottingForPrecompilation()) {
s/nullptr/NULL here and elsewhere

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:783: if (entry.library_uri == nullptr) {
s/nullptr/NULL here and elsewhere

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:786: free((char*)entry.library_uri);
use c++ casts

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:826: if (standalone_entry_points == nullptr) {
Can you hoist this out or rename it? It's confusing to see the call to
CreateAndWritePrecompiledSnapshot below when it's only writing a precompiled
snapshot sometimes.

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

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/assembler_arm64....
runtime/vm/assembler_arm64.cc:17: #if !defined(USING_SIMULATOR) &&
!defined(__linux__) && !defined(ANDROID) && !TARGET_OS_IOS
80 columns

[rmacnak, 2015-10-21 23:06:19]

https://codereview.chromium.org/1411383004/diff/1/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/1411383004/diff/1/runtime/BUILD.gn#newcode24
runtime/BUILD.gn:24: if (!is_ios || !is_android) {
Won't this also cause desktop Linux builds to use the simulator?

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

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:42: static const char* vm_entry_points_manifest =
NULL;
Throughout: s/vm_entry_points/embedder_entry_points/

There is a different list in the precompiler for the VM's own entry points.

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:509: machine code that must be linked into   \n
s/machine code/assembly

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:537: ASSERT(false);
UNREACHABLE();

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:553: static void SetupStubNativeResolver(size_t
lib_index,
TODO(24686): Remove this.

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:594: if (entries == nullptr) {
Here and throughout s/nullptr/NULL

nullptr is undefined in the standalone builds

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:641: char* component = strchr(line + offset, i == 2
? '\n' : ',');
const char* component

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:979:
vm_options.AddArgument("--write_protect_code=false");
--write_protect_code=false should be unnecessary now

https://codereview.chromium.org/1411383004/diff/1/runtime/tools/create_snapsh...
File runtime/tools/create_snapshot_bin.py (right):

https://codereview.chromium.org/1411383004/diff/1/runtime/tools/create_snapsh...
runtime/tools/create_snapshot_bin.py:37: help="output file name into which
instructions snapshot in binary form " +
It's in assembly form, not binary form.

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

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/assembler_arm64....
runtime/vm/assembler_arm64.cc:17: #if !defined(USING_SIMULATOR) &&
!defined(__linux__) && !defined(ANDROID) && !TARGET_OS_IOS
Same guard on arm32.

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/assembler_arm64....
runtime/vm/assembler_arm64.cc:17: #if !defined(USING_SIMULATOR) &&
!defined(__linux__) && !defined(ANDROID) && !TARGET_OS_IOS
Zach, is this needed on ARM64?  As far as I know we don't do any CPU feature
detection on ARM64.

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

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/cpu_arm64.cc#new...
runtime/vm/cpu_arm64.cc:20: void CPU::FlushICache(uword start, uword size) {
#if TARGET_OS_IOS
  // Precompilation never patches code so there should be no I cache flushes.
  UNREACHABLE();
#endif

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

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:374: if (Dart::IsRunningPrecompiledCode()) {
// TODO(24651): Remove this.

[Chinmay, 2015-10-22 00:13:17]

johnmccutchan@ and rmacnak@: I believe I have addressed all concerns. PTAL

https://codereview.chromium.org/1411383004/diff/1/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/1411383004/diff/1/runtime/BUILD.gn#newcode24
runtime/BUILD.gn:24: if (!is_ios || !is_android) {
On 2015/10/21 at 23:06:18, rmacnak wrote:
> Won't this also cause desktop Linux builds to use the simulator?

Good point. I have switched this to a target_os check

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

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:42: static const char* vm_entry_points_manifest =
NULL;
On 2015/10/21 at 23:06:18, rmacnak wrote:
> Throughout: s/vm_entry_points/embedder_entry_points/
> 
> There is a different list in the precompiler for the VM's own entry points.

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:509: machine code that must be linked into   \n
On 2015/10/21 at 23:06:18, rmacnak wrote:
> s/machine code/assembly

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:537: ASSERT(false);
On 2015/10/21 at 23:06:18, rmacnak wrote:
> UNREACHABLE();

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:542: int argument_count,
On 2015/10/21 at 22:00:08, Cutch wrote:
> align arguments

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:553: static void SetupStubNativeResolver(size_t
lib_index,
On 2015/10/21 at 23:06:18, rmacnak wrote:
> TODO(24686): Remove this.

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:594: if (entries == nullptr) {
On 2015/10/21 at 23:06:18, rmacnak wrote:
> Here and throughout s/nullptr/NULL
> 
> nullptr is undefined in the standalone builds

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:636: static bool ParseEntryPointsManifestSingleLine(
On 2015/10/21 at 22:00:08, Cutch wrote:
> This should take an char** error which indicates what went wrong on failure
which can be printed by the caller.

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:641: char* component = strchr(line + offset, i == 2
? '\n' : ',');
On 2015/10/21 at 23:06:18, rmacnak wrote:
> const char* component

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:703: entries = -1;
On 2015/10/21 at 22:00:08, Cutch wrote:
> This might leak some |entry_item| allocated above in the error case.

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:749: if (parsed_entry_count != entry_count) {
On 2015/10/21 at 22:00:08, Cutch wrote:
> You're calling the same function twice with the same inputs. How could this
happen?

Was being paranoid since I am reading from a file on disk. Will get rid of it
and add an assert instead.

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:750: free(entries);
On 2015/10/21 at 22:00:08, Cutch wrote:
> This leaks pointers inside of |entries|. Call CleanupEntryPointsCollection
instead of just free.

ack. Removed by the assert.

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:766: if (entries == nullptr &&
IsSnapshottingForPrecompilation()) {
On 2015/10/21 at 22:00:08, Cutch wrote:
> s/nullptr/NULL here and elsewhere

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:783: if (entry.library_uri == nullptr) {
On 2015/10/21 at 22:00:08, Cutch wrote:
> s/nullptr/NULL here and elsewhere

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:786: free((char*)entry.library_uri);
On 2015/10/21 at 22:00:08, Cutch wrote:
> use c++ casts

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:826: if (standalone_entry_points == nullptr) {
On 2015/10/21 at 22:00:08, Cutch wrote:
> Can you hoist this out or rename it? It's confusing to see the call to
CreateAndWritePrecompiledSnapshot below when it's only writing a precompiled
snapshot sometimes.

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/bin/gen_snapshot.cc...
runtime/bin/gen_snapshot.cc:979:
vm_options.AddArgument("--write_protect_code=false");
On 2015/10/21 at 23:06:18, rmacnak wrote:
> --write_protect_code=false should be unnecessary now

ack

https://codereview.chromium.org/1411383004/diff/1/runtime/tools/create_snapsh...
File runtime/tools/create_snapshot_bin.py (right):

https://codereview.chromium.org/1411383004/diff/1/runtime/tools/create_snapsh...
runtime/tools/create_snapshot_bin.py:37: help="output file name into which
instructions snapshot in binary form " +
On 2015/10/21 at 23:06:18, rmacnak wrote:
> It's in assembly form, not binary form.

ack

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

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/assembler_arm64....
runtime/vm/assembler_arm64.cc:17: #if !defined(USING_SIMULATOR) &&
!defined(__linux__) && !defined(ANDROID) && !TARGET_OS_IOS
On 2015/10/21 at 22:00:08, Cutch wrote:
> 80 columns

ack

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

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/cpu_arm64.cc#new...
runtime/vm/cpu_arm64.cc:20: void CPU::FlushICache(uword start, uword size) {
On 2015/10/21 at 23:06:18, rmacnak wrote:
> #if TARGET_OS_IOS
>   // Precompilation never patches code so there should be no I cache flushes.
>   UNREACHABLE();
> #endif

ack

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

https://codereview.chromium.org/1411383004/diff/1/runtime/vm/service_isolate....
runtime/vm/service_isolate.cc:374: if (Dart::IsRunningPrecompiledCode()) {
On 2015/10/21 at 23:06:18, rmacnak wrote:
> // TODO(24651): Remove this.

ack

[rmacnak, 2015-10-22 16:46:15]

https://codereview.chromium.org/1411383004/diff/20001/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/1411383004/diff/20001/runtime/BUILD.gn#newcode26
runtime/BUILD.gn:26: # compile the snapshotter on the host. If no instruction
buffer is created
The snapshotter is osx not ios.

It would be better to use an explicit flag for this rather than have every GN
build of a OSX VM using a simulator.

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:669: char *ParserErrorStringCreate(const char
*format, ...) {
char* Parser
const char* format

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:778: char *error_buffer = NULL;
char* error_buffer

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:803: FILE* file = fopen(path, "r");
Are these IO functions portable? gen_snapshot needs to keep working on Windows.
It's better to use the functions from runtime/bin/file.h instead like
WriteSnapshotFile does.

[Chinmay, 2015-10-22 19:01:30]

rmacnack@ Have added the dart_target_arch to runtime/BUILD.gn + other minor
fixes.

https://codereview.chromium.org/1411383004/diff/20001/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/1411383004/diff/20001/runtime/BUILD.gn#newcode26
runtime/BUILD.gn:26: # compile the snapshotter on the host. If no instruction
buffer is created
On 2015/10/22 at 16:46:15, rmacnak wrote:
> The snapshotter is osx not ios.
> 
> It would be better to use an explicit flag for this rather than have every GN
build of a OSX VM using a simulator.

ack

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:669: char *ParserErrorStringCreate(const char
*format, ...) {
On 2015/10/22 at 16:46:15, rmacnak wrote:
> char* Parser
> const char* format

ack

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:778: char *error_buffer = NULL;
On 2015/10/22 at 16:46:15, rmacnak wrote:
> char* error_buffer

ack

https://codereview.chromium.org/1411383004/diff/20001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:803: FILE* file = fopen(path, "r");
On 2015/10/22 at 16:46:15, rmacnak wrote:
> Are these IO functions portable? gen_snapshot needs to keep working on
Windows. It's better to use the functions from runtime/bin/file.h instead like
WriteSnapshotFile does.

I am quite certain I have used portable functions. I could not find functions to
check the stream status in bin/file.h. So had to resort to this implementation.

[rmacnak, 2015-10-22 19:22:39]

LGTM

[Chinmay, 2015-10-22 19:26:55]

Committed patchset #3 (id:40001) manually as
87ce99fd005a26abbc3c6c9bcc4c3c39a2baf952.