Alexander Markov would like Ryan Macnak, Siva Annamalai and Régis Crelier to review this change.
[vm/nnbd] Add separate Snapshot::Kind for core snapshots
Core snapshots should be agnostic to the sound null safety mode
(so they can be used both in weak and strong modes), and snapshot
writer verifies that.
Snapshot::kFull was previously used both for core snapshots and
app snapshots on ia32. However, app snapshots are not guaranteed to
be agnostic, which appeared as failures on a few test on ia32.
Also, VM should be able to detect null safety mode from app snapshots,
even if they do not contain code, but null safety mode was not
written into features string of kFull snapshots.
In order to disambiguate core snapshots, a new Snapshot::Kind is
added. Snapshot::kFullCore works exactly as Snapshot::kFull, except
for verification of agnostic null safety and snapshot features string
omitting null safety mode. All snapshots except kFullCore now have
null safety mode included into their features string.
Fixes https://github.com/dart-lang/sdk/issues/43626
Issue https://github.com/dart-lang/sdk/issues/43613
Change-Id: I8cd3b049ef4e428dd5e1ce666d4c7aa3b596d70c
---
M runtime/bin/gen_snapshot.cc
M runtime/bin/snapshot_utils.cc
M runtime/include/dart_api.h
M runtime/vm/benchmark_test.cc
M runtime/vm/clustered_snapshot.cc
M runtime/vm/dart.cc
M runtime/vm/dart_api_impl.cc
M runtime/vm/object_store.h
M runtime/vm/raw_object.h
M runtime/vm/snapshot.cc
M runtime/vm/snapshot.h
M runtime/vm/snapshot_test.cc
12 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/runtime/bin/gen_snapshot.cc b/runtime/bin/gen_snapshot.cc
index 187cf92..eba26b5 100644
--- a/runtime/bin/gen_snapshot.cc
+++ b/runtime/bin/gen_snapshot.cc
@@ -446,7 +446,8 @@
// First create a snapshot.
result = Dart_CreateSnapshot(&vm_snapshot_data_buffer, &vm_snapshot_data_size,
&isolate_snapshot_data_buffer,
- &isolate_snapshot_data_size);
+ &isolate_snapshot_data_size,
+ /*is_core=*/true);
CHECK_RESULT(result);
// Now write the vm isolate and isolate snapshots out to the
@@ -539,7 +540,7 @@
intptr_t isolate_snapshot_data_size = 0;
result = Dart_CreateSnapshot(NULL, NULL, &isolate_snapshot_data_buffer,
- &isolate_snapshot_data_size);
+ &isolate_snapshot_data_size, /*is_core=*/false);
CHECK_RESULT(result);
WriteFile(isolate_snapshot_data_filename, isolate_snapshot_data_buffer,
diff --git a/runtime/bin/snapshot_utils.cc b/runtime/bin/snapshot_utils.cc
index d31be2d..4a19ba2 100644
--- a/runtime/bin/snapshot_utils.cc
+++ b/runtime/bin/snapshot_utils.cc
@@ -489,8 +489,8 @@
uint8_t* isolate_buffer = NULL;
intptr_t isolate_size = 0;
- Dart_Handle result =
- Dart_CreateSnapshot(NULL, NULL, &isolate_buffer, &isolate_size);
+ Dart_Handle result = Dart_CreateSnapshot(NULL, NULL, &isolate_buffer,
+ &isolate_size, /*is_core=*/false);
if (Dart_IsError(result)) {
ErrorExit(kErrorExitCode, "%s\n", Dart_GetError(result));
}
diff --git a/runtime/include/dart_api.h b/runtime/include/dart_api.h
index 5268f5d..5d7d117 100644
--- a/runtime/include/dart_api.h
+++ b/runtime/include/dart_api.h
@@ -1242,6 +1242,8 @@
* snapshot. This buffer is scope allocated and is only valid
* until the next call to Dart_ExitScope.
* \param size Returns the size of the buffer.
+ * \param is_core Create a snapshot containing core libraries.
+ * Such snapshot should be agnostic to null safety mode.
*
* \return A valid handle if no error occurs during the operation.
*/
@@ -1249,7 +1251,8 @@
Dart_CreateSnapshot(uint8_t** vm_snapshot_data_buffer,
intptr_t* vm_snapshot_data_size,
uint8_t** isolate_snapshot_data_buffer,
- intptr_t* isolate_snapshot_data_size);
+ intptr_t* isolate_snapshot_data_size,
+ bool is_core);
/**
* Returns whether the buffer contains a kernel file.
diff --git a/runtime/vm/benchmark_test.cc b/runtime/vm/benchmark_test.cc
index 2215c7d..f7b1266 100644
--- a/runtime/vm/benchmark_test.cc
+++ b/runtime/vm/benchmark_test.cc
@@ -528,12 +528,12 @@
MallocWriteStream vm_snapshot_data(FullSnapshotWriter::kInitialSize);
MallocWriteStream isolate_snapshot_data(FullSnapshotWriter::kInitialSize);
FullSnapshotWriter writer(
- Snapshot::kFull, &vm_snapshot_data, &isolate_snapshot_data,
+ Snapshot::kFullCore, &vm_snapshot_data, &isolate_snapshot_data,
/*vm_image_writer=*/nullptr, /*iso_image_writer=*/nullptr);
writer.WriteFullSnapshot();
const Snapshot* snapshot =
Snapshot::SetupFromBuffer(isolate_snapshot_data.buffer());
- ASSERT(snapshot->kind() == Snapshot::kFull);
+ ASSERT(snapshot->kind() == Snapshot::kFullCore);
benchmark->set_score(snapshot->length());
}
@@ -566,12 +566,12 @@
MallocWriteStream vm_snapshot_data(FullSnapshotWriter::kInitialSize);
MallocWriteStream isolate_snapshot_data(FullSnapshotWriter::kInitialSize);
FullSnapshotWriter writer(
- Snapshot::kFull, &vm_snapshot_data, &isolate_snapshot_data,
+ Snapshot::kFullCore, &vm_snapshot_data, &isolate_snapshot_data,
/*vm_image_writer=*/nullptr, /*iso_image_writer=*/nullptr);
writer.WriteFullSnapshot();
const Snapshot* snapshot =
Snapshot::SetupFromBuffer(isolate_snapshot_data.buffer());
- ASSERT(snapshot->kind() == Snapshot::kFull);
+ ASSERT(snapshot->kind() == Snapshot::kFullCore);
benchmark->set_score(snapshot->length());
}
diff --git a/runtime/vm/clustered_snapshot.cc b/runtime/vm/clustered_snapshot.cc
index 7fb5eca..27d263a 100644
--- a/runtime/vm/clustered_snapshot.cc
+++ b/runtime/vm/clustered_snapshot.cc
@@ -245,7 +245,8 @@
s->UnexpectedObject(cls, "Class with illegal cid");
}
s->WriteCid(class_id);
- if (s->kind() == Snapshot::kFull && RequireLegacyErasureOfConstants(cls)) {
+ if (s->kind() == Snapshot::kFullCore &&
+ RequireLegacyErasureOfConstants(cls)) {
s->UnexpectedObject(cls, "Class with non mode agnostic constants");
}
if (s->kind() != Snapshot::kFullAOT) {
@@ -596,7 +597,7 @@
objects_.Add(func);
PushFromTo(func);
- if (kind == Snapshot::kFull) {
+ if ((kind == Snapshot::kFull) || (kind == Snapshot::kFullCore)) {
NOT_IN_PRECOMPILED(s->Push(func->ptr()->bytecode_));
} else if (kind == Snapshot::kFullAOT) {
s->Push(func->ptr()->code_);
@@ -625,7 +626,7 @@
FunctionPtr func = objects_[i];
AutoTraceObjectName(func, MakeDisambiguatedFunctionName(s, func));
WriteFromTo(func);
- if (kind == Snapshot::kFull) {
+ if ((kind == Snapshot::kFull) || (kind == Snapshot::kFullCore)) {
NOT_IN_PRECOMPILED(WriteField(func, bytecode_));
} else if (kind == Snapshot::kFullAOT) {
WriteField(func, code_);
@@ -692,7 +693,7 @@
Function::InstanceSize());
ReadFromTo(func);
- if (kind == Snapshot::kFull) {
+ if ((kind == Snapshot::kFull) || (kind == Snapshot::kFullCore)) {
NOT_IN_PRECOMPILED(func->ptr()->bytecode_ =
static_cast<BytecodePtr>(d->ReadRef()));
} else if (kind == Snapshot::kFullAOT) {
diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc
index f271e57..dac8c18 100644
--- a/runtime/vm/dart.cc
+++ b/runtime/vm/dart.cc
@@ -782,12 +782,12 @@
// when generating the snapshot.
ASSERT(FLAG_sound_null_safety == kNullSafetyOptionUnspecified);
- // If snapshot is an appJIT/AOT snapshot we will figure out the mode by
+ // If snapshot is not a core snapshot we will figure out the mode by
// sniffing the feature string in the snapshot.
if (snapshot_data != nullptr) {
// Read the snapshot and check for null safety option.
const Snapshot* snapshot = Snapshot::SetupFromBuffer(snapshot_data);
- if (Snapshot::IncludesCode(snapshot->kind())) {
+ if (!Snapshot::IsAgnosticToNullSafety(snapshot->kind())) {
return SnapshotHeaderReader::NullSafetyFromSnapshot(snapshot);
}
}
@@ -1034,7 +1034,9 @@
#else
#error What architecture?
#endif
+ }
+ if (!Snapshot::IsAgnosticToNullSafety(kind)) {
if (isolate != NULL) {
if (isolate->null_safety()) {
buffer.AddString(" null-safety");
diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc
index 291f8f7..6fdb197 100644
--- a/runtime/vm/dart_api_impl.cc
+++ b/runtime/vm/dart_api_impl.cc
@@ -1934,7 +1934,8 @@
Dart_CreateSnapshot(uint8_t** vm_snapshot_data_buffer,
intptr_t* vm_snapshot_data_size,
uint8_t** isolate_snapshot_data_buffer,
- intptr_t* isolate_snapshot_data_size) {
+ intptr_t* isolate_snapshot_data_size,
+ bool is_core) {
#if defined(DART_PRECOMPILED_RUNTIME)
return Api::NewError("Cannot create snapshots on an AOT runtime.");
#else
@@ -1966,8 +1967,10 @@
FullSnapshotWriter::kInitialSize);
ZoneWriteStream isolate_snapshot_data(Api::TopScope(T)->zone(),
FullSnapshotWriter::kInitialSize);
+ const Snapshot::Kind snapshot_kind =
+ is_core ? Snapshot::kFullCore : Snapshot::kFull;
FullSnapshotWriter writer(
- Snapshot::kFull, &vm_snapshot_data, &isolate_snapshot_data,
+ snapshot_kind, &vm_snapshot_data, &isolate_snapshot_data,
nullptr /* vm_image_writer */, nullptr /* isolate_image_writer */);
writer.WriteFullSnapshot();
if (vm_snapshot_data_buffer != nullptr) {
diff --git a/runtime/vm/object_store.h b/runtime/vm/object_store.h
index 05f290c..fa649a5 100644
--- a/runtime/vm/object_store.h
+++ b/runtime/vm/object_store.h
@@ -479,6 +479,7 @@
ObjectPtr* to_snapshot(Snapshot::Kind kind) {
switch (kind) {
case Snapshot::kFull:
+ case Snapshot::kFullCore:
return reinterpret_cast<ObjectPtr*>(&global_object_pool_);
case Snapshot::kFullJIT:
case Snapshot::kFullAOT:
diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h
index 9dd7f8f..5f03f71 100644
--- a/runtime/vm/raw_object.h
+++ b/runtime/vm/raw_object.h
@@ -762,6 +762,7 @@
case Snapshot::kFullAOT:
return reinterpret_cast<ObjectPtr*>(&allocation_stub_);
case Snapshot::kFull:
+ case Snapshot::kFullCore:
return reinterpret_cast<ObjectPtr*>(&direct_subclasses_);
case Snapshot::kFullJIT:
return reinterpret_cast<ObjectPtr*>(&dependent_code_);
@@ -836,6 +837,7 @@
case Snapshot::kFullAOT:
return reinterpret_cast<ObjectPtr*>(&script_);
case Snapshot::kFull:
+ case Snapshot::kFullCore:
case Snapshot::kFullJIT:
return reinterpret_cast<ObjectPtr*>(&library_kernel_data_);
case Snapshot::kMessage:
@@ -1026,6 +1028,7 @@
switch (kind) {
case Snapshot::kFullAOT:
case Snapshot::kFull:
+ case Snapshot::kFullCore:
case Snapshot::kFullJIT:
return reinterpret_cast<ObjectPtr*>(&data_);
case Snapshot::kMessage:
@@ -1194,6 +1197,7 @@
ObjectPtr* to_snapshot(Snapshot::Kind kind) {
switch (kind) {
case Snapshot::kFull:
+ case Snapshot::kFullCore:
case Snapshot::kFullJIT:
case Snapshot::kFullAOT:
return reinterpret_cast<ObjectPtr*>(&initializer_function_);
@@ -1270,6 +1274,7 @@
case Snapshot::kFullAOT:
return reinterpret_cast<ObjectPtr*>(&url_);
case Snapshot::kFull:
+ case Snapshot::kFullCore:
case Snapshot::kFullJIT:
return reinterpret_cast<ObjectPtr*>(&kernel_program_info_);
case Snapshot::kMessage:
@@ -1343,6 +1348,7 @@
case Snapshot::kFullAOT:
return reinterpret_cast<ObjectPtr*>(&exports_);
case Snapshot::kFull:
+ case Snapshot::kFullCore:
case Snapshot::kFullJIT:
return reinterpret_cast<ObjectPtr*>(&kernel_data_);
case Snapshot::kMessage:
@@ -2067,6 +2073,7 @@
case Snapshot::kFullAOT:
return reinterpret_cast<ObjectPtr*>(&entries_);
case Snapshot::kFull:
+ case Snapshot::kFullCore:
case Snapshot::kFullJIT:
return to();
case Snapshot::kMessage:
@@ -2174,6 +2181,7 @@
case Snapshot::kFullAOT:
return reinterpret_cast<ObjectPtr*>(&imports_);
case Snapshot::kFull:
+ case Snapshot::kFullCore:
case Snapshot::kFullJIT:
return reinterpret_cast<ObjectPtr*>(&importer_);
case Snapshot::kMessage:
diff --git a/runtime/vm/snapshot.cc b/runtime/vm/snapshot.cc
index 9403472..5c0d911 100644
--- a/runtime/vm/snapshot.cc
+++ b/runtime/vm/snapshot.cc
@@ -229,6 +229,8 @@
switch (kind) {
case kFull:
return "full";
+ case kFullCore:
+ return "full-core";
case kFullJIT:
return "full-jit";
case kFullAOT:
diff --git a/runtime/vm/snapshot.h b/runtime/vm/snapshot.h
index d685fb2..94aa3e7 100644
--- a/runtime/vm/snapshot.h
+++ b/runtime/vm/snapshot.h
@@ -91,11 +91,12 @@
class Snapshot {
public:
enum Kind {
- kFull, // Full snapshot of core libraries or an application.
- kFullJIT, // Full + JIT code
- kFullAOT, // Full + AOT code
- kMessage, // A partial snapshot used only for isolate messaging.
- kNone, // gen_snapshot
+ kFull, // Full snapshot of an application.
+ kFullCore, // Full snapshot of core libraries. Agnostic to null safety.
+ kFullJIT, // Full + JIT code
+ kFullAOT, // Full + AOT code
+ kMessage, // A partial snapshot used only for isolate messaging.
+ kNone, // gen_snapshot
kInvalid
};
static const char* KindToCString(Kind kind);
@@ -130,13 +131,15 @@
void set_kind(Kind value) { return Write<int64_t>(kKindOffset, value); }
static bool IsFull(Kind kind) {
- return (kind == kFull) || (kind == kFullJIT) || (kind == kFullAOT);
+ return (kind == kFull) || (kind == kFullCore) || (kind == kFullJIT) ||
+ (kind == kFullAOT);
}
+ static bool IsAgnosticToNullSafety(Kind kind) { return (kind == kFullCore); }
static bool IncludesCode(Kind kind) {
return (kind == kFullJIT) || (kind == kFullAOT);
}
static bool IncludesBytecode(Kind kind) {
- return (kind == kFull) || (kind == kFullJIT);
+ return (kind == kFull) || (kind == kFullCore) || (kind == kFullJIT);
}
const uint8_t* Addr() const { return reinterpret_cast<const uint8_t*>(this); }
@@ -171,7 +174,8 @@
inline static bool IsSnapshotCompatible(Snapshot::Kind vm_kind,
Snapshot::Kind isolate_kind) {
if (vm_kind == isolate_kind) return true;
- if (vm_kind == Snapshot::kFull && isolate_kind == Snapshot::kFullJIT)
+ if (((vm_kind == Snapshot::kFull) || (vm_kind == Snapshot::kFullCore)) &&
+ isolate_kind == Snapshot::kFullJIT)
return true;
return Snapshot::IsFull(isolate_kind);
}
diff --git a/runtime/vm/snapshot_test.cc b/runtime/vm/snapshot_test.cc
index 4ff7ccf..ed0383c 100644
--- a/runtime/vm/snapshot_test.cc
+++ b/runtime/vm/snapshot_test.cc
@@ -2073,7 +2073,8 @@
// Write snapshot with object content.
MallocWriteStream isolate_snapshot_data(FullSnapshotWriter::kInitialSize);
FullSnapshotWriter writer(
- Snapshot::kFull, /*vm_snapshot_data=*/nullptr, &isolate_snapshot_data,
+ Snapshot::kFullCore, /*vm_snapshot_data=*/nullptr,
+ &isolate_snapshot_data,
/*vm_image_writer=*/nullptr, /*iso_image_writer=*/nullptr);
writer.WriteFullSnapshot();
}
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
Please take a look.
Failing language_2/type_promotion/assignment_defeats_promotion_lhs_and_test is not related to this change: it also fails on the CI bots.
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 1:Code-Review +1
1 comment:
Patchset:
Thanks for fixing this, Alex!
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 1:Commit-Queue +2
1 comment:
Patchset:
Thank you for the review, Regis.
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
Failed builds:
luci.dart.try/vm-kernel-linux-release-x64-try JOB_FAILED https://cr-buildbucket.appspot.com/build/8866985921050472032
1 out of 1 aggregated steps failed: Step('test results') (retcode: 1)
Patch set 1:Commit-Queue +2
commi...@chromium.org submitted this change.
[vm/nnbd] Add separate Snapshot::Kind for core snapshots
Core snapshots should be agnostic to the sound null safety mode
(so they can be used both in weak and strong modes), and snapshot
writer verifies that.
Snapshot::kFull was previously used both for core snapshots and
app snapshots on ia32. However, app snapshots are not guaranteed to
be agnostic, which appeared as failures on a few test on ia32.
Also, VM should be able to detect null safety mode from app snapshots,
even if they do not contain code, but null safety mode was not
written into features string of kFull snapshots.
In order to disambiguate core snapshots, a new Snapshot::Kind is
added. Snapshot::kFullCore works exactly as Snapshot::kFull, except
for verification of agnostic null safety and snapshot features string
omitting null safety mode. All snapshots except kFullCore now have
null safety mode included into their features string.
Fixes https://github.com/dart-lang/sdk/issues/43626
Issue https://github.com/dart-lang/sdk/issues/43613
Change-Id: I8cd3b049ef4e428dd5e1ce666d4c7aa3b596d70c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166308
Reviewed-by: Régis Crelier <re...@google.com>
Commit-Queue: Alexander Markov <alexm...@google.com>
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
I don't believe we create core application snapshots anywhere, the use cases in the Dart SDK are all for app-jit snapshots. I am wondering if we can just get rid of plain application snapshots.
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
I don't believe we create core application snapshots anywhere, the use cases in the Dart SDK are all […]
We use them on IA32 when app-jit snapshot is requested.
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
We use them on IA32 when app-jit snapshot is requested.
Is there a shipping version of an IA32 app snapshot ? Not sure if we build a IA32 sdk anymore (maybe on windows)
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
Is there a shipping version of an IA32 app snapshot ? Not sure if we build a IA32 sdk anymore (maybe […]
Are you saying we should drop support for app snapshots, i.e.
gen_snapshot --snapshot_kind=app
on all platforms and
dart --snapshot-kind=app-jit
on IA32? Are you sure they are not used anywhere? Flutter for android/x86?
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
Are you saying we should drop support for app snapshots, i.e. […]
Possible cleanup CL: https://dart-review.googlesource.com/c/sdk/+/166792
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
Hello! This change probably broke all or most Dart snapshot parsers in existence, since JIT & AOT snapshots now have a different kind value.
I know the snapshot format has no ABI stability guarantees and I have to review each of your commits, but being a basic enum that goes in the header, this means tools now need to match the snapshot hash before they can even know the kind of snapshot that is being parsed.
I would really appreciate if this could be kept in mind a bit in the future, especially in cases like this one where preserving ABI only takes a minor change.
To view, visit change 166308. To unsubscribe, or for help writing mail filters, visit settings.