From 58c6d78712ed3034384740efbfd9a7f4c3bf5c96 Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Wed, 1 May 2019 00:36:14 +0000 Subject: [PATCH] [vm] Check Zones are only [un]chained on the correct side of a safepoint transition. Fix order in Dart_CreateIsolate and some tests. Change-Id: Iceb748be3f7aa6b68ce7abb7f65a3c54ad63c1d8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/100795 Commit-Queue: Ryan Macnak Reviewed-by: Alexander Markov --- runtime/vm/dart_api_impl.cc | 34 ++++++++++++++++++-------------- runtime/vm/dart_api_impl_test.cc | 2 +- runtime/vm/snapshot_test.cc | 2 +- runtime/vm/thread_test.cc | 1 + runtime/vm/zone.cc | 9 +++++++++ runtime/vm/zone_test.cc | 6 +++++- 6 files changed, 36 insertions(+), 18 deletions(-) diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index ec07f5ad6fc..8ef04732010 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/runtime/vm/dart_api_impl.cc @@ -1093,8 +1093,10 @@ static Dart_Isolate CreateIsolate(const char* script_uri, } return reinterpret_cast(NULL); } + + Thread* T = Thread::Current(); + bool success = false; { - Thread* T = Thread::Current(); StackZone zone(T); HANDLESCOPE(T); // We enter an API scope here as InitializeIsolate could compile some @@ -1112,25 +1114,27 @@ static Dart_Isolate CreateIsolate(const char* script_uri, Library::CheckFunctionFingerprints(); } #endif // defined(DART_NO_SNAPSHOT) && !defined(PRODUCT). - // We exit the API scope entered above. - T->ExitApiScope(); - // A Thread structure has been associated to the thread, we do the - // safepoint transition explicitly here instead of using the - // TransitionXXX scope objects as the reverse transition happens - // outside this scope in Dart_ShutdownIsolate/Dart_ExitIsolate. - T->set_execution_state(Thread::kThreadInNative); - T->EnterSafepoint(); - if (error != NULL) { - *error = NULL; - } - return Api::CastIsolate(I); - } - if (error != NULL) { + success = true; + } else if (error != NULL) { *error = strdup(error_obj.ToErrorCString()); } // We exit the API scope entered above. T->ExitApiScope(); } + + if (success) { + // A Thread structure has been associated to the thread, we do the + // safepoint transition explicitly here instead of using the + // TransitionXXX scope objects as the reverse transition happens + // outside this scope in Dart_ShutdownIsolate/Dart_ExitIsolate. + T->set_execution_state(Thread::kThreadInNative); + T->EnterSafepoint(); + if (error != NULL) { + *error = NULL; + } + return Api::CastIsolate(I); + } + Dart::ShutdownIsolate(); return reinterpret_cast(NULL); } diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index 406901ae02e..0682bb97cd0 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/runtime/vm/dart_api_impl_test.cc @@ -3469,8 +3469,8 @@ VM_UNIT_TEST_CASE(DartAPI_LocalHandles) { ApiLocalScope* scope = thread->api_top_scope(); Dart_Handle handles[300]; { - StackZone zone(thread); TransitionNativeToVM transition1(thread); + StackZone zone(thread); HANDLESCOPE(thread); Smi& val = Smi::Handle(); TransitionVMToNative transition2(thread); diff --git a/runtime/vm/snapshot_test.cc b/runtime/vm/snapshot_test.cc index ec774c5b14b..d11d421350a 100644 --- a/runtime/vm/snapshot_test.cc +++ b/runtime/vm/snapshot_test.cc @@ -2030,7 +2030,7 @@ VM_UNIT_TEST_CASE(PostCObject) { Dart_ExitScope(); } -TEST_CASE(OmittedObjectEncodingLength) { +ISOLATE_UNIT_TEST_CASE(OmittedObjectEncodingLength) { StackZone zone(Thread::Current()); MessageWriter writer(true); writer.WriteInlinedObjectHeader(kOmittedObjectId); diff --git a/runtime/vm/thread_test.cc b/runtime/vm/thread_test.cc index 9a4280093a7..1030795a250 100644 --- a/runtime/vm/thread_test.cc +++ b/runtime/vm/thread_test.cc @@ -376,6 +376,7 @@ TEST_CASE(ThreadRegistry) { TestCase::CreateTestIsolate(); { // Create a stack resource this time, and exercise it. + TransitionNativeToVM transition(Thread::Current()); StackZone stack_zone(Thread::Current()); Zone* zone1 = Thread::Current()->zone(); EXPECT(zone1 != zone0); diff --git a/runtime/vm/zone.cc b/runtime/vm/zone.cc index 2d95479bd23..595871ada1c 100644 --- a/runtime/vm/zone.cc +++ b/runtime/vm/zone.cc @@ -337,11 +337,20 @@ StackZone::StackZone(ThreadState* thread) : StackResource(thread), zone_() { reinterpret_cast(this), reinterpret_cast(&zone_)); } + + // This thread must be preventing safepoints or the GC could be visiting the + // chain of handle blocks we're about the mutate. + ASSERT(Thread::Current()->MayAllocateHandles()); + zone_.Link(thread->zone()); thread->set_zone(&zone_); } StackZone::~StackZone() { + // This thread must be preventing safepoints or the GC could be visiting the + // chain of handle blocks we're about the mutate. + ASSERT(Thread::Current()->MayAllocateHandles()); + ASSERT(thread()->zone() == &zone_); thread()->set_zone(zone_.previous_); if (FLAG_trace_zones) { diff --git a/runtime/vm/zone_test.cc b/runtime/vm/zone_test.cc index aeb98d6f98d..e52b940628d 100644 --- a/runtime/vm/zone_test.cc +++ b/runtime/vm/zone_test.cc @@ -18,6 +18,7 @@ VM_UNIT_TEST_CASE(AllocateZone) { Thread* thread = Thread::Current(); EXPECT(thread->zone() == NULL); { + TransitionNativeToVM transition(thread); StackZone stack_zone(thread); EXPECT(thread->zone() != NULL); Zone* zone = stack_zone.GetZone(); @@ -78,6 +79,7 @@ VM_UNIT_TEST_CASE(AllocGeneric_Success) { Thread* thread = Thread::Current(); EXPECT(thread->zone() == NULL); { + TransitionNativeToVM transition(thread); StackZone zone(thread); EXPECT(thread->zone() != NULL); uintptr_t allocated_size = 0; @@ -131,6 +133,7 @@ VM_UNIT_TEST_CASE(ZoneAllocated) { // Create a few zone allocated objects. { + TransitionNativeToVM transition(thread); StackZone zone(thread); EXPECT_EQ(0UL, zone.SizeInBytes()); SimpleZoneObject* first = new SimpleZoneObject(); @@ -156,6 +159,7 @@ VM_UNIT_TEST_CASE(ZoneAllocated) { } TEST_CASE(PrintToString) { + TransitionNativeToVM transition(Thread::Current()); StackZone zone(Thread::Current()); const char* result = zone.GetZone()->PrintToString("Hello %s!", "World"); EXPECT_STREQ("Hello World!", result); @@ -219,7 +223,7 @@ TEST_CASE(StressMallocDirectly) { #endif // !defined(PRODUCT) } -TEST_CASE(StressMallocThroughZones) { +ISOLATE_UNIT_TEST_CASE(StressMallocThroughZones) { #if !defined(PRODUCT) int64_t start_rss = Service::CurrentRSS(); #endif // !defined(PRODUCT)