commit 9ef78b59da51080882e47505896b420977fd79ae
author Mathieu Chartier <mathieuc@google.com> Thu Sep 25 17:03:12 2014 -0700
committer Mathieu Chartier <mathieuc@google.com> Fri Sep 26 16:04:56 2014 -0700
tree 6c568756e4e16e68d5d3346261009350969d5b77
parent 95f03e6a4737f90685fab86e98709f1c4393d5ef

Fix broken runtime SetStatsEnabled logic

Previously, Runtime::SetStatsEnabled wouldn't take stats_enabled_
into account when deciding whether or not to increment / decrement
teh stats enabled counter. This resulted in counter underflows and
other errors which caused some CTS tests to fail.

Also added some locking to prevent race conditions.

Bug: 17360878

(cherry picked from commit a98ffd745bbecb2e84a492194950c0b94966546b)

Change-Id: I21d241a58d35bd6a607aa2305c6da81720bd0886

runtime/instrumentation.cc

diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index a2e88a6..15be6b7 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -597,45 +597,52 @@
   thread->ResetQuickAllocEntryPointsForThread();
 }
 
-void Instrumentation::SetEntrypointsInstrumented(bool instrumented, bool suspended) {
+void Instrumentation::SetEntrypointsInstrumented(bool instrumented) {
+  Thread* self = Thread::Current();
   Runtime* runtime = Runtime::Current();
   ThreadList* tl = runtime->GetThreadList();
-  if (suspended) {
-    Locks::mutator_lock_->AssertExclusiveHeld(Thread::Current());
-  }
-  if (runtime->IsStarted() && !suspended) {
+  Locks::mutator_lock_->AssertNotHeld(self);
+  Locks::instrument_entrypoints_lock_->AssertHeld(self);
+  if (runtime->IsStarted()) {
     tl->SuspendAll();
   }
   {
-    MutexLock mu(Thread::Current(), *Locks::runtime_shutdown_lock_);
+    MutexLock mu(self, *Locks::runtime_shutdown_lock_);
     SetQuickAllocEntryPointsInstrumented(instrumented);
     ResetQuickAllocEntryPoints();
   }
-  if (runtime->IsStarted() && !suspended) {
+  if (runtime->IsStarted()) {
     tl->ResumeAll();
   }
 }
 
-void Instrumentation::InstrumentQuickAllocEntryPoints(bool suspended) {
-  // TODO: the read of quick_alloc_entry_points_instrumentation_counter_ is racey and this code
-  //       should be guarded by a lock.
-  DCHECK_GE(quick_alloc_entry_points_instrumentation_counter_.LoadSequentiallyConsistent(), 0);
-  const bool enable_instrumentation =
-      quick_alloc_entry_points_instrumentation_counter_.FetchAndAddSequentiallyConsistent(1) == 0;
-  if (enable_instrumentation) {
-    SetEntrypointsInstrumented(true, suspended);
-  }
+void Instrumentation::InstrumentQuickAllocEntryPoints() {
+  MutexLock mu(Thread::Current(), *Locks::instrument_entrypoints_lock_);
+  InstrumentQuickAllocEntryPointsLocked();
 }
 
-void Instrumentation::UninstrumentQuickAllocEntryPoints(bool suspended) {
-  // TODO: the read of quick_alloc_entry_points_instrumentation_counter_ is racey and this code
-  //       should be guarded by a lock.
-  DCHECK_GT(quick_alloc_entry_points_instrumentation_counter_.LoadSequentiallyConsistent(), 0);
-  const bool disable_instrumentation =
-      quick_alloc_entry_points_instrumentation_counter_.FetchAndSubSequentiallyConsistent(1) == 1;
-  if (disable_instrumentation) {
-    SetEntrypointsInstrumented(false, suspended);
+void Instrumentation::UninstrumentQuickAllocEntryPoints() {
+  MutexLock mu(Thread::Current(), *Locks::instrument_entrypoints_lock_);
+  UninstrumentQuickAllocEntryPointsLocked();
+}
+
+void Instrumentation::InstrumentQuickAllocEntryPointsLocked() {
+  Locks::instrument_entrypoints_lock_->AssertHeld(Thread::Current());
+  if (quick_alloc_entry_points_instrumentation_counter_ == 0) {
+    SetEntrypointsInstrumented(true);
   }
+  ++quick_alloc_entry_points_instrumentation_counter_;
+  LOG(INFO) << "Counter: " << quick_alloc_entry_points_instrumentation_counter_;
+}
+
+void Instrumentation::UninstrumentQuickAllocEntryPointsLocked() {
+  Locks::instrument_entrypoints_lock_->AssertHeld(Thread::Current());
+  CHECK_GT(quick_alloc_entry_points_instrumentation_counter_, 0U);
+  --quick_alloc_entry_points_instrumentation_counter_;
+  if (quick_alloc_entry_points_instrumentation_counter_ == 0) {
+    SetEntrypointsInstrumented(false);
+  }
+  LOG(INFO) << "Counter: " << quick_alloc_entry_points_instrumentation_counter_;
 }
 
 void Instrumentation::ResetQuickAllocEntryPoints() {