[vm] Use atomics when accessing patchable object pool entries

Although the data races on patchable object pool entries are benign,
without proper release-acquire ordering there is no happens-before
relationship between store and load and there is a data race as defined
by the C++ standard (and detected by TSAN), which is an undefined
behavior.

It seems like additional memory_order_acquire loads only happen in
runtime entries when we need to patch calls, which is rather
infrequent. Unless additional barriers measurably affect
performance, we should prefer to avoid UB.

TEST=ci
Fixes https://github.com/dart-lang/sdk/issues/62236

Change-Id: I3a4d8c3dca7ce1ee1a75efd1ea8ba4e9c06c9dd6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/476500
Reviewed-by: Alexander Aprelev <aam@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>

runtime/vm/code_patcher_arm64.cc

@@ -25,7 +25,8 @@ class PoolPointerCall : public ValueObject {
   intptr_t pp_index() const { return index_; }
 
   CodePtr Target() const {
-    return static_cast<CodePtr>(object_pool_.ObjectAt(pp_index()));
+    return static_cast<CodePtr>(
+        object_pool_.ObjectAt<std::memory_order_acquire>(pp_index()));
   }
 
   void SetTarget(const Code& target) const {

runtime/vm/code_patcher_riscv.cc

@@ -35,7 +35,8 @@ class PoolPointerCall : public ValueObject {
   intptr_t pp_index() const { return index_; }
 
   CodePtr Target() const {
-    return static_cast<CodePtr>(object_pool_.ObjectAt(pp_index()));
+    return static_cast<CodePtr>(
+        object_pool_.ObjectAt<std::memory_order_acquire>(pp_index()));
   }
 
   void SetTarget(const Code& target) const {

runtime/vm/code_patcher_x64.cc

@@ -138,14 +138,16 @@ class UnoptimizedCall : public ValueObject {
     MatchCallPattern(&pc);
     MatchDataLoadFromPool(&pc, &argument_index_);
     MatchCodeLoadFromPool(&pc, &code_index_);
-    ASSERT(Object::Handle(object_pool_.ObjectAt(code_index_)).IsCode());
+    ASSERT(Object::Handle(
+               object_pool_.ObjectAt<std::memory_order_acquire>(code_index_))
+               .IsCode());
   }
 
   intptr_t argument_index() const { return argument_index_; }
 
   CodePtr target() const {
     Code& code = Code::Handle();
-    code ^= object_pool_.ObjectAt(code_index_);
+    code ^= object_pool_.ObjectAt<std::memory_order_acquire>(code_index_);
     return code.ptr();
   }
 
@@ -173,7 +175,9 @@ class NativeCall : public ValueObject {
     MatchCallPattern(&pc);
     MatchCodeLoadFromPool(&pc, &code_index_);
     MatchDataLoadFromPool(&pc, &argument_index_);
-    ASSERT(Object::Handle(object_pool_.ObjectAt(code_index_)).IsCode());
+    ASSERT(Object::Handle(
+               object_pool_.ObjectAt<std::memory_order_acquire>(code_index_))
+               .IsCode());
   }
 
   intptr_t argument_index() const { return argument_index_; }
@@ -190,7 +194,7 @@ class NativeCall : public ValueObject {
 
   CodePtr target() const {
     Code& code = Code::Handle();
-    code ^= object_pool_.ObjectAt(code_index_);
+    code ^= object_pool_.ObjectAt<std::memory_order_acquire>(code_index_);
     return code.ptr();
   }
 
@@ -261,12 +265,14 @@ class PoolPointerCall : public ValueObject {
 
     MatchCallPattern(&pc);
     MatchCodeLoadFromPool(&pc, &code_index_);
-    ASSERT(Object::Handle(object_pool_.ObjectAt(code_index_)).IsCode());
+    ASSERT(Object::Handle(
+               object_pool_.ObjectAt<std::memory_order_acquire>(code_index_))
+               .IsCode());
   }
 
   CodePtr Target() const {
     Code& code = Code::Handle();
-    code ^= object_pool_.ObjectAt(code_index_);
+    code ^= object_pool_.ObjectAt<std::memory_order_acquire>(code_index_);
     return code.ptr();
   }
 
@@ -301,7 +307,9 @@ class SwitchableCallBase : public ValueObject {
   }
 
   void SetDataRelease(const Object& data) const {
-    ASSERT(!Object::Handle(object_pool_.ObjectAt(data_index())).IsCode());
+    ASSERT(!Object::Handle(
+                object_pool_.ObjectAt<std::memory_order_relaxed>(data_index()))
+                .IsCode());
     object_pool_.SetObjectAt<std::memory_order_release>(data_index(), data);
     // No need to flush the instruction cache, since the code is not modified.
   }
@@ -359,16 +367,22 @@ class SwitchableCall : public SwitchableCallBase {
     } else {
       FATAL("Failed to decode at %" Px, pc);
     }
-    ASSERT(Object::Handle(object_pool_.ObjectAt(target_index_)).IsCode());
+    ASSERT(Object::Handle(
+               object_pool_.ObjectAt<std::memory_order_relaxed>(target_index_))
+               .IsCode());
   }
 
   void SetTargetRelease(const Code& target) const {
-    ASSERT(Object::Handle(object_pool_.ObjectAt(target_index())).IsCode());
+    ASSERT(Object::Handle(
+               object_pool_.ObjectAt<std::memory_order_relaxed>(target_index()))
+               .IsCode());
     object_pool_.SetObjectAt<std::memory_order_release>(target_index(), target);
     // No need to flush the instruction cache, since the code is not modified.
   }
 
-  ObjectPtr target() const { return object_pool_.ObjectAt(target_index()); }
+  ObjectPtr target() const {
+    return object_pool_.ObjectAt<std::memory_order_acquire>(target_index());
+  }
 };
 
 // See [SwitchableCallBase] for a switchable calls in general.
@@ -413,7 +427,9 @@ class BareSwitchableCall : public SwitchableCallBase {
     } else {
       FATAL("Failed to decode at %" Px, pc);
     }
-    ASSERT(!Object::Handle(object_pool_.ObjectAt(data_index_)).IsCode());
+    ASSERT(!Object::Handle(
+                object_pool_.ObjectAt<std::memory_order_relaxed>(data_index_))
+                .IsCode());
 
     // movq RCX, [PP + offset]
     static int16_t load_code_disp8[] = {

runtime/vm/instructions_arm.cc

@@ -71,7 +71,8 @@ NativeCallPattern::NativeCallPattern(uword pc, const Code& code)
 }
 
 CodePtr NativeCallPattern::target() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_code_pool_index_));
+  return static_cast<CodePtr>(object_pool_.ObjectAt<std::memory_order_acquire>(
+      target_code_pool_index_));
 }
 
 void NativeCallPattern::set_target(const Code& new_target) const {
@@ -213,7 +214,8 @@ bool DecodeLoadObjectFromPoolOrThread(uword pc, const Code& code, Object* obj) {
 }
 
 CodePtr CallPattern::TargetCode() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_code_pool_index_));
+  return static_cast<CodePtr>(object_pool_.ObjectAt<std::memory_order_acquire>(
+      target_code_pool_index_));
 }
 
 void CallPattern::SetTargetCode(const Code& target_code) const {
@@ -231,7 +233,8 @@ void ICCallPattern::SetData(const Object& data) const {
 }
 
 CodePtr ICCallPattern::TargetCode() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_pool_index_));
+  return static_cast<CodePtr>(
+      object_pool_.ObjectAt<std::memory_order_acquire>(target_pool_index_));
 }
 
 void ICCallPattern::SetTargetCode(const Code& target_code) const {
@@ -248,7 +251,9 @@ ObjectPtr SwitchableCallPatternBase::data() const {
 }
 
 void SwitchableCallPatternBase::SetDataRelease(const Object& data) const {
-  ASSERT(!Object::Handle(object_pool_.ObjectAt(data_pool_index_)).IsCode());
+  ASSERT(!Object::Handle(object_pool_.ObjectAt<std::memory_order_relaxed>(
+                             data_pool_index_))
+              .IsCode());
   object_pool_.SetObjectAt<std::memory_order_release>(data_pool_index_, data);
 }
 
@@ -268,11 +273,13 @@ SwitchableCallPattern::SwitchableCallPattern(uword pc, const Code& code)
 }
 
 ObjectPtr SwitchableCallPattern::target() const {
-  return object_pool_.ObjectAt(target_pool_index_);
+  return object_pool_.ObjectAt<std::memory_order_acquire>(target_pool_index_);
 }
 
 void SwitchableCallPattern::SetTargetRelease(const Code& target) const {
-  ASSERT(Object::Handle(object_pool_.ObjectAt(target_pool_index_)).IsCode());
+  ASSERT(Object::Handle(object_pool_.ObjectAt<std::memory_order_relaxed>(
+                            target_pool_index_))
+             .IsCode());
   object_pool_.SetObjectAt<std::memory_order_release>(target_pool_index_,
                                                       target);
 }

runtime/vm/instructions_arm64.cc

@@ -72,7 +72,8 @@ NativeCallPattern::NativeCallPattern(uword pc, const Code& code)
 }
 
 CodePtr NativeCallPattern::target() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_code_pool_index_));
+  return static_cast<CodePtr>(object_pool_.ObjectAt<std::memory_order_acquire>(
+      target_code_pool_index_));
 }
 
 void NativeCallPattern::set_target(const Code& target) const {
@@ -395,7 +396,8 @@ void InstructionPattern::EncodeLoadWordFromPoolFixed(uword end,
 }
 
 CodePtr CallPattern::TargetCode() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_code_pool_index_));
+  return static_cast<CodePtr>(object_pool_.ObjectAt<std::memory_order_acquire>(
+      target_code_pool_index_));
 }
 
 void CallPattern::SetTargetCode(const Code& target) const {
@@ -414,7 +416,8 @@ void ICCallPattern::SetData(const Object& data) const {
 }
 
 CodePtr ICCallPattern::TargetCode() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_pool_index_));
+  return static_cast<CodePtr>(
+      object_pool_.ObjectAt<std::memory_order_acquire>(target_pool_index_));
 }
 
 void ICCallPattern::SetTargetCode(const Code& target) const {
@@ -432,7 +435,9 @@ ObjectPtr SwitchableCallPatternBase::data() const {
 }
 
 void SwitchableCallPatternBase::SetDataRelease(const Object& data) const {
-  ASSERT(!Object::Handle(object_pool_.ObjectAt(data_pool_index_)).IsCode());
+  ASSERT(!Object::Handle(object_pool_.ObjectAt<std::memory_order_relaxed>(
+                             data_pool_index_))
+              .IsCode());
   object_pool_.SetObjectAt<std::memory_order_release>(data_pool_index_, data);
 }
 
@@ -454,11 +459,13 @@ SwitchableCallPattern::SwitchableCallPattern(uword pc, const Code& code)
 }
 
 ObjectPtr SwitchableCallPattern::target() const {
-  return object_pool_.ObjectAt(target_pool_index_);
+  return object_pool_.ObjectAt<std::memory_order_acquire>(target_pool_index_);
 }
 
 void SwitchableCallPattern::SetTargetRelease(const Code& target) const {
-  ASSERT(Object::Handle(object_pool_.ObjectAt(target_pool_index_)).IsCode());
+  ASSERT(Object::Handle(object_pool_.ObjectAt<std::memory_order_relaxed>(
+                            target_pool_index_))
+             .IsCode());
   object_pool_.SetObjectAt<std::memory_order_release>(target_pool_index_,
                                                       target);
 }

runtime/vm/instructions_riscv.cc

@@ -86,7 +86,8 @@ NativeCallPattern::NativeCallPattern(uword pc, const Code& code)
 }
 
 CodePtr NativeCallPattern::target() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_code_pool_index_));
+  return static_cast<CodePtr>(object_pool_.ObjectAt<std::memory_order_acquire>(
+      target_code_pool_index_));
 }
 
 void NativeCallPattern::set_target(const Code& target) const {
@@ -294,7 +295,8 @@ void InstructionPattern::EncodeLoadWordFromPoolFixed(uword end,
 }
 
 CodePtr CallPattern::TargetCode() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_code_pool_index_));
+  return static_cast<CodePtr>(object_pool_.ObjectAt<std::memory_order_acquire>(
+      target_code_pool_index_));
 }
 
 void CallPattern::SetTargetCode(const Code& target) const {
@@ -313,7 +315,8 @@ void ICCallPattern::SetData(const Object& data) const {
 }
 
 CodePtr ICCallPattern::TargetCode() const {
-  return static_cast<CodePtr>(object_pool_.ObjectAt(target_pool_index_));
+  return static_cast<CodePtr>(
+      object_pool_.ObjectAt<std::memory_order_acquire>(target_pool_index_));
 }
 
 void ICCallPattern::SetTargetCode(const Code& target) const {
@@ -331,7 +334,9 @@ ObjectPtr SwitchableCallPatternBase::data() const {
 }
 
 void SwitchableCallPatternBase::SetDataRelease(const Object& data) const {
-  ASSERT(!Object::Handle(object_pool_.ObjectAt(data_pool_index_)).IsCode());
+  ASSERT(!Object::Handle(object_pool_.ObjectAt<std::memory_order_relaxed>(
+                             data_pool_index_))
+              .IsCode());
   object_pool_.SetObjectAt<std::memory_order_release>(data_pool_index_, data);
 }
 
@@ -357,11 +362,13 @@ SwitchableCallPattern::SwitchableCallPattern(uword pc, const Code& code)
 }
 
 ObjectPtr SwitchableCallPattern::target() const {
-  return object_pool_.ObjectAt(target_pool_index_);
+  return object_pool_.ObjectAt<std::memory_order_acquire>(target_pool_index_);
 }
 
 void SwitchableCallPattern::SetTargetRelease(const Code& target) const {
-  ASSERT(Object::Handle(object_pool_.ObjectAt(target_pool_index_)).IsCode());
+  ASSERT(Object::Handle(object_pool_.ObjectAt<std::memory_order_relaxed>(
+                            target_pool_index_))
+             .IsCode());
   object_pool_.SetObjectAt<std::memory_order_release>(target_pool_index_,
                                                       target);
 }