Add 2275-pthread-name test for Thread.setName

This checks that we set the pthread name correctly in response to
Thread.setName() calls.

We did not. In fact, we incorrectly set the pthread name for the calling
thread, rather than the `this` thread.

Fix that, by never erroneously setting the calling threads name, and by
making a good effort to set the correct one. This difers from the
documented RI behavior of setting the pthread name only if this == self.

That decision is not a slam dunk either way, and opinions are welcome.
This approach almost certainly matches naive expectations more of the
time, but it currently has the disadvantage that it can fail under much
less common, but also much less predictable, circumstances.

It could possibly be made fully predictable if we managed to clean
up some other issues, which I highlighted in the code.

Test: Host run tests, Treehugger
Change-Id: I295e0af0157a530919df4120c5dbf347dd9416a2

10 files changed, 162 insertions, 7 deletions

diff --git a/libartbase/base/utils.cc b/libartbase/base/utils.cc
index 0ec262e696..3cc5924593 100644
--- a/libartbase/base/utils.cc
+++ b/libartbase/base/utils.cc
@@ -283,7 +283,7 @@ template void Split(const std::string_view& s,
                     size_t len,
                     std::string_view* out_result);
 
-void SetThreadName(const char* thread_name) {
+void SetThreadName(pthread_t thr, const char* thread_name) {
   bool hasAt = false;
   bool hasDot = false;
   const char* s = thread_name;
@@ -306,15 +306,21 @@ void SetThreadName(const char* thread_name) {
   char buf[16];       // MAX_TASK_COMM_LEN=16 is hard-coded in the kernel.
   strncpy(buf, s, sizeof(buf)-1);
   buf[sizeof(buf)-1] = '\0';
-  errno = pthread_setname_np(pthread_self(), buf);
+  errno = pthread_setname_np(thr, buf);
   if (errno != 0) {
     PLOG(WARNING) << "Unable to set the name of current thread to '" << buf << "'";
   }
 #else  // __APPLE__
-  pthread_setname_np(thread_name);
+  if (pthread_equal(thr, pthread_self())) {
+    pthread_setname_np(thread_name);
+  } else {
+    PLOG(WARNING) << "Unable to set the name of another thread to '" << thread_name << "'";
+  }
 #endif
 }
 
+void SetThreadName(const char* thread_name) { SetThreadName(pthread_self(), thread_name); }
+
 void GetTaskStats(pid_t tid, char* state, int* utime, int* stime, int* task_cpu) {
   *utime = *stime = *task_cpu = 0;
 #ifdef _WIN32
diff --git a/libartbase/base/utils.h b/libartbase/base/utils.h
index b5e03dcdb8..32f8b87204 100644
--- a/libartbase/base/utils.h
+++ b/libartbase/base/utils.h
@@ -65,10 +65,14 @@ uint32_t GetTid();
 // Returns the given thread's name.
 std::string GetThreadName(pid_t tid);
 
-// Sets the name of the current thread. The name may be truncated to an
+// Sets the pthread name of the current thread. The name may be truncated to an
 // implementation-defined limit.
 void SetThreadName(const char* thread_name);
 
+// Sets the pthread name of the given thread. The name may be truncated to an
+// implementation-defined limit. Does nothing if not supported by the OS.
+void SetThreadName(pthread_t thr, const char* thread_name);
+
 // Reads data from "/proc/self/task/${tid}/stat".
 void GetTaskStats(pid_t tid, char* state, int* utime, int* stime, int* task_cpu);
 
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 6d8b6fb3f0..4c17362bba 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -957,8 +957,8 @@ bool Thread::Init(ThreadList* thread_list, JavaVMExt* java_vm, JNIEnvExt* jni_en
   // thread hasn't been through here already...
   CHECK(Thread::Current() == nullptr);
 
-  // Set pthread_self_ ahead of pthread_setspecific, that makes Thread::Current function, this
-  // avoids pthread_self_ ever being invalid when discovered from Thread::Current().
+  // Set pthread_self ahead of pthread_setspecific, that makes Thread::Current function, this
+  // avoids pthread_self ever being invalid when discovered from Thread::Current().
   tlsPtr_.pthread_self = pthread_self();
   CHECK(is_started_);
 
@@ -1266,8 +1266,14 @@ void Thread::SetCachedThreadName(const char* name) {
 }
 
 void Thread::SetThreadName(const char* name) {
+  DCHECK(this == Thread::Current() || IsSuspended());  // O.w. `this` may disappear.
   SetCachedThreadName(name);
-  ::art::SetThreadName(name);
+  if (!IsStillStarting() || this == Thread::Current()) {
+    // The RI is documented to do this only in the this == self case, which would avoid the
+    // IsStillStarting() issue below. We instead use a best effort approach.
+    ::art::SetThreadName(tlsPtr_.pthread_self /* Not necessarily current thread! */, name);
+  }  // O.w. this will normally be set when we finish starting. We can rarely fail to set the
+     // pthread name. See TODO in IsStillStarting().
   Dbg::DdmSendThreadNotification(this, CHUNK_TYPE("THNM"));
 }
 
@@ -2540,6 +2546,13 @@ bool Thread::IsStillStarting() const {
   // assigned fairly early on, and needs to be.
   // 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".
+  // TODO: I believe that SetThreadName(), ThreadGroup::GetThreads() and many jvmti functions can
+  // call this while the thread is in the process of starting. Thus we appear to have data races
+  // here on opeer and jpeer, and our result may be obsolete by the time we return. Aside from the
+  // data races, it is not immediately clear whether clients are robust against this behavior.  It
+  // may make sense to acquire a per-thread lock during the transition, and have this function
+  // REQUIRE that. `runtime_shutdown_lock_` might almost work, but is global and currently not
+  // held long enough.
   return (tlsPtr_.jpeer == nullptr && tlsPtr_.opeer == nullptr) ||
       (tlsPtr_.name.load() == kThreadNameDuringStartup);
 }
diff --git a/test/2275-pthread-name/expected-stderr.txt b/test/2275-pthread-name/expected-stderr.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/2275-pthread-name/expected-stderr.txt
diff --git a/test/2275-pthread-name/expected-stdout.txt b/test/2275-pthread-name/expected-stdout.txt
new file mode 100644
index 0000000000..baf819ce94
--- /dev/null
+++ b/test/2275-pthread-name/expected-stdout.txt
@@ -0,0 +1,10 @@
+JNI_OnLoad called
+Main Started; java name: main
+Pthread name: main
+Child's Java name: birth name
+Setting name from main
+Name was set
+Final child Java name: new name
+Final child pthread name: new name
+Final parent Java name: main
+Final parent pthread name: main
diff --git a/test/2275-pthread-name/info.txt b/test/2275-pthread-name/info.txt
new file mode 100644
index 0000000000..7cdaa631b4
--- /dev/null
+++ b/test/2275-pthread-name/info.txt
@@ -0,0 +1,2 @@
+Check that Thread.setName() and Thread.getName() generally behave reasonably,
+and particularly that the pthread name gets set as expected.
diff --git a/test/2275-pthread-name/native_getname.cc b/test/2275-pthread-name/native_getname.cc
new file mode 100644
index 0000000000..8c2f7ffacd
--- /dev/null
+++ b/test/2275-pthread-name/native_getname.cc
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
+#include "jni.h"
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <string.h>
+
+extern "C" JNIEXPORT void JNICALL Java_Main_printPthreadName(
+    [[maybe_unused]] JNIEnv* env, [[maybe_unused]] jclass klass) {
+  constexpr size_t kBufSize = 20;  // Thread names are at most 16 characters.
+  char name[kBufSize];
+  int ret = pthread_getname_np(pthread_self(), name, kBufSize);
+  if (ret == 0) {
+    printf("%s\n", name);
+  } else {
+    fprintf(stderr, "pthread_getname_np failed: %s\n", strerror(ret));
+  }
+}
diff --git a/test/2275-pthread-name/src/Main.java b/test/2275-pthread-name/src/Main.java
new file mode 100644
index 0000000000..7271a14c92
--- /dev/null
+++ b/test/2275-pthread-name/src/Main.java
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class Main {
+  static volatile boolean name_was_set = false;
+  static final String BIRTH_NAME = "birth name";
+  static final String NEW_NAME = "new name";
+
+  private static class Child implements Runnable {
+    @Override
+    public void run() {
+      String bname = Thread.currentThread().getName();
+      if (!name_was_set && !bname.equals(BIRTH_NAME)) {
+        System.err.println("Wrong birth name: " + bname);
+      }
+      while (!name_was_set) {
+        try {
+          Thread.sleep(10);
+        } catch (InterruptedException e) {
+          System.out.println("Unexpected interrupt in child");
+        }
+      }
+      System.out.println("Name was set");
+      System.out.println("Final child Java name: " + Thread.currentThread().getName());
+      System.out.print("Final child pthread name: ");
+      printPthreadName();
+    }
+  }
+
+  public static void main(String[] args) {
+    System.loadLibrary(args[0]);
+    System.out.print("Main Started; java name: ");
+    System.out.println(Thread.currentThread().getName());
+    System.out.print("Pthread name: ");
+    printPthreadName();
+    Thread t = new Thread(new Child(), BIRTH_NAME);
+    System.out.print("Child's Java name: ");
+    System.out.println(t.getName());
+    t.start();
+    try {
+      // Wait for it to get started, though it shouldn't matter.
+      Thread.sleep(10);
+    } catch (InterruptedException e) {
+      System.out.println("Unexpected interrupt in parent");
+    }
+    System.out.println("Setting name from " + Thread.currentThread().getName());
+    t.setName(NEW_NAME);
+    if (!t.getName().equals(NEW_NAME)) {
+      System.err.println("Wrong new name from main thread: " + t.getName());
+    }
+    name_was_set = true;
+    try {
+      t.join();
+    } catch (InterruptedException e) {
+      System.out.println("Unexpected interrupt in join()");
+    }
+    System.out.println("Final parent Java name: " + Thread.currentThread().getName());
+    System.out.print("Final parent pthread name: ");
+    printPthreadName();
+  }
+
+  private static native void printPthreadName();
+}
diff --git a/test/Android.bp b/test/Android.bp
index ba34037800..9d4660abef 100644
--- a/test/Android.bp
+++ b/test/Android.bp
@@ -969,6 +969,7 @@ cc_defaults {
         "2246-trace-v2/dump_trace.cc",
         "2262-miranda-methods/jni_invoke.cc",
         "2270-mh-internal-hiddenapi-use/mh-internal-hidden-api.cc",
+        "2275-pthread-name/native_getname.cc",
         "common/runtime_state.cc",
         "common/stack_inspect.cc",
     ],
diff --git a/test/knownfailures.json b/test/knownfailures.json
index dff43c55e8..0d89533ef0 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1092,6 +1092,11 @@
         "description": ["Failing on RI. Needs further investigating. Some of these use smali."]
     },
     {
+        "tests": ["2275-pthread-name"],
+        "variant": "jvm",
+        "description": ["ART specific. We try harder to set pthread thread name."]
+    },
+    {
         "tests": ["2042-reference-processing",
                   "2043-reference-pauses"],
         "variant": "jvm",