- Use the simulator to do atomic operations that could also

runtime/vm/simulator_arm.cc

Index: runtime/vm/simulator_arm.cc
===================================================================
--- runtime/vm/simulator_arm.cc	(revision 41298)
+++ runtime/vm/simulator_arm.cc	(working copy)
@@ -678,55 +678,14 @@
 
 // Synchronization primitives support.
 Mutex* Simulator::exclusive_access_lock_ = NULL;
-Simulator::AddressTag Simulator::exclusive_access_state_[kNumAddressTags];
-int Simulator::next_address_tag_;
+Simulator::AddressTag Simulator::exclusive_access_state_[kNumAddressTags] =
+    {{NULL, 0}};
+int Simulator::next_address_tag_ = 0;
 
 
-void Simulator::SetExclusiveAccess(uword addr) {
-  Isolate* isolate = Isolate::Current();
-  ASSERT(isolate != NULL);
-  int i = 0;
-  while ((i < kNumAddressTags) &&
-         (exclusive_access_state_[i].isolate != isolate)) {
-    i++;
-  }
-  if (i == kNumAddressTags) {
-    i = next_address_tag_;
-    if (++next_address_tag_ == kNumAddressTags) next_address_tag_ = 0;
-    exclusive_access_state_[i].isolate = isolate;
-  }
-  exclusive_access_state_[i].addr = addr;
-}
-
-
-bool Simulator::HasExclusiveAccessAndOpen(uword addr) {
-  Isolate* isolate = Isolate::Current();
-  ASSERT(isolate != NULL);
-  bool result = false;
-  for (int i = 0; i < kNumAddressTags; i++) {
-    if (exclusive_access_state_[i].isolate == isolate) {
-      if (exclusive_access_state_[i].addr == addr) {
-        result = true;
-      }
-      exclusive_access_state_[i].addr = NULL;
-      continue;
-    }
-    if (exclusive_access_state_[i].addr == addr) {
-      exclusive_access_state_[i].addr = NULL;
-    }
-  }
-  return result;
-}
-
-
 void Simulator::InitOnce() {
-  // Setup exclusive access state.
+  // Setup exclusive access state lock.
   exclusive_access_lock_ = new Mutex();
-  for (int i = 0; i < kNumAddressTags; i++) {
-    exclusive_access_state_[i].isolate = NULL;
-    exclusive_access_state_[i].addr = NULL;
-  }
-  next_address_tag_ = 0;
 }
 
 
@@ -1112,16 +1071,57 @@
 
 
 // Synchronization primitives support.
+void Simulator::SetExclusiveAccess(uword addr) {
+  Isolate* isolate = Isolate::Current();
+  ASSERT(isolate != NULL);
+  int i = 0;
+  // Find an entry for this isolate in the exclusive access state.
+  while ((i < kNumAddressTags) &&
+         (exclusive_access_state_[i].isolate != isolate)) {
+    i++;
+  }
+  // Round-robin replacement of previously used entries.
+  if (i == kNumAddressTags) {
+    i = next_address_tag_;
+    if (++next_address_tag_ == kNumAddressTags) {
+      next_address_tag_ = 0;
+    }
+    exclusive_access_state_[i].isolate = isolate;

[zra, 2014/10/27 14:53:12]

Should we assert here that exclusive_access_state_[i].addr == 0? Or instead
search for an entry where that's the case?

[Ivan Posva, 2014/10/27 17:58:24]

You cannot assert this. As it is perfectly legal to:
ldrex Rt, some_addr
then branch out if Rt is not the expected value and then later
ldrex Rt, some_other_addr
At this point exclusive_access_state's addr for this isolate is not zero and
you'd get a wrong assertion failure.
The CPU the simulator is modeling has only one reservation slot to be very
conservative. Thus you need to invalidate the existing reservation if it exists
for this isolate.

[zra, 2014/10/27 18:09:17]

What happens when you run out of address tags?

+  }
+  // Remember the address being reserved.
+  exclusive_access_state_[i].addr = addr;
+}
+
+
+bool Simulator::HasExclusiveAccessAndOpen(uword addr) {
+  Isolate* isolate = Isolate::Current();
+  ASSERT(isolate != NULL);
+  ASSERT(addr != 0);
+  bool result = false;
+  for (int i = 0; i < kNumAddressTags; i++) {
+    if (exclusive_access_state_[i].isolate == isolate) {
+      // Check whether the current isolates address reservation matches.

[zra, 2014/10/27 14:53:12]

isolate's

[Ivan Posva, 2014/10/27 17:58:23]

Done.

+      if (exclusive_access_state_[i].addr == addr) {
+        result = true;
+      }
+      exclusive_access_state_[i].addr = 0;
+    } else if (exclusive_access_state_[i].addr == addr) {
+      // Other isolates with matching address lose their reservations.
+      exclusive_access_state_[i].addr = 0;
+    }
+  }
+  return result;
+}
+
+
 void Simulator::ClearExclusive() {
-  // This lock is initialized in Simulator::InitOnce().
   MutexLocker ml(exclusive_access_lock_);
-  // Set exclusive access to open state for this isolate.
-  HasExclusiveAccessAndOpen(NULL);
+  // Remove the reservation for this isolate.
+  SetExclusiveAccess(NULL);
 }
 
 
 intptr_t Simulator::ReadExclusiveW(uword addr, Instr* instr) {
-  // This lock is initialized in Simulator::InitOnce().
   MutexLocker ml(exclusive_access_lock_);
   SetExclusiveAccess(addr);
   return ReadW(addr, instr);
@@ -1129,7 +1129,6 @@
 
 
 intptr_t Simulator::WriteExclusiveW(uword addr, intptr_t value, Instr* instr) {
-  // This lock is initialized in Simulator::InitOnce().
   MutexLocker ml(exclusive_access_lock_);
   bool write_allowed = HasExclusiveAccessAndOpen(addr);
   if (write_allowed) {
@@ -1145,6 +1144,8 @@
                                  uword new_value) {
   // This lock is initialized in Simulator::InitOnce().

[koda, 2014/10/27 14:05:10]

Why keep this comment but remove the other two instances?

[Ivan Posva, 2014/10/27 17:58:24]

Done. Oversight, thanks for noticing.

   MutexLocker ml(exclusive_access_lock_);
+  // We do not check for a reservation as it would be guaranteed to be found. No

[koda, 2014/10/27 14:05:10]

Why not ASSERT this then?

[Ivan Posva, 2014/10/27 17:58:23]

Maybe the comment needs rephrasing: What it is supposed to convey that since we
hold the lock the load would get the reservation and nobody else can get it and
steal it while the lock is held.

[koda, 2014/10/27 18:32:48]

Then you should assert that the current isolate holds the lock when
SetExclusiveAccess/HasExclusiveAccessAndOpen are called, and document it in the
header.

[Ivan Posva, 2014/10/27 19:21:08]

Done.

+  // other isolate is able to make a reservation while we hold the lock.
   uword value = *address;
   if (value == compare_value) {
     *address = new_value;