From 23792e78168748946ead2d894990d111ee2c26cd Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Thu, 19 Mar 2020 16:19:59 +0000 Subject: [PATCH] [vm] Require explicit loads and stores when using AcqRelAtomic. Change-Id: I19d64667cea7dd735c3bf7194285383289019aaa Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/139942 Reviewed-by: Alexander Aprelev Reviewed-by: Martin Kustermann Commit-Queue: Ryan Macnak --- runtime/platform/atomic.h | 22 ++++----- runtime/vm/class_table.cc | 94 +++++++++++++++++++++------------------ runtime/vm/class_table.h | 30 ++++++------- 3 files changed, 73 insertions(+), 73 deletions(-) diff --git a/runtime/platform/atomic.h b/runtime/platform/atomic.h index 7cdfb12d309..7a966f5adb5 100644 --- a/runtime/platform/atomic.h +++ b/runtime/platform/atomic.h @@ -74,8 +74,8 @@ template class AcqRelAtomic { public: constexpr AcqRelAtomic() : value_() {} - constexpr AcqRelAtomic(T arg) : value_(arg) {} // NOLINT - AcqRelAtomic(const AcqRelAtomic& arg) : value_(arg) {} // NOLINT + constexpr AcqRelAtomic(T arg) : value_(arg) {} // NOLINT + AcqRelAtomic(const AcqRelAtomic& arg) = delete; T load(std::memory_order order = std::memory_order_acquire) const { return value_.load(order); @@ -110,18 +110,12 @@ class AcqRelAtomic { return value_.compare_exchange_strong(expected, desired, order, order); } - operator T() const { return load(); } - T operator=(T arg) { - store(arg); - return arg; - } - T operator=(const AcqRelAtomic& arg) { - T loaded_once = arg; - store(loaded_once); - return loaded_once; - } - T operator+=(T arg) { return fetch_add(arg) + arg; } - T operator-=(T arg) { return fetch_sub(arg) - arg; } + // Require explicit loads and stores. + operator T() const = delete; + T operator=(T arg) = delete; + T operator=(const AcqRelAtomic& arg) = delete; + T operator+=(T arg) = delete; + T operator-=(T arg) = delete; private: std::atomic value_; diff --git a/runtime/vm/class_table.cc b/runtime/vm/class_table.cc index 0a08dbab0a3..d19460a0a59 100644 --- a/runtime/vm/class_table.cc +++ b/runtime/vm/class_table.cc @@ -27,28 +27,27 @@ SharedClassTable::SharedClassTable() ASSERT(kInitialCapacity >= kNumPredefinedCids); capacity_ = kInitialCapacity; // Note that [calloc] will zero-initialize the memory. - table_ = reinterpret_cast*>( - calloc(capacity_, sizeof(RelaxedAtomic))); + table_.store(reinterpret_cast*>( + calloc(capacity_, sizeof(RelaxedAtomic)))); } else { // Duplicate the class table from the VM isolate. auto vm_shared_class_table = Dart::vm_isolate()->group()->class_table(); capacity_ = vm_shared_class_table->capacity_; // Note that [calloc] will zero-initialize the memory. - table_ = reinterpret_cast*>( + RelaxedAtomic* table = reinterpret_cast*>( calloc(capacity_, sizeof(RelaxedAtomic))); // The following cids don't have a corresponding class object in Dart code. // We therefore need to initialize them eagerly. for (intptr_t i = kObjectCid; i < kInstanceCid; i++) { - table_[i] = vm_shared_class_table->SizeAt(i); + table[i] = vm_shared_class_table->SizeAt(i); } - table_[kTypeArgumentsCid] = - vm_shared_class_table->SizeAt(kTypeArgumentsCid); - table_[kFreeListElement] = vm_shared_class_table->SizeAt(kFreeListElement); - table_[kForwardingCorpse] = - vm_shared_class_table->SizeAt(kForwardingCorpse); - table_[kDynamicCid] = vm_shared_class_table->SizeAt(kDynamicCid); - table_[kVoidCid] = vm_shared_class_table->SizeAt(kVoidCid); - table_[kNeverCid] = vm_shared_class_table->SizeAt(kNeverCid); + table[kTypeArgumentsCid] = vm_shared_class_table->SizeAt(kTypeArgumentsCid); + table[kFreeListElement] = vm_shared_class_table->SizeAt(kFreeListElement); + table[kForwardingCorpse] = vm_shared_class_table->SizeAt(kForwardingCorpse); + table[kDynamicCid] = vm_shared_class_table->SizeAt(kDynamicCid); + table[kVoidCid] = vm_shared_class_table->SizeAt(kVoidCid); + table[kNeverCid] = vm_shared_class_table->SizeAt(kNeverCid); + table_.store(table); } #if defined(SUPPORT_UNBOXED_INSTANCE_FIELDS) // Note that [calloc] will zero-initialize the memory. @@ -57,8 +56,8 @@ SharedClassTable::SharedClassTable() #endif // defined(SUPPORT_UNBOXED_INSTANCE_FIELDS) #ifndef PRODUCT // Note that [calloc] will zero-initialize the memory. - trace_allocation_table_ = - static_cast(calloc(capacity_, sizeof(uint8_t))); + trace_allocation_table_.store( + static_cast(calloc(capacity_, sizeof(uint8_t)))); #endif // !PRODUCT } SharedClassTable::~SharedClassTable() { @@ -66,10 +65,10 @@ SharedClassTable::~SharedClassTable() { FreeOldTables(); delete old_tables_; } - free(table_); + free(table_.load()); free(unboxed_fields_map_); - NOT_IN_PRODUCT(free(trace_allocation_table_)); + NOT_IN_PRODUCT(free(trace_allocation_table_.load())); } ClassTable::ClassTable(SharedClassTable* shared_class_table) @@ -82,7 +81,7 @@ ClassTable::ClassTable(SharedClassTable* shared_class_table) ASSERT(kInitialCapacity >= kNumPredefinedCids); capacity_ = kInitialCapacity; // Note that [calloc] will zero-initialize the memory. - table_ = static_cast(calloc(capacity_, sizeof(RawClass*))); + table_.store(static_cast(calloc(capacity_, sizeof(RawClass*)))); } else { // Duplicate the class table from the VM isolate. ClassTable* vm_class_table = Dart::vm_isolate()->class_table(); @@ -101,7 +100,7 @@ ClassTable::ClassTable(SharedClassTable* shared_class_table) table[kDynamicCid] = vm_class_table->At(kDynamicCid); table[kVoidCid] = vm_class_table->At(kVoidCid); table[kNeverCid] = vm_class_table->At(kNeverCid); - table_ = table; + table_.store(table); } } @@ -110,7 +109,7 @@ ClassTable::~ClassTable() { FreeOldTables(); delete old_class_tables_; } - free(table_); + free(table_.load()); } void ClassTable::AddOldTable(RawClass** old_class_table) { @@ -151,8 +150,8 @@ void ClassTable::Register(const Class& cls) { if (index != kIllegalCid) { ASSERT(index > 0 && index < kNumPredefinedCids && index < top_); - ASSERT(table_[index] == nullptr); - table_[index] = cls.raw(); + ASSERT(table_.load()[index] == nullptr); + table_.load()[index] = cls.raw(); } else { if (top_ == capacity_) { const intptr_t new_capacity = capacity_ + kCapacityIncrement; @@ -160,7 +159,7 @@ void ClassTable::Register(const Class& cls) { } ASSERT(top_ < capacity_); cls.set_id(top_); - table_[top_] = cls.raw(); + table_.load()[top_] = cls.raw(); top_++; // Increment next index. } ASSERT(expected_cid == cls.id()); @@ -185,7 +184,7 @@ intptr_t SharedClassTable::Register(intptr_t index, intptr_t size) { Grow(new_capacity); } ASSERT(top_ < capacity_); - table_[top_] = size; + table_.load()[top_] = size; return top_++; // Increment next index. } } @@ -200,7 +199,7 @@ void ClassTable::AllocateIndex(intptr_t index) { Grow(new_capacity); } - ASSERT(table_[index] == nullptr); + ASSERT(table_.load()[index] == nullptr); if (index >= top_) { top_ = index + 1; } @@ -212,13 +211,14 @@ void ClassTable::AllocateIndex(intptr_t index) { void ClassTable::Grow(intptr_t new_capacity) { ASSERT(new_capacity > capacity_); + auto old_table = table_.load(); auto new_table = static_cast( malloc(new_capacity * sizeof(RawClass*))); // NOLINT - memmove(new_table, table_, capacity_ * sizeof(RawClass*)); + memmove(new_table, old_table, capacity_ * sizeof(RawClass*)); memset(new_table + capacity_, 0, (new_capacity - capacity_) * sizeof(RawClass*)); - old_class_tables_->Add(table_); - table_ = new_table; + old_class_tables_->Add(old_table); + table_.store(new_table); capacity_ = new_capacity; } @@ -232,7 +232,7 @@ void SharedClassTable::AllocateIndex(intptr_t index) { Grow(new_capacity); } - ASSERT(table_[index] == 0); + ASSERT(table_.load()[index] == 0); if (index >= top_) { top_ = index + 1; } @@ -241,27 +241,28 @@ void SharedClassTable::AllocateIndex(intptr_t index) { void SharedClassTable::Grow(intptr_t new_capacity) { ASSERT(new_capacity >= capacity_); + RelaxedAtomic* old_table = table_.load(); RelaxedAtomic* new_table = reinterpret_cast*>( malloc(new_capacity * sizeof(RelaxedAtomic))); // NOLINT - memmove(new_table, table_, capacity_ * sizeof(intptr_t)); + memmove(new_table, old_table, capacity_ * sizeof(intptr_t)); memset(new_table + capacity_, 0, (new_capacity - capacity_) * sizeof(intptr_t)); #if !defined(PRODUCT) + auto old_trace_table = trace_allocation_table_.load(); auto new_trace_table = static_cast(malloc(new_capacity * sizeof(uint8_t))); // NOLINT - memmove(new_trace_table, trace_allocation_table_, - capacity_ * sizeof(uint8_t)); + memmove(new_trace_table, old_trace_table, capacity_ * sizeof(uint8_t)); memset(new_trace_table + capacity_, 0, (new_capacity - capacity_) * sizeof(uint8_t)); #endif - old_tables_->Add(table_); - table_ = new_table; - NOT_IN_PRODUCT(old_tables_->Add(trace_allocation_table_)); - NOT_IN_PRODUCT(trace_allocation_table_ = new_trace_table); + old_tables_->Add(old_table); + table_.store(new_table); + NOT_IN_PRODUCT(old_tables_->Add(old_trace_table)); + NOT_IN_PRODUCT(trace_allocation_table_.store(new_trace_table)); #if defined(SUPPORT_UNBOXED_INSTANCE_FIELDS) auto new_unboxed_fields_map = static_cast( @@ -279,11 +280,11 @@ void SharedClassTable::Grow(intptr_t new_capacity) { void ClassTable::Unregister(intptr_t index) { shared_class_table_->Unregister(index); - table_[index] = nullptr; + table_.load()[index] = nullptr; } void SharedClassTable::Unregister(intptr_t index) { - table_[index] = 0; + table_.load()[index] = 0; #if defined(SUPPORT_UNBOXED_INSTANCE_FIELDS) unboxed_fields_map_[index].Reset(); #endif // defined(SUPPORT_UNBOXED_INSTANCE_FIELDS) @@ -293,9 +294,10 @@ void ClassTable::Remap(intptr_t* old_to_new_cid) { ASSERT(Thread::Current()->IsAtSafepoint()); const intptr_t num_cids = NumCids(); std::unique_ptr cls_by_old_cid(new RawClass*[num_cids]); - memmove(cls_by_old_cid.get(), table_, sizeof(RawClass*) * num_cids); + auto* table = table_.load(); + memmove(cls_by_old_cid.get(), table, sizeof(RawClass*) * num_cids); for (intptr_t i = 0; i < num_cids; i++) { - table_[old_to_new_cid[i]] = cls_by_old_cid[i]; + table[old_to_new_cid[i]] = cls_by_old_cid[i]; } } @@ -303,11 +305,12 @@ void SharedClassTable::Remap(intptr_t* old_to_new_cid) { ASSERT(Thread::Current()->IsAtSafepoint()); const intptr_t num_cids = NumCids(); std::unique_ptr size_by_old_cid(new intptr_t[num_cids]); + auto* table = table_.load(); for (intptr_t i = 0; i < num_cids; i++) { - size_by_old_cid[i] = table_[i]; + size_by_old_cid[i] = table[i]; } for (intptr_t i = 0; i < num_cids; i++) { - table_[old_to_new_cid[i]] = size_by_old_cid[i]; + table[old_to_new_cid[i]] = size_by_old_cid[i]; } #if defined(SUPPORT_UNBOXED_INSTANCE_FIELDS) @@ -325,8 +328,11 @@ void SharedClassTable::Remap(intptr_t* old_to_new_cid) { void ClassTable::VisitObjectPointers(ObjectPointerVisitor* visitor) { ASSERT(visitor != NULL); visitor->set_gc_root_type("class table"); - for (intptr_t i = 0; i < top_; i++) { - visitor->VisitPointer(reinterpret_cast(&(table_[i]))); + if (top_ != 0) { + auto* table = table_.load(); + RawObject** from = reinterpret_cast(&table[0]); + RawObject** to = reinterpret_cast(&table[top_ - 1]); + visitor->VisitPointers(from, to); } visitor->clear_gc_root_type(); } @@ -378,7 +384,7 @@ void ClassTable::SetAt(intptr_t index, RawClass* raw_cls) { const intptr_t size = raw_cls == nullptr ? 0 : Class::host_instance_size(raw_cls); shared_class_table_->SetSizeAt(index, size); - table_[index] = raw_cls; + table_.load()[index] = raw_cls; } #ifndef PRODUCT diff --git a/runtime/vm/class_table.h b/runtime/vm/class_table.h index 1f43f2fcaed..eb5a431bfe4 100644 --- a/runtime/vm/class_table.h +++ b/runtime/vm/class_table.h @@ -71,13 +71,13 @@ class SharedClassTable { // Thread-safe. intptr_t SizeAt(intptr_t index) const { ASSERT(IsValidIndex(index)); - return table_[index]; + return table_.load()[index]; } bool HasValidClassAt(intptr_t index) const { ASSERT(IsValidIndex(index)); - ASSERT(table_[index] >= 0); - return table_[index] != 0; + ASSERT(table_.load()[index] >= 0); + return table_.load()[index] != 0; } void SetSizeAt(intptr_t index, intptr_t size) { @@ -86,7 +86,7 @@ class SharedClassTable { // Ensure we never change size for a given cid from one non-zero size to // another non-zero size. intptr_t old_size = 0; - if (!table_[index].compare_exchange_strong(old_size, size)) { + if (!table_.load()[index].compare_exchange_strong(old_size, size)) { RELEASE_ASSERT(old_size == size); } } @@ -119,12 +119,12 @@ class SharedClassTable { void SetTraceAllocationFor(intptr_t cid, bool trace) { ASSERT(cid > 0); ASSERT(cid < top_); - trace_allocation_table_[cid] = trace ? 1 : 0; + trace_allocation_table_.load()[cid] = trace ? 1 : 0; } bool TraceAllocationFor(intptr_t cid) { ASSERT(cid > 0); ASSERT(cid < top_); - return trace_allocation_table_[cid] != 0; + return trace_allocation_table_.load()[cid] != 0; } #endif // !defined(PRODUCT) @@ -134,14 +134,14 @@ class SharedClassTable { const intptr_t num_cids = NumCids(); const intptr_t bytes = sizeof(intptr_t) * num_cids; auto size_table = static_cast(malloc(bytes)); - memmove(size_table, table_, sizeof(intptr_t) * num_cids); + memmove(size_table, table_.load(), sizeof(intptr_t) * num_cids); *copy_num_cids = num_cids; *copy = size_table; } void ResetBeforeHotReload() { // The [IsolateReloadContext] is now source-of-truth for GC. - memset(table_, 0, sizeof(intptr_t) * top_); + memset(table_.load(), 0, sizeof(intptr_t) * top_); } void ResetAfterHotReload(intptr_t* old_table, @@ -151,7 +151,7 @@ class SharedClassTable { // return, so we restore size information for all classes. if (is_rollback) { SetNumCids(num_old_cids); - memmove(table_, old_table, sizeof(intptr_t) * num_old_cids); + memmove(table_.load(), old_table, sizeof(intptr_t) * num_old_cids); } // Can't free this table immediately as another thread (e.g., concurrent @@ -207,7 +207,7 @@ class SharedClassTable { #ifndef PRODUCT // Copy-on-write is used for trace_allocation_table_, with old copies stored // in old_tables_. - AcqRelAtomic trace_allocation_table_ = nullptr; + AcqRelAtomic trace_allocation_table_ = {nullptr}; #endif // !PRODUCT void AddOldTable(intptr_t* old_table); @@ -219,7 +219,7 @@ class SharedClassTable { // Copy-on-write is used for table_, with old copies stored in old_tables_. // Maps the cid to the instance size. - AcqRelAtomic*> table_ = nullptr; + AcqRelAtomic*> table_ = {nullptr}; MallocGrowableArray* old_tables_; IsolateGroupReloadContext* reload_context_ = nullptr; @@ -247,7 +247,7 @@ class ClassTable { const intptr_t num_cids = NumCids(); const intptr_t bytes = sizeof(RawClass*) * num_cids; auto class_table = static_cast(malloc(bytes)); - memmove(class_table, table_, sizeof(RawClass*) * num_cids); + memmove(class_table, table_.load(), sizeof(RawClass*) * num_cids); *copy_num_cids = num_cids; *copy = class_table; } @@ -267,7 +267,7 @@ class ClassTable { // return, so we restore size information for all classes. if (is_rollback) { SetNumCids(num_old_cids); - memmove(table_, old_table, sizeof(RawClass*) * num_old_cids); + memmove(table_.load(), old_table, sizeof(RawClass*) * num_old_cids); } else { CopySizesFromClassObjects(); } @@ -282,7 +282,7 @@ class ClassTable { // Thread-safe. RawClass* At(intptr_t index) const { ASSERT(IsValidIndex(index)); - return table_[index]; + return table_.load()[index]; } intptr_t SizeAt(intptr_t index) const { @@ -297,7 +297,7 @@ class ClassTable { bool HasValidClassAt(intptr_t index) const { ASSERT(IsValidIndex(index)); - return table_[index] != nullptr; + return table_.load()[index] != nullptr; } intptr_t NumCids() const { return shared_class_table_->NumCids(); }