commit 7840675171e8dc2740312f0609518819ca9bf038
author Vyacheslav Egorov <vegorov@google.com> Tue Dec 18 16:46:03 2018 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org> Tue Dec 18 16:46:03 2018 +0000
tree 5f3b082a9fcf00c0d47d50bbb188afe625c6b4ef
parent 4163478f9b2ff47ed91d712eab42a373cfe624aa

[vm] Fix vm/cc/ICDataTest: use release-store/acquire-load for array of ICData objects.

Previous the test was accessing this array without any synchronization
meaning that stores of new ICData objects into this array could
be observed too early, before ICData object itself is initialized.

Also add release/acquire in the code that attaches ICData map to
the function - to ensure that if compiler thread sees ICData map
then it is guaranteed to be fully initialized.

Closes #34999

Change-Id: I95ba857122e2a5afed90b1518d591282e8255002
Reviewed-on: https://dart-review.googlesource.com/c/87583
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Auto-Submit: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>

runtime/vm/object.cc

diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index dae8d6b..fed6805 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -7964,11 +7964,12 @@
 }
 
 void Function::set_ic_data_array(const Array& value) const {
-  StorePointer(&raw_ptr()->ic_data_array_, value.raw());
+  StorePointer<RawArray*, MemoryOrder::kRelease>(&raw_ptr()->ic_data_array_,
+                                                 value.raw());
 }
 
 RawArray* Function::ic_data_array() const {
-  return raw_ptr()->ic_data_array_;
+  return AtomicOperations::LoadAcquire(&raw_ptr()->ic_data_array_);
 }
 
 void Function::ClearICDataArray() const {

runtime/vm/object.h

diff --git a/runtime/vm/object.h b/runtime/vm/object.h
index ee91913..ec52787 100644
--- a/runtime/vm/object.h
+++ b/runtime/vm/object.h
@@ -7853,6 +7853,16 @@
     StoreArrayPointer(ObjectAddr(index), value.raw());
   }
 
+  // Access to the array with acquire release semantics.
+  RawObject* AtAcquire(intptr_t index) const {
+    return AtomicOperations::LoadAcquire(ObjectAddr(index));
+  }
+  void SetAtRelease(intptr_t index, const Object& value) const {
+    // TODO(iposva): Add storing NoSafepointScope.
+    StoreArrayPointer<RawObject*, MemoryOrder::kRelease>(ObjectAddr(index),
+                                                         value.raw());
+  }
+
   bool IsImmutable() const { return raw()->GetClassId() == kImmutableArrayCid; }
 
   virtual RawTypeArguments* GetTypeArguments() const {
@@ -7953,9 +7963,9 @@
     StoreSmi(&raw_ptr()->length_, Smi::New(value));
   }
 
-  template <typename type>
+  template <typename type, MemoryOrder order = MemoryOrder::kRelaxed>
   void StoreArrayPointer(type const* addr, type value) const {
-    raw()->StoreArrayPointer(addr, value);
+    raw()->StoreArrayPointer<type, order>(addr, value);
   }
 
   // Store a range of pointers [from, from + count) into [to, to + count).

runtime/vm/raw_object.h

diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h
index f9e0a5c..98a4097 100644
--- a/runtime/vm/raw_object.h
+++ b/runtime/vm/raw_object.h
@@ -716,6 +716,7 @@
     if (order == MemoryOrder::kRelease) {
       AtomicOperations::StoreRelease(const_cast<type*>(addr), value);
     } else {
+      ASSERT(order == MemoryOrder::kRelaxed);
       *const_cast<type*>(addr) = value;
     }
     if (value->IsHeapObject()) {
@@ -761,9 +762,14 @@
     }
   }
 
-  template <typename type>
+  template <typename type, MemoryOrder order = MemoryOrder::kRelaxed>
   void StoreArrayPointer(type const* addr, type value) {
-    *const_cast<type*>(addr) = value;
+    if (order == MemoryOrder::kRelease) {
+      AtomicOperations::StoreRelease(const_cast<type*>(addr), value);
+    } else {
+      ASSERT(order == MemoryOrder::kRelaxed);
+      *const_cast<type*>(addr) = value;
+    }
     if (value->IsHeapObject()) {
       CheckArrayPointerStore(addr, value, Thread::Current());
     }

runtime/vm/thread_test.cc

diff --git a/runtime/vm/thread_test.cc b/runtime/vm/thread_test.cc
index 48df459..d71628e 100644
--- a/runtime/vm/thread_test.cc
+++ b/runtime/vm/thread_test.cc
@@ -419,7 +419,7 @@
       while (true) {
         for (intptr_t cnt = 0; cnt < 0x1000; cnt++) {
           for (intptr_t i = 0; i < len_; i++) {
-            ic_data ^= ic_datas_.At(i);
+            ic_data ^= ic_datas_.AtAcquire(i);
             arr = ic_data.ic_data();
             intptr_t num_checks = arr.Length() / 3;
             if (num_checks < 0 || num_checks > 5) {
@@ -492,7 +492,7 @@
     ic_data = ICData::New(owner, name, args_desc, /*deopt_id=*/0,
                           /*num_args_tested=*/1, ICData::kInstance,
                           Object::null_abstract_type());
-    ic_datas.SetAt(i, ic_data);
+    ic_datas.SetAtRelease(i, ic_data);
   }
 
   for (int i = 0; i < ICDataTestTask::kTaskCount; i++) {
@@ -510,7 +510,7 @@
         ic_data = ICData::New(owner, name, args_desc, /*deopt_id=*/0,
                               /*num_args_tested=*/1, ICData::kInstance,
                               Object::null_abstract_type());
-        ic_datas.SetAt(i, ic_data);
+        ic_datas.SetAtRelease(i, ic_data);
       }
     }
   }