Remove race on cached thread-name deletion

Manage the thread name using an RCU-like scheme, in which the name
is an atomic pointer to an immutable string, and readers explicitly
maintain a count of active readers. Writers wait until there are
no readers before deallocating a name that was just replaced.

Currently this potentially blocks writers, but readers should be very
fast, and relatively infrequent, so I can't imagine this is a real
issue.

This also replaces JVM_SetNativeThreadName with a stub. This part of
the CL came from Nicolas' aosp/1706702, which this is otherwise
intended to replace.

Test: Build and boot AOSP.
Bug: 37970289
Change-Id: I7e1d13d24adc10b4009d60ee68bc9e837ce78b34

4 files changed, 60 insertions, 46 deletions

diff --git a/compiler/utils/assembler_thumb_test_expected.cc.inc b/compiler/utils/assembler_thumb_test_expected.cc.inc
index 2d1de97a98..b6c6025e41 100644
--- a/compiler/utils/assembler_thumb_test_expected.cc.inc
+++ b/compiler/utils/assembler_thumb_test_expected.cc.inc
@@ -76,7 +76,7 @@ const char* const VixlJniHelpersResults = {
   "      f2: bb f1 00 0f   cmp.w r11, #0\n"
   "      f6: 18 bf         it ne\n"
   "      f8: e3 46         movne r11, r12\n"
-  "      fa: d9 f8 8c c0   ldr.w r12, [r9, #140]\n"
+  "      fa: d9 f8 94 c0   ldr.w r12, [r9, #148]\n"
   "      fe: bc f1 00 0f   cmp.w r12, #0\n"
   "     102: 71 d1         bne 0x1e8     @ imm = #226\n"
   "     104: cd f8 ff c7   str.w r12, [sp, #2047]\n"
@@ -152,8 +152,8 @@ const char* const VixlJniHelpersResults = {
   "     218: bd e8 e0 4d   pop.w {r5, r6, r7, r8, r10, r11, lr}\n"
   "     21c: d9 f8 24 80   ldr.w r8, [r9, #36]\n"
   "     220: 70 47         bx lr\n"
-  "     222: d9 f8 8c 00   ldr.w r0, [r9, #140]\n"
-  "     226: d9 f8 c0 e2   ldr.w lr, [r9, #704]\n"
+  "     222: d9 f8 94 00   ldr.w r0, [r9, #148]\n"
+  "     226: d9 f8 c8 e2   ldr.w lr, [r9, #712]\n"
   "     22a: f0 47         blx lr\n"
 };
 
diff --git a/openjdkjvm/OpenjdkJvm.cc b/openjdkjvm/OpenjdkJvm.cc
index a9fa0fc6c7..63503f459e 100644
--- a/openjdkjvm/OpenjdkJvm.cc
+++ b/openjdkjvm/OpenjdkJvm.cc
@@ -406,38 +406,12 @@ JNIEXPORT jboolean JVM_HoldsLock(JNIEnv* env, jclass unused ATTRIBUTE_UNUSED, jo
   return soa.Self()->HoldsLock(object);
 }
 
-JNIEXPORT void JVM_SetNativeThreadName(JNIEnv* env, jobject jthread, jstring java_name) {
-  ScopedUtfChars name(env, java_name);
-  {
-    art::ScopedObjectAccess soa(env);
-    if (soa.Decode<art::mirror::Object>(jthread) == soa.Self()->GetPeer()) {
-      soa.Self()->SetThreadName(name.c_str());
-      return;
-    }
-  }
-  // Suspend thread to avoid it from killing itself while we set its name. We don't just hold the
-  // thread list lock to avoid this, as setting the thread name causes mutator to lock/unlock
-  // in the DDMS send code.
-  art::ThreadList* thread_list = art::Runtime::Current()->GetThreadList();
-  bool timed_out;
-  // Take suspend thread lock to avoid races with threads trying to suspend this one.
-  art::Thread* thread;
-  {
-    thread = thread_list->SuspendThreadByPeer(jthread,
-                                              art::SuspendReason::kInternal,
-                                              &timed_out);
-  }
-  if (thread != nullptr) {
-    {
-      art::ScopedObjectAccess soa(env);
-      thread->SetThreadName(name.c_str());
-    }
-    bool resumed = thread_list->Resume(thread, art::SuspendReason::kInternal);
-    DCHECK(resumed);
-  } else if (timed_out) {
-    LOG(ERROR) << "Trying to set thread name to '" << name.c_str() << "' failed as the thread "
-        "failed to suspend within a generous timeout.";
-  }
+JNIEXPORT __attribute__((noreturn)) void JVM_SetNativeThreadName(
+    JNIEnv* env ATTRIBUTE_UNUSED,
+    jobject jthread ATTRIBUTE_UNUSED,
+    jstring java_name ATTRIBUTE_UNUSED) {
+  UNIMPLEMENTED(FATAL) << "JVM_SetNativeThreadName is not implemented";
+  UNREACHABLE();
 }
 
 JNIEXPORT __attribute__((noreturn)) jint JVM_IHashCode(JNIEnv* env ATTRIBUTE_UNUSED,
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 8b2a4117e9..0a3c09adf3 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -19,6 +19,7 @@
 #include <limits.h>  // for INT_MAX
 #include <pthread.h>
 #include <signal.h>
+#include <stdlib.h>
 #include <sys/resource.h>
 #include <sys/time.h>
 
@@ -29,6 +30,7 @@
 #endif
 
 #include <algorithm>
+#include <atomic>
 #include <bitset>
 #include <cerrno>
 #include <iostream>
@@ -1066,7 +1068,7 @@ Thread* Thread::Attach(const char* thread_name,
     } else {
       // These aren't necessary, but they improve diagnostics for unit tests & command-line tools.
       if (thread_name != nullptr) {
-        self->tlsPtr_.name->assign(thread_name);
+        self->SetCachedThreadName(thread_name);
         ::art::SetThreadName(thread_name);
       } else if (self->GetJniEnv()->IsCheckJniEnabled()) {
         LOG(WARNING) << *Thread::Current() << " attached without supplying a name";
@@ -1232,8 +1234,27 @@ void Thread::InitPeer(ScopedObjectAccessAlreadyRunnable& soa,
       SetInt<kTransactionActive>(peer, thread_priority);
 }
 
+void Thread::SetCachedThreadName(const char* name) {
+  DCHECK(name != kThreadNameDuringStartup);
+  const char* old_name = tlsPtr_.name.exchange(name == nullptr ? nullptr : strdup(name));
+  if (old_name != nullptr && old_name !=  kThreadNameDuringStartup) {
+    // Deallocate it, carefully. Note that the load has to be ordered wrt the store of the xchg.
+    for (uint32_t i = 0; UNLIKELY(tls32_.num_name_readers.load(std::memory_order_seq_cst) != 0);
+         ++i) {
+      static constexpr uint32_t kNumSpins = 1000;
+      // Ugly, but keeps us from having to do anything on the reader side.
+      if (i > kNumSpins) {
+        usleep(500);
+      }
+    }
+    // We saw the reader count drop to zero since we replaced the name; old one is now safe to
+    // deallocate.
+    free(const_cast<char *>(old_name));
+  }
+}
+
 void Thread::SetThreadName(const char* name) {
-  tlsPtr_.name->assign(name);
+  SetCachedThreadName(name);
   ::art::SetThreadName(name);
   Dbg::DdmSendThreadNotification(this, CHUNK_TYPE("THNM"));
 }
@@ -1359,11 +1380,14 @@ void Thread::ShortDump(std::ostream& os) const {
     os << GetThreadId()
        << ",tid=" << GetTid() << ',';
   }
+  tls32_.num_name_readers.fetch_add(1, std::memory_order_seq_cst);
+  const char* name = tlsPtr_.name.load();
   os << GetState()
      << ",Thread*=" << this
      << ",peer=" << tlsPtr_.opeer
-     << ",\"" << (tlsPtr_.name != nullptr ? *tlsPtr_.name : "null") << "\""
+     << ",\"" << (name == nullptr ? "null" : name) << "\""
      << "]";
+  tls32_.num_name_readers.fetch_sub(1 /* at least memory_order_release */);
 }
 
 void Thread::Dump(std::ostream& os, bool dump_native_stack, BacktraceMap* backtrace_map,
@@ -1382,7 +1406,10 @@ ObjPtr<mirror::String> Thread::GetThreadName() const {
 }
 
 void Thread::GetThreadName(std::string& name) const {
-  name.assign(*tlsPtr_.name);
+  tls32_.num_name_readers.fetch_add(1, std::memory_order_seq_cst);
+  // The store part of the increment has to be ordered with respect to the following load.
+  name.assign(tlsPtr_.name.load(std::memory_order_seq_cst));
+  tls32_.num_name_readers.fetch_sub(1 /* at least memory_order_release */);
 }
 
 uint64_t Thread::GetCpuMicroTime() const {
@@ -1941,7 +1968,9 @@ void Thread::DumpState(std::ostream& os, const Thread* thread, pid_t tid) {
   }
 
   if (thread != nullptr) {
-    os << '"' << *thread->tlsPtr_.name << '"';
+    thread->tls32_.num_name_readers.fetch_add(1, std::memory_order_seq_cst);
+    os << '"' << thread->tlsPtr_.name.load() << '"';
+    thread->tls32_.num_name_readers.fetch_sub(1 /* at least memory_order_release */);
     if (is_daemon) {
       os << " daemon";
     }
@@ -2382,7 +2411,7 @@ Thread::Thread(bool daemon)
   DCHECK(tlsPtr_.mutator_lock != nullptr);
   tlsPtr_.instrumentation_stack =
       new std::map<uintptr_t, instrumentation::InstrumentationStackFrame>;
-  tlsPtr_.name = new std::string(kThreadNameDuringStartup);
+  tlsPtr_.name.store(kThreadNameDuringStartup, std::memory_order_relaxed);
 
   static_assert((sizeof(Thread) % 4) == 0U,
                 "art::Thread has a size which is not a multiple of 4.");
@@ -2419,7 +2448,7 @@ bool Thread::IsStillStarting() const {
   // It turns out that the last thing to change is the thread name; that's a good proxy for "has
   // this thread _ever_ entered kRunnable".
   return (tlsPtr_.jpeer == nullptr && tlsPtr_.opeer == nullptr) ||
-      (*tlsPtr_.name == kThreadNameDuringStartup);
+      (tlsPtr_.name.load() == kThreadNameDuringStartup);
 }
 
 void Thread::AssertPendingException() const {
@@ -2572,7 +2601,7 @@ Thread::~Thread() {
   }
 
   delete tlsPtr_.instrumentation_stack;
-  delete tlsPtr_.name;
+  SetCachedThreadName(nullptr);  // Deallocate name.
   delete tlsPtr_.deps_or_stack_trace_sample.stack_trace_sample;
 
   Runtime::Current()->GetHeap()->AssertThreadLocalBuffersAreRevoked(this);
diff --git a/runtime/thread.h b/runtime/thread.h
index 7eb57e914c..b64de25dde 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -1545,6 +1545,8 @@ class Thread {
 
   void ReleaseLongJumpContextInternal();
 
+  void SetCachedThreadName(const char* name);
+
   // Helper class for manipulating the 32 bits of atomically changed state and flags.
   class StateAndFlags {
    public:
@@ -1690,7 +1692,8 @@ class Thread {
           user_code_suspend_count(0),
           force_interpreter_count(0),
           make_visibly_initialized_counter(0),
-          define_class_counter(0) {}
+          define_class_counter(0),
+          num_name_readers(0) {}
 
     // The state and flags field must be changed atomically so that flag values aren't lost.
     // See `StateAndFlags` for bit assignments of `ThreadFlag` and `ThreadState` values.
@@ -1779,6 +1782,11 @@ class Thread {
     // Counter for how many nested define-classes are ongoing in this thread. Used to allow waiting
     // for threads to be done with class-definition work.
     uint32_t define_class_counter;
+
+    // A count of the number of readers of tlsPtr_.name that may still be looking at a string they
+    // retrieved.
+    mutable std::atomic<uint32_t> num_name_readers;
+    static_assert(std::atomic<uint32_t>::is_always_lock_free);
   } tls32_;
 
   struct PACKED(8) tls_64bit_sized_values {
@@ -1924,8 +1932,11 @@ class Thread {
     // set local values there first.
     FrameIdToShadowFrame* frame_id_to_shadow_frame;
 
-    // A cached copy of the java.lang.Thread's name.
-    std::string* name;
+    // A cached copy of the java.lang.Thread's (modified UTF-8) name.
+    // If this is not null or kThreadNameDuringStartup, then it owns the malloc memory holding
+    // the string. Updated in an RCU-like manner.
+    std::atomic<const char*> name;
+    static_assert(std::atomic<const char*>::is_always_lock_free);
 
     // A cached pthread_t for the pthread underlying this Thread*.
     pthread_t pthread_self;