commit 0f5f6bbc3a0bb125875f28ad61584001989a7f10
author Dave Allison <dallison@google.com> Fri Nov 22 17:39:19 2013 -0800
committer Dave Allison <dallison@google.com> Thu Dec 19 23:15:48 2013 +0000
tree e3668ea3ff2a004780fce093d941708cf495615b
parent e40687d053b89c495b6fbeb7a766b01c9c7e039c

Fix thread checkpoint issue

This changes a do...while loop into a while(true) with
a break out.  Fixes issue where checkpoint functions can
cause the thread state to become overwritten by a garbage
value.

Bug: 11809176
Change-Id: Ifbbc5e5cee85649d57f71267933539eefa7e25ea

runtime/thread-inl.h

diff --git a/runtime/thread-inl.h b/runtime/thread-inl.h
index 6f3c117..b87a8ec 100644
--- a/runtime/thread-inl.h
+++ b/runtime/thread-inl.h
@@ -87,18 +87,22 @@
   DCHECK_EQ(GetState(), kRunnable);
   union StateAndFlags old_state_and_flags;
   union StateAndFlags new_state_and_flags;
-  do {
+  while (true) {
     old_state_and_flags.as_int = state_and_flags_.as_int;
     if (UNLIKELY((old_state_and_flags.as_struct.flags & kCheckpointRequest) != 0)) {
       RunCheckpointFunction();
       continue;
     }
-    // Copy over flags and try to clear the checkpoint bit if it is set.
-    new_state_and_flags.as_struct.flags = old_state_and_flags.as_struct.flags & ~kCheckpointRequest;
+    // Change the state but keep the current flags (kCheckpointRequest is clear).
+    DCHECK_EQ((old_state_and_flags.as_struct.flags & kCheckpointRequest), 0);
+    new_state_and_flags.as_struct.flags = old_state_and_flags.as_struct.flags;
     new_state_and_flags.as_struct.state = new_state;
-    // CAS the value without a memory barrier, that will occur in the unlock below.
-  } while (UNLIKELY(android_atomic_cas(old_state_and_flags.as_int, new_state_and_flags.as_int,
-                                       &state_and_flags_.as_int) != 0));
+    int status = android_atomic_cas(old_state_and_flags.as_int, new_state_and_flags.as_int,
+                                       &state_and_flags_.as_int);
+    if (LIKELY(status == 0)) {
+      break;
+    }
+  }
   // Release share on mutator_lock_.
   Locks::mutator_lock_->SharedUnlock(this);
 }