Fix ABA bug with PreObjectAlloc event being missed

We prevent the PreObjectAlloc event from ever being disabled after
being activated for allocation pausing the first time. This is to
prevent an instance of the ABA problem where a thread can miss the
fact that the size of the object it's allocating has changed if it was
suspended within the allocation path during the redefinition.

We need to make sure that every thread gets a chance to see the
PreObjectAlloc event at least once or else it could miss the fact that
the object its allocating had its size changed.

Consider the following 2 threads. T1 is allocating an object of class
K. It is suspended (by user code) somewhere in the
AllocObjectWithAllocator function, perhaps while doing a GC to attempt
to clear space. With that thread suspended on thread T2 we decide to
structurally redefine 'K', changing its size. To do this we insert
this PreObjectAlloc event to check and update the size of the class
being allocated. This is done successfully. Now imagine if T2 removed
the listener event then T1 subsequently resumes. T1 would see there is
no PreObjectAlloc event and so allocate using the old object size.
This leads to it not allocating enough. To prevent this we simply
force every allocation after our first pause to go through the
PreObjectAlloc event.

Test: setup runit.sh to run test 2005.
      ./art/tools/parallel_run.py -j80 --out art/out.txt ./art/runit.sh
Test: ./test.py --host
Bug: 146436130

Change-Id: I7cdc1d133a7cbba7784b906b587e565072a453b2

1 files changed, 31 insertions, 3 deletions

diff --git a/openjdkjvmti/alloc_manager.cc b/openjdkjvmti/alloc_manager.cc
index 76605399b2..5910073d79 100644
--- a/openjdkjvmti/alloc_manager.cc
+++ b/openjdkjvmti/alloc_manager.cc
@@ -93,7 +93,7 @@ void JvmtiAllocationListener::ObjectAllocated(art::Thread* self,
 }
 
 bool JvmtiAllocationListener::HasPreAlloc() const {
-  return manager_->allocations_paused_thread_.load(std::memory_order_seq_cst) != nullptr;
+  return manager_->allocations_paused_ever_.load(std::memory_order_seq_cst);
 }
 
 void JvmtiAllocationListener::PreObjectAllocated(art::Thread* self,
@@ -162,7 +162,34 @@ void AllocationManager::IncrListenerInstall(art::Thread* self) {
 
 void AllocationManager::PauseAllocations(art::Thread* self) {
   art::Thread* null_thr = nullptr;
-  IncrListenerInstall(self);
+  // Unfortunately once we've paused allocations once we have to leave the listener and
+  // PreObjectAlloc event enabled forever. This is to avoid an instance of the ABA problem. We need
+  // to make sure that every thread gets a chance to see the PreObjectAlloc event at least once or
+  // else it could miss the fact that the object its allocating had its size changed.
+  //
+  // Consider the following 2 threads. T1 is allocating an object of class K. It is suspended (by
+  // user code) somewhere in the AllocObjectWithAllocator function, perhaps while doing a GC to
+  // attempt to clear space. With that thread suspended on thread T2 we decide to structurally
+  // redefine 'K', changing its size. To do this we insert this PreObjectAlloc event to check and
+  // update the size of the class being allocated. This is done successfully. Now imagine if T2
+  // removed the listener event then T1 subsequently resumes. T1 would see there is no
+  // PreObjectAlloc event and so allocate using the old object size. This leads to it not allocating
+  // enough. To prevent this we simply force every allocation after our first pause to go through
+  // the PreObjectAlloc event.
+  //
+  // TODO Technically we could do better than this. We just need to be able to require that all
+  // threads within allocation functions go through the PreObjectAlloc at least once after we turn
+  // it on. This is easier said than done though since we don't want to place a marker on threads
+  // (allocation is just too common) and we can't just have every thread go through the event since
+  // there are some threads that never or almost never allocate. We would also need to ensure that
+  // this thread doesn't pause waiting for all threads to pass the barrier since the other threads
+  // might be suspended. We could accomplish this by storing callbacks on each thread that would do
+  // the work. Honestly though this is a debug feature and it doesn't slow things down very much so
+  // simply leaving it on forever is simpler and safer.
+  bool expected = false;
+  if (allocations_paused_ever_.compare_exchange_strong(expected, true, std::memory_order_seq_cst)) {
+    IncrListenerInstall(self);
+  }
   do {
     PauseForAllocation(self, []() { return "request to pause allocations on other threads"; });
   } while (!allocations_paused_thread_.compare_exchange_strong(
@@ -180,7 +207,8 @@ void AllocationManager::PauseAllocations(art::Thread* self) {
 
 void AllocationManager::ResumeAllocations(art::Thread* self) {
   CHECK_EQ(allocations_paused_thread_.load(), self) << "not paused! ";
-  DecrListenerInstall(self);
+  // See above for why we don't decr the install count.
+  CHECK(allocations_paused_ever_.load(std::memory_order_seq_cst));
   art::ScopedThreadSuspension sts(self, art::ThreadState::kSuspended);
   art::MutexLock mu(self, alloc_listener_mutex_);
   allocations_paused_thread_.store(nullptr, std::memory_order_seq_cst);