[VM/AOT] Ensure we set flags from snapshot very early in the VM initialization

There are uses of flags before the VM snapshot gets read, so we need to
move the "snapshot sniffing" code to the very beginning of Dart::Init().

Due to the fact that FullSnapshotReader/Deserializer assume there is a
[Thread] available - and we don't have a [Thread] yet when we should
process the flags - this CL moves version/feature logic into a new
[SnapshotHeaderReader] class which does not depend on Thread/...

Change-Id: I38a45041f0696b96e44fc2b294e3442a5fba1a81
Reviewed-on: https://dart-review.googlesource.com/c/90941
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
diff --git a/runtime/tests/vm/dart/use_bare_instructions_flag_test.dart b/runtime/tests/vm/dart/use_bare_instructions_flag_test.dart
new file mode 100644
index 0000000..bb8cc83
--- /dev/null
+++ b/runtime/tests/vm/dart/use_bare_instructions_flag_test.dart
@@ -0,0 +1,136 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// This test is ensuring that the flag for --use-bare-instructions given at
+// AOT compile-time will be used at runtime (irrespective if other values were
+// passed to the runtime).
+
+import "dart:async";
+import "dart:io";
+
+import 'package:expect/expect.dart';
+import 'package:path/path.dart' as path;
+
+main(List<String> args) async {
+  if (!Platform.executable.endsWith("dart_precompiled_runtime")) {
+    return; // Running in JIT: AOT binaries not available.
+  }
+
+  if (Platform.isAndroid) {
+    return; // SDK tree and dart_bootstrap not available on the test device.
+  }
+
+  final buildDir = path.dirname(Platform.executable);
+  final sdkDir = path.dirname(path.dirname(buildDir));
+  final platformDill = path.join(buildDir, 'vm_platform_strong.dill');
+  final genSnapshot = path.join(buildDir, 'gen_snapshot');
+  final aotRuntime = path.join(buildDir, 'dart_precompiled_runtime');
+
+  await withTempDir((String tempDir) async {
+    final script = path.join(sdkDir, 'pkg/kernel/bin/dump.dart');
+    final scriptDill = path.join(tempDir, 'kernel_dump.dill');
+
+    // Compile script to Kernel IR.
+    await run('pkg/vm/tool/gen_kernel', <String>[
+      '--aot',
+      '--platform=$platformDill',
+      '-o',
+      scriptDill,
+      script,
+    ]);
+
+    // Run the AOT compiler with/without bare instructions.
+    final scriptBareSnapshot = path.join(tempDir, 'bare.snapshot');
+    final scriptNonBareSnapshot = path.join(tempDir, 'non_bare.snapshot');
+    await Future.wait(<Future>[
+      run(genSnapshot, <String>[
+        '--use-bare-instructions',
+        '--snapshot-kind=app-aot-blobs',
+        '--blobs_container_filename=$scriptBareSnapshot',
+        scriptDill,
+      ]),
+      run(genSnapshot, <String>[
+        '--no-use-bare-instructions',
+        '--snapshot-kind=app-aot-blobs',
+        '--blobs_container_filename=$scriptNonBareSnapshot',
+        scriptDill,
+      ]),
+    ]);
+
+    // Run the resulting bare-AOT compiled script.
+    final bareOut1 = path.join(tempDir, 'bare-out1.txt');
+    final bareOut2 = path.join(tempDir, 'bare-out2.txt');
+    await Future.wait(<Future>[
+      run(aotRuntime, <String>[
+        '--use-bare-instructions',
+        scriptBareSnapshot,
+        scriptDill,
+        bareOut1,
+      ]),
+      run(aotRuntime, <String>[
+        '--no-use-bare-instructions',
+        scriptBareSnapshot,
+        scriptDill,
+        bareOut2,
+      ]),
+    ]);
+
+    // Run the resulting non-bare-AOT compiled script.
+    final nonBareOut1 = path.join(tempDir, 'non-bare-out1.txt');
+    final nonBareOut2 = path.join(tempDir, 'non-bare-out2.txt');
+    await Future.wait(<Future>[
+      run(aotRuntime, <String>[
+        '--use-bare-instructions',
+        scriptNonBareSnapshot,
+        scriptDill,
+        nonBareOut1,
+      ]),
+      run(aotRuntime, <String>[
+        '--no-use-bare-instructions',
+        scriptNonBareSnapshot,
+        scriptDill,
+        nonBareOut2,
+      ]),
+    ]);
+
+    // Ensure we got 4 times the same result.
+    final output = await readFile(bareOut1);
+    Expect.equals(output, await readFile(bareOut2));
+    Expect.equals(output, await readFile(nonBareOut1));
+    Expect.equals(output, await readFile(nonBareOut2));
+  });
+}
+
+Future<String> readFile(String file) {
+  return new File(file).readAsString();
+}
+
+Future run(String executable, List<String> args) async {
+  print('Running $executable ${args.join(' ')}');
+
+  final result = await Process.run(executable, args);
+  final String stdout = result.stdout;
+  final String stderr = result.stderr;
+  if (stdout.isNotEmpty) {
+    print('stdout:');
+    print(stdout);
+  }
+  if (stderr.isNotEmpty) {
+    print('stderr:');
+    print(stderr);
+  }
+
+  if (result.exitCode != 0) {
+    throw 'Command failed with non-zero exit code (was ${result.exitCode})';
+  }
+}
+
+withTempDir(Future fun(String dir)) async {
+  final tempDir = Directory.systemTemp.createTempSync('bare-flag-test');
+  try {
+    await fun(tempDir.path);
+  } finally {
+    tempDir.deleteSync(recursive: true);
+  }
+}
diff --git a/runtime/tests/vm/vm.status b/runtime/tests/vm/vm.status
index 3314426..302dbc5 100644
--- a/runtime/tests/vm/vm.status
+++ b/runtime/tests/vm/vm.status
@@ -22,6 +22,7 @@
 dart/snapshot_version_test: Skip # This test is a Dart1 test (script snapshot)
 dart/slow_path_shared_stub_test: Pass, Slow # Uses --shared-slow-path-triggers-gc flag.
 dart/stack_overflow_shared_test: Pass, Slow # Uses --shared-slow-path-triggers-gc flag.
+dart/use_bare_instructions_flag_test: Pass, Slow # Spawns several subprocesses
 
 [ $mode == debug ]
 dart/appjit_cha_deopt_test: Pass, Slow # Quite slow in debug mode, uses --optimization-counter-threshold=100
@@ -110,6 +111,9 @@
 [ $runtime != dart_precompiled  || $system == android ]
 dart/bare_instructions_trampolines_test: SkipByDesign # This test is for VM AOT only (android fails due to listing interfaces).
 
+[ $mode == debug || $runtime != dart_precompiled  || $system == android ]
+dart/use_bare_instructions_flag_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
+
 [ $system == fuchsia ]
 cc/CorelibIsolateStartup: Skip # OOM crash can bring down the OS.
 cc/Read: Fail # TODO(zra): Investigate, ../../dart/runtime/bin/file_test.cc: 34: error: expected: !file->WriteByte(1)
diff --git a/runtime/vm/clustered_snapshot.cc b/runtime/vm/clustered_snapshot.cc
index ef183f9..784b5f9 100644
--- a/runtime/vm/clustered_snapshot.cc
+++ b/runtime/vm/clustered_snapshot.cc
@@ -4810,7 +4810,8 @@
                            const uint8_t* data_buffer,
                            const uint8_t* instructions_buffer,
                            const uint8_t* shared_data_buffer,
-                           const uint8_t* shared_instructions_buffer)
+                           const uint8_t* shared_instructions_buffer,
+                           intptr_t offset)
     : ThreadStackResource(thread),
       heap_(thread->isolate()->heap()),
       zone_(thread->zone()),
@@ -4827,6 +4828,7 @@
         new (zone_) ImageReader(data_buffer, instructions_buffer,
                                 shared_data_buffer, shared_instructions_buffer);
   }
+  stream_.SetPosition(offset);
 }
 
 Deserializer::~Deserializer() {
@@ -4953,39 +4955,43 @@
   return NULL;
 }
 
-RawApiError* Deserializer::VerifyVersionAndFeatures(Isolate* isolate) {
-  if (image_reader_ != NULL) {
-    RawApiError* error = image_reader_->VerifyAlignment();
-    if (error != ApiError::null()) {
-      return error;
-    }
+RawApiError* Deserializer::VerifyImageAlignment() {
+  if (image_reader_ != nullptr) {
+    return image_reader_->VerifyAlignment();
   }
-
-  RawApiError* error = VerifyVersion();
-  if (error != ApiError::null()) {
-    return error;
-  }
-
-  return VerifyFeatures(isolate);
+  return ApiError::null();
 }
 
-RawApiError* Deserializer::VerifyVersion() {
+char* SnapshotHeaderReader::VerifyVersionAndFeatures(Isolate* isolate,
+                                                     intptr_t* offset) {
+  char* error = VerifyVersion();
+  if (error == nullptr) {
+    error = VerifyFeatures(isolate);
+  }
+  if (error == nullptr) {
+    *offset = stream_.Position();
+  }
+  return error;
+}
+
+char* SnapshotHeaderReader::VerifyVersion() {
   // If the version string doesn't match, return an error.
   // Note: New things are allocated only if we're going to return an error.
 
   const char* expected_version = Version::SnapshotString();
   ASSERT(expected_version != NULL);
   const intptr_t version_len = strlen(expected_version);
-  if (PendingBytes() < version_len) {
+  if (stream_.PendingBytes() < version_len) {
     const intptr_t kMessageBufferSize = 128;
     char message_buffer[kMessageBufferSize];
     Utils::SNPrint(message_buffer, kMessageBufferSize,
                    "No full snapshot version found, expected '%s'",
                    expected_version);
-    return BuildApiError(message_buffer);
+    return BuildError(message_buffer);
   }
 
-  const char* version = reinterpret_cast<const char*>(CurrentBufferAddress());
+  const char* version =
+      reinterpret_cast<const char*>(stream_.AddressOfCurrentPosition());
   ASSERT(version != NULL);
   if (strncmp(version, expected_version, version_len)) {
     const intptr_t kMessageBufferSize = 256;
@@ -4996,14 +5002,14 @@
                    (Snapshot::IsFull(kind_)) ? "full" : "script",
                    expected_version, actual_version);
     free(actual_version);
-    return BuildApiError(message_buffer);
+    return BuildError(message_buffer);
   }
-  Advance(version_len);
+  stream_.Advance(version_len);
 
-  return ApiError::null();
+  return nullptr;
 }
 
-RawApiError* Deserializer::VerifyFeatures(Isolate* isolate) {
+char* SnapshotHeaderReader::VerifyFeatures(Isolate* isolate) {
   const char* expected_features =
       Dart::FeaturesString(isolate, (isolate == NULL), kind_);
   ASSERT(expected_features != NULL);
@@ -5012,8 +5018,8 @@
   const char* features = nullptr;
   intptr_t features_length = 0;
 
-  RawApiError* error = ReadFeatures(&features, &features_length);
-  if (error != ApiError::null()) {
+  auto error = ReadFeatures(&features, &features_length);
+  if (error != nullptr) {
     return error;
   }
 
@@ -5029,27 +5035,35 @@
                    actual_features, expected_features);
     free(const_cast<char*>(expected_features));
     free(actual_features);
-    return BuildApiError(message_buffer);
+    return BuildError(message_buffer);
   }
   free(const_cast<char*>(expected_features));
-  return ApiError::null();
+  return nullptr;
 }
 
-RawApiError* Deserializer::ReadFeatures(const char** features,
-                                        intptr_t* features_length) {
-  const char* cursor = reinterpret_cast<const char*>(CurrentBufferAddress());
-  const intptr_t length = Utils::StrNLen(cursor, PendingBytes());
-  if (length == PendingBytes()) {
-    return BuildApiError(
+char* SnapshotHeaderReader::ReadFeatures(const char** features,
+                                         intptr_t* features_length) {
+  const char* cursor =
+      reinterpret_cast<const char*>(stream_.AddressOfCurrentPosition());
+  const intptr_t length = Utils::StrNLen(cursor, stream_.PendingBytes());
+  if (length == stream_.PendingBytes()) {
+    return BuildError(
         "The features string in the snapshot was not '\\0'-terminated.");
   }
   *features = cursor;
   *features_length = length;
-  Advance(length + 1);
-  return ApiError::null();
+  stream_.Advance(length + 1);
+  return nullptr;
 }
 
-RawApiError* Deserializer::BuildApiError(const char* message) {
+char* SnapshotHeaderReader::BuildError(const char* message) {
+  return strdup(message);
+}
+
+RawApiError* FullSnapshotReader::ConvertToApiError(char* message) {
+  // The [message] was constructed with [BuildError] and needs to be freed.
+  free(message);
+
   // This can also fail while bringing up the VM isolate, so make sure to
   // allocate the error message in old space.
   const String& msg = String::Handle(String::New(message, Heap::kOld));
@@ -5568,21 +5582,19 @@
   shared_instructions_image_ = shared_instructions;
 }
 
-RawApiError* FullSnapshotReader::InitializeGlobalVMFlagsFromSnapshot() {
-  Deserializer deserializer(thread_, kind_, buffer_, size_, data_image_,
-                            instructions_image_, NULL, NULL);
+char* SnapshotHeaderReader::InitializeGlobalVMFlagsFromSnapshot(
+    const Snapshot* snapshot) {
+  SnapshotHeaderReader header_reader(snapshot);
 
-  deserializer.SkipHeader();
-
-  RawApiError* error = deserializer.VerifyVersion();
-  if (error != ApiError::null()) {
+  char* error = header_reader.VerifyVersion();
+  if (error != nullptr) {
     return error;
   }
 
   const char* features = nullptr;
   intptr_t features_length = 0;
-  error = deserializer.ReadFeatures(&features, &features_length);
-  if (error != ApiError::null()) {
+  error = header_reader.ReadFeatures(&features, &features_length);
+  if (error != nullptr) {
     return error;
   }
 
@@ -5614,18 +5626,24 @@
     cursor = end;
   }
 
-  return ApiError::null();
+  return nullptr;
 }
 
 RawApiError* FullSnapshotReader::ReadVMSnapshot() {
+  SnapshotHeaderReader header_reader(kind_, buffer_, size_);
+
+  intptr_t offset = 0;
+  char* error =
+      header_reader.VerifyVersionAndFeatures(/*isolate=*/NULL, &offset);
+  if (error != nullptr) {
+    return ConvertToApiError(error);
+  }
+
   Deserializer deserializer(thread_, kind_, buffer_, size_, data_image_,
-                            instructions_image_, NULL, NULL);
-
-  deserializer.SkipHeader();
-
-  RawApiError* error = deserializer.VerifyVersionAndFeatures(/*isolate=*/NULL);
-  if (error != ApiError::null()) {
-    return error;
+                            instructions_image_, NULL, NULL, offset);
+  RawApiError* api_error = deserializer.VerifyImageAlignment();
+  if (api_error != ApiError::null()) {
+    return api_error;
   }
 
   if (Snapshot::IncludesCode(kind_)) {
@@ -5643,16 +5661,20 @@
 }
 
 RawApiError* FullSnapshotReader::ReadIsolateSnapshot() {
+  SnapshotHeaderReader header_reader(kind_, buffer_, size_);
+  intptr_t offset = 0;
+  char* error =
+      header_reader.VerifyVersionAndFeatures(thread_->isolate(), &offset);
+  if (error != nullptr) {
+    return ConvertToApiError(error);
+  }
+
   Deserializer deserializer(thread_, kind_, buffer_, size_, data_image_,
                             instructions_image_, shared_data_image_,
-                            shared_instructions_image_);
-
-  deserializer.SkipHeader();
-
-  RawApiError* error =
-      deserializer.VerifyVersionAndFeatures(thread_->isolate());
-  if (error != ApiError::null()) {
-    return error;
+                            shared_instructions_image_, offset);
+  RawApiError* api_error = deserializer.VerifyImageAlignment();
+  if (api_error != ApiError::null()) {
+    return api_error;
   }
 
   if (Snapshot::IncludesCode(kind_)) {
diff --git a/runtime/vm/clustered_snapshot.h b/runtime/vm/clustered_snapshot.h
index f3c574e..3b863bd 100644
--- a/runtime/vm/clustered_snapshot.h
+++ b/runtime/vm/clustered_snapshot.h
@@ -429,6 +429,42 @@
   Serializer* serializer_;
 };
 
+// This class can be used to read version and features from a snapshot before
+// the VM has been initialized.
+class SnapshotHeaderReader {
+ public:
+  static char* InitializeGlobalVMFlagsFromSnapshot(const Snapshot* snapshot);
+
+  explicit SnapshotHeaderReader(const Snapshot* snapshot)
+      : SnapshotHeaderReader(snapshot->kind(),
+                             snapshot->Addr(),
+                             snapshot->length()) {}
+
+  SnapshotHeaderReader(Snapshot::Kind kind,
+                       const uint8_t* buffer,
+                       intptr_t size)
+      : kind_(kind), stream_(buffer, size) {
+    stream_.SetPosition(Snapshot::kHeaderSize);
+  }
+
+  // Verifies the version and features in the snapshot are compatible with the
+  // current VM.  If isolate is non-null it validates isolate-specific features.
+  //
+  // Returns null on success and a malloc()ed error on failure.
+  // The [offset] will be the next position in the snapshot stream after the
+  // features.
+  char* VerifyVersionAndFeatures(Isolate* isolate, intptr_t* offset);
+
+ private:
+  char* VerifyVersion();
+  char* ReadFeatures(const char** features, intptr_t* features_length);
+  char* VerifyFeatures(Isolate* isolate);
+  char* BuildError(const char* message);
+
+  Snapshot::Kind kind_;
+  ReadStream stream_;
+};
+
 class Deserializer : public ThreadStackResource {
  public:
   Deserializer(Thread* thread,
@@ -438,9 +474,16 @@
                const uint8_t* data_buffer,
                const uint8_t* instructions_buffer,
                const uint8_t* shared_data_buffer,
-               const uint8_t* shared_instructions_buffer);
+               const uint8_t* shared_instructions_buffer,
+               intptr_t offset = 0);
   ~Deserializer();
 
+  // Verifies the image alignment.
+  //
+  // Returns ApiError::null() on success and an ApiError with an an appropriate
+  // message otherwise.
+  RawApiError* VerifyImageAlignment();
+
   void ReadIsolateSnapshot(ObjectStore* object_store);
   void ReadVMSnapshot();
 
@@ -468,8 +511,6 @@
   void Advance(intptr_t value) { stream_.Advance(value); }
   void Align(intptr_t alignment) { stream_.Align(alignment); }
 
-  intptr_t PendingBytes() const { return stream_.PendingBytes(); }
-
   void AddBaseObject(RawObject* base_object) { AssignRef(base_object); }
 
   void AssignRef(RawObject* object) {
@@ -515,13 +556,6 @@
 
   void SkipHeader() { stream_.SetPosition(Snapshot::kHeaderSize); }
 
-  RawApiError* VerifyVersionAndFeatures(Isolate* isolate);
-  RawApiError* VerifyVersion();
-  RawApiError* VerifyFeatures(Isolate* isolate);
-  RawApiError* ReadFeatures(const char** features, intptr_t* features_length);
-
-  RawApiError* BuildApiError(const char* message);
-
   void Prepare();
   void Deserialize();
 
@@ -632,12 +666,12 @@
                      Thread* thread);
   ~FullSnapshotReader() {}
 
-  RawApiError* InitializeGlobalVMFlagsFromSnapshot();
-
   RawApiError* ReadVMSnapshot();
   RawApiError* ReadIsolateSnapshot();
 
  private:
+  RawApiError* ConvertToApiError(char* message);
+
   Snapshot::Kind kind_;
   Thread* thread_;
   const uint8_t* buffer_;
diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc
index e72d3e9..f39694d 100644
--- a/runtime/vm/dart.cc
+++ b/runtime/vm/dart.cc
@@ -135,6 +135,27 @@
   if (vm_isolate_ != NULL || !Flags::Initialized()) {
     return strdup("VM already initialized or flags not initialized.");
   }
+
+  const Snapshot* snapshot = nullptr;
+  if (vm_isolate_snapshot != nullptr) {
+    snapshot = Snapshot::SetupFromBuffer(vm_isolate_snapshot);
+    if (snapshot == nullptr) {
+      return strdup("Invalid vm isolate snapshot seen");
+    }
+  }
+
+  // We are initializing the VM. We will take the VM-global flags used
+  // during snapshot generation time also at runtime (this avoids the need
+  // for the embedder to pass the same flags used during snapshot generation
+  // also to the runtime).
+  if (snapshot != nullptr) {
+    char* error =
+        SnapshotHeaderReader::InitializeGlobalVMFlagsFromSnapshot(snapshot);
+    if (error != nullptr) {
+      return error;
+    }
+  }
+
 #if defined(DEBUG)
   // Turn on verify_gc_contains if any of the other GC verification flag
   // is turned on.
@@ -216,10 +237,7 @@
 #if defined(SUPPORT_TIMELINE)
       TimelineDurationScope tds(Timeline::GetVMStream(), "VMIsolateSnapshot");
 #endif
-      const Snapshot* snapshot = Snapshot::SetupFromBuffer(vm_isolate_snapshot);
-      if (snapshot == NULL) {
-        return strdup("Invalid vm isolate snapshot seen");
-      }
+      ASSERT(snapshot != nullptr);
       vm_snapshot_kind_ = snapshot->kind();
 
       if (Snapshot::IncludesCode(vm_snapshot_kind_)) {
@@ -251,13 +269,6 @@
         return strdup("Invalid vm isolate snapshot seen");
       }
       FullSnapshotReader reader(snapshot, instructions_snapshot, NULL, NULL, T);
-
-      // We are initializing the VM. We will take the VM-global flags used
-      // during snapshot generation time also at runtime (this avoids the need
-      // for the embedder to pass the same flags used during snapshot generation
-      // also to the runtime).
-      reader.InitializeGlobalVMFlagsFromSnapshot();
-
       const Error& error = Error::Handle(reader.ReadVMSnapshot());
       if (!error.IsNull()) {
         // Must copy before leaving the zone.