Remove HostThreadLinux/Free/NetBSD

[labath]

Summary

These classes existed only because of the GetName() static function,
which can be moved to a more natural place anyway. I move the linux
version to NativeProcessLinux (and get rid of ProcFileReader), the
freebsd version to ProcessFreeBSD (and fix a bug where it was using the
current process ID, instead of the inferior pid), and remove the NetBSD
version (which was probably incorrect anyway, as it assumes the current
process instead of the inferior.

I also add an llgs test to that verifies thread names are read
correctly.

[krytarowski]

remove the NetBSD version (which was probably incorrect anyway, as it assumes the current process instead of the inferior.

Yes, I had issues with this and I wanted to take rid of it completely during the last month. I'm patching it out in my local LLDB tree... (I also noted it in one of my TNF blog posts that I delayed it for future until it will be researched properly and have real-like user within the debugger).

We cannot call pthread_getname_np and pthread_setname_np of an inferior process, because it's limited to the current one.

FreeBSD offers a way to read inferiors thread name via PT_LWPINFO (and surrounding calls to iterate over all native threads in a tracee) and read td_name. Alternatively use kvm(3) or sysctl(7) interfaces for the same purpose. (I can see in the patch that the sysctl(7) way is used).

Currently these calls for set/get thread name were moved to LLVM. I haven't checked other systems but the NetBSD one must be called for the current process.

The original NetBSD version was ported from FreeBSD... and it inherited its oddness that used to bite me. Another important note - the NetBSD LLVM version works only for pthread_t (POSIX threads), not native threads. There is also no interface exposed for users to translate native thread (LWP) <-> POSIX thread (pthread_t), it's not a lack of feature but the design that pthread_t is fully opaque. I'm not sure what's the purpose of the LLVM interface, whether to use pthread_t for the current process or any process & native thread like what might happen on Linux. I verified that other interfaces use pthread_t API out there and I accepted that revision.

Should I implement an interface to investigate tracee's thread name [on NetBSD] in ptrace(2)? I didn't want to introduce a ptrace(2) calls for set/get thread name, as thread name stored in the kernel is shadowed in pthread_t in our libpthread(3) (this field is called pt_name). It might be a remnant from the M:N thread model from the past (pre-5.0 NetBSD release)... If there is a use-case from a debugger point of view to set a new name, I will add dedicated calls.

I think it's cleaner and simpler to use the ptrace(2) API on FreeBSD.

I was thinking about adding set/get inferiors's thread name (PT_SET_LWPNAME / PT_GET_LWPNAME are a good guess) from the checkpointing point of view... on the other hand full-process snapshoting can be done reliably mostly/only from the kernel, not from userland (at least on NetBSD)... so this apparently limits users of this interface to tracers...

If there is no real-life use-case I will implement GetName with sysctl(7) on NetBSD (after merging this code).

[krytarowski]

source/Plugins/Process/FreeBSD/FreeBSDThread.cpp

128

Besides my other notes about dedicated ptrace(2) call for reading thread name on FreeBSD, I'm not sure whether FreeBSD supports "online" debugging (there was a dedicated GDB term I forgot right now) of a running process. If so, there is no need extra space approach.

[labath]

In D30981#701902, @krytarowski wrote:

Currently these calls for set/get thread name were moved to LLVM. I haven't checked other systems but the NetBSD one must be called for the current process.

The original NetBSD version was ported from FreeBSD... and it inherited its oddness that used to bite me. Another important note - the NetBSD LLVM version works only for pthread_t (POSIX threads), not native threads. There is also no interface exposed for users to translate native thread (LWP) <-> POSIX thread (pthread_t), it's not a lack of feature but the design that pthread_t is fully opaque. I'm not sure what's the purpose of the LLVM interface, whether to use pthread_t for the current process or any process & native thread like what might happen on Linux. I verified that other interfaces use pthread_t API out there and I accepted that revision.

Yes, the versions in LLVM are for the current process (in fact, current thread) only, which is fine for usage as a debuggee. In the debugger we need another set of functions which will read the names from the process we are debugging, and may be implemented using completely different means.

Should I implement an interface to investigate tracee's thread name [on NetBSD] in ptrace(2)? I didn't want to introduce a ptrace(2) calls for set/get thread name, as thread name stored in the kernel is shadowed in pthread_t in our libpthread(3) (this field is called pt_name). It might be a remnant from the M:N thread model from the past (pre-5.0 NetBSD release)... If there is a use-case from a debugger point of view to set a new name, I will add dedicated calls.

Displaying the thread name in the debugger is definitely useful, but not all that important on the grand scale of things. If the kernel already knows about the name, then I'd recommend reading it out from there, although for non-critical functionality like this you could make the case for keeping the kernel interface simple (although, then I don't know why is the kernel storing the name in the first place). I don't know what a real use case for *setting* a thread name from a debugger would be. I suppose someone might want to do that eventually, but it's probably not that useful, so it may be better to just leave it out for now.

I think it's cleaner and simpler to use the ptrace(2) API on FreeBSD.

I am not familiar with the FreeBSD APIs so I'd leave that to the FreeBSD maintainer.

[krytarowski]

Thank you for your explanation. I will use approach similar to FreeBSD on NetBSD.

I agree that leaving the FreeBSD code is best for the maintainers - it's good enough.

[krytarowski]

packages/Python/lldbsuite/test/tools/lldb-server/thread-name/main.cpp

6

I overlooked it.

int
pthread_setname_np(pthread_t thread, const char *name, void *arg);

It should be:

::pthread_setname_np(::pthread_self(), "%s", name);

include/lldb/Host/HostNativeThread.h

 //===-- HostNativeThread.h --------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef lldb_Host_HostNativeThread_h_
 #define lldb_Host_HostNativeThread_h_
 
 #include "HostNativeThreadForward.h"
 
 #if defined(_WIN32)
 #include "lldb/Host/windows/HostThreadWindows.h"
-#elif defined(__linux__)
-#include "lldb/Host/linux/HostThreadLinux.h"
-#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-#include "lldb/Host/freebsd/HostThreadFreeBSD.h"
-#elif defined(__NetBSD__)
-#include "lldb/Host/netbsd/HostThreadNetBSD.h"
 #elif defined(__APPLE__)
 #include "lldb/Host/macosx/HostThreadMacOSX.h"
+#else
+#include "lldb/Host/posix/HostThreadPosix.h"
 #endif
 
 #endif

include/lldb/Host/HostNativeThreadForward.h

 //===-- HostNativeThreadForward.h -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef lldb_Host_HostNativeThreadForward_h_
 #define lldb_Host_HostNativeThreadForward_h_
 
 namespace lldb_private {
 #if defined(_WIN32)
 class HostThreadWindows;
 typedef HostThreadWindows HostNativeThread;
-#elif defined(__linux__)
-class HostThreadLinux;
-typedef HostThreadLinux HostNativeThread;
-#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-class HostThreadFreeBSD;
-typedef HostThreadFreeBSD HostNativeThread;
-#elif defined(__NetBSD__)
-class HostThreadNetBSD;
-typedef HostThreadNetBSD HostNativeThread;
 #elif defined(__APPLE__)
 class HostThreadMacOSX;
 typedef HostThreadMacOSX HostNativeThread;
+#else
+class HostThreadPosix;
+typedef HostThreadPosix HostNativeThread;
 #endif
 }
 
 #endif

include/lldb/Host/freebsd/HostThreadFreeBSD.h

-//===-- HostThreadFreeBSD.h -------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef lldb_Host_freebsd_HostThreadFreeBSD_h_
-#define lldb_Host_freebsd_HostThreadFreeBSD_h_
-
-#include "lldb/Host/posix/HostThreadPosix.h"
-
-#include "llvm/ADT/SmallString.h"
-#include "llvm/ADT/StringRef.h"
-
-namespace lldb_private {
-
-class HostThreadFreeBSD : public HostThreadPosix {
-public:
-  HostThreadFreeBSD();
-  HostThreadFreeBSD(lldb::thread_t thread);
-
-  static void GetName(lldb::tid_t tid, llvm::SmallVectorImpl<char> &name);
-};
-}
-
-#endif

include/lldb/Host/linux/HostThreadLinux.h

-//===-- HostThreadLinux.h ---------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef lldb_Host_linux_HostThreadLinux_h_
-#define lldb_Host_linux_HostThreadLinux_h_
-
-#include "lldb/Host/posix/HostThreadPosix.h"
-
-#include "llvm/ADT/SmallString.h"
-#include "llvm/ADT/StringRef.h"
-
-namespace lldb_private {
-
-class HostThreadLinux : public HostThreadPosix {
-public:
-  HostThreadLinux();
-  HostThreadLinux(lldb::thread_t thread);
-
-  static void GetName(lldb::thread_t thread, llvm::SmallVectorImpl<char> &name);
-};
-}
-
-#endif

include/lldb/Host/netbsd/HostThreadNetBSD.h

-//===-- HostThreadNetBSD.h -------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef lldb_Host_netbsd_HostThreadNetBSD_h_
-#define lldb_Host_netbsd_HostThreadNetBSD_h_
-
-#include "lldb/Host/posix/HostThreadPosix.h"
-
-#include "llvm/ADT/SmallString.h"
-#include "llvm/ADT/StringRef.h"
-
-namespace lldb_private {
-
-class HostThreadNetBSD : public HostThreadPosix {
-public:
-  HostThreadNetBSD();
-  HostThreadNetBSD(lldb::thread_t thread);
-
-  static void SetName(lldb::thread_t tid, llvm::StringRef &name);
-  static void GetName(lldb::thread_t tid, llvm::SmallVectorImpl<char> &name);
-};
-}
-
-#endif

packages/Python/lldbsuite/test/tools/lldb-server/thread-name/Makefile

+LEVEL = ../../../make
+
+ENABLE_THREADS := YES
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/tools/lldb-server/thread-name/TestGdbRemoteThreadName.py

+from __future__ import print_function
+
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestGdbRemoteThreadName(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def run_and_check_name(self, expected_name):
+        self.test_sequence.add_log_lines(["read packet: $vCont;c#a8",
+                                          {"direction": "send",
+                                           "regex":
+                                           r"^\$T([0-9a-fA-F]{2})([^#]+)#[0-9a-fA-F]{2}$",
+                                           "capture": {
+                                               1: "signal",
+                                               2: "key_vals_text"}},
+                                          ],
+                                         True)
+
+        context = self.expect_gdbremote_sequence()
+        self.assertIsNotNone(context)
+
+        sigint = lldbutil.get_signal_number("SIGINT")
+        self.assertEqual(sigint, int(context.get("signal"), 16))
+        kv_dict = self.parse_key_val_dict(context.get("key_vals_text"))
+        self.assertEqual(expected_name, kv_dict.get("name"))
+
+    @llgs_test
+    def test(self):
+        """ Make sure lldb-server can retrieve inferior thread name"""
+        self.init_llgs_test()
+        self.build()
+        self.set_inferior_startup_launch()
+        procs = self.prep_debug_monitor_and_inferior()
+
+        self.run_and_check_name("hello world")
+        self.run_and_check_name("goodbye world")

packages/Python/lldbsuite/test/tools/lldb-server/thread-name/main.cpp

+#include <pthread.h>
+#include <signal.h>
+
+void set_thread_name(const char *name) {
+#if defined(__linux__) || defined(__NetBSD__)
+  ::pthread_setname_np(::pthread_self(), name);
+#elif defined(__FreeBSD__)
+  ::pthread_set_name_np(::pthread_self(), name);
+#elif defined(__APPLE__)
+  ::pthread_setname_np(name);
+#endif
+}
+
+int main() {
+  set_thread_name("hello world");
+  raise(SIGINT);
+  set_thread_name("goodbye world");
+  raise(SIGINT);
+  return 0;
+}

source/Host/CMakeLists.txt

add_host_subdirectory(macosx
       )
 
 
   elseif (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
     add_host_subdirectory(linux
       linux/AbstractSocket.cpp
       linux/Host.cpp
       linux/HostInfoLinux.cpp
-      linux/HostThreadLinux.cpp
       linux/LibcGlue.cpp
       linux/Support.cpp
       )
     list(APPEND LLDB_PLUGINS lldbPluginProcessLinux)
     if (CMAKE_SYSTEM_NAME MATCHES "Android")
       add_host_subdirectory(android
         android/HostInfoAndroid.cpp
         android/LibcGlue.cpp
         )
     endif()
   elseif (CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
     add_host_subdirectory(freebsd
       freebsd/Host.cpp
       freebsd/HostInfoFreeBSD.cpp
-      freebsd/HostThreadFreeBSD.cpp
       )
 
   elseif (CMAKE_SYSTEM_NAME MATCHES "NetBSD")
     add_host_subdirectory(netbsd
       netbsd/Host.cpp
       netbsd/HostInfoNetBSD.cpp
-      netbsd/HostThreadNetBSD.cpp
       )
   endif()
 endif()
 
 if (${get_python_libdir})
   # Call a python script to gather the arch-specific libdir for
   # modules like the lldb module.
   execute_process(

source/Host/freebsd/HostThreadFreeBSD.cpp

-//===-- HostThreadFreeBSD.cpp -----------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-// lldb Includes
-#include "lldb/Host/freebsd/HostThreadFreeBSD.h"
-#include "lldb/Host/Host.h"
-
-// C includes
-#include <errno.h>
-#include <pthread.h>
-#if defined(__FreeBSD__)
-#include <pthread_np.h>
-#endif
-#include <stdlib.h>
-#include <sys/sysctl.h>
-#include <sys/user.h>
-
-// C++ includes
-#include <string>
-
-using namespace lldb_private;
-
-HostThreadFreeBSD::HostThreadFreeBSD() {}
-
-HostThreadFreeBSD::HostThreadFreeBSD(lldb::thread_t thread)
-    : HostThreadPosix(thread) {}
-
-void HostThreadFreeBSD::GetName(lldb::tid_t tid,
-                                llvm::SmallVectorImpl<char> &name) {
-  name.clear();
-  int pid = Host::GetCurrentProcessID();
-
-  struct kinfo_proc *kp = nullptr, *nkp;
-  size_t len = 0;
-  int error;
-  int ctl[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PID | KERN_PROC_INC_THREAD,
-                (int)pid};
-
-  while (1) {
-    error = sysctl(ctl, 4, kp, &len, nullptr, 0);
-    if (kp == nullptr || (error != 0 && errno == ENOMEM)) {
-      // Add extra space in case threads are added before next call.
-      len += sizeof(*kp) + len / 10;
-      nkp = (struct kinfo_proc *)realloc(kp, len);
-      if (nkp == nullptr) {
-        free(kp);
-        return;
-      }
-      kp = nkp;
-      continue;
-    }
-    if (error != 0)
-      len = 0;
-    break;
-  }
-
-  for (size_t i = 0; i < len / sizeof(*kp); i++) {
-    if (kp[i].ki_tid == (lwpid_t)tid) {
-      name.append(kp[i].ki_tdname, kp[i].ki_tdname + strlen(kp[i].ki_tdname));
-      break;
-    }
-  }
-  free(kp);
-}

source/Host/linux/HostThreadLinux.cpp

-//===-- HostThreadLinux.cpp -------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#include "lldb/Host/linux/HostThreadLinux.h"
-#include "Plugins/Process/Linux/ProcFileReader.h"
-#include "lldb/Utility/DataBuffer.h"
-
-#include "llvm/ADT/SmallVector.h"
-
-using namespace lldb_private;
-
-HostThreadLinux::HostThreadLinux() : HostThreadPosix() {}
-
-HostThreadLinux::HostThreadLinux(lldb::thread_t thread)
-    : HostThreadPosix(thread) {}
-
-void HostThreadLinux::GetName(lldb::thread_t thread,
-                              llvm::SmallVectorImpl<char> &name) {
-  // Read /proc/$TID/comm file.
-  lldb::DataBufferSP buf_sp =
-      process_linux::ProcFileReader::ReadIntoDataBuffer(thread, "comm");
-  const char *comm_str = (const char *)buf_sp->GetBytes();
-  const char *cr_str = ::strchr(comm_str, '\n');
-  size_t length = cr_str ? (cr_str - comm_str) : strlen(comm_str);
-
-  name.clear();
-  name.append(comm_str, comm_str + length);
-}

source/Host/netbsd/HostThreadNetBSD.cpp

-//===-- HostThreadNetBSD.cpp -----------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-// lldb Includes
-#include "lldb/Host/netbsd/HostThreadNetBSD.h"
-#include "lldb/Host/Host.h"
-
-// C includes
-#include <errno.h>
-#include <pthread.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/sysctl.h>
-#include <sys/user.h>
-
-// C++ includes
-#include <string>
-
-using namespace lldb_private;
-
-HostThreadNetBSD::HostThreadNetBSD() {}
-
-HostThreadNetBSD::HostThreadNetBSD(lldb::thread_t thread)
-    : HostThreadPosix(thread) {}
-
-void HostThreadNetBSD::SetName(lldb::thread_t thread, llvm::StringRef &name) {
-  ::pthread_setname_np(thread, "%s", const_cast<char *>(name.data()));
-}
-
-void HostThreadNetBSD::GetName(lldb::thread_t thread,
-                               llvm::SmallVectorImpl<char> &name) {
-  char buf[PTHREAD_MAX_NAMELEN_NP];
-  ::pthread_getname_np(thread, buf, PTHREAD_MAX_NAMELEN_NP);
-
-  name.clear();
-  name.append(buf, buf + strlen(buf));
-}

source/Plugins/Process/FreeBSD/FreeBSDThread.cpp

void FreeBSDThread::SetName(const char *name) {
   if (m_thread_name_valid)
     m_thread_name.assign(name);
   else
     m_thread_name.clear();
 }
 
 const char *FreeBSDThread::GetName() {
   if (!m_thread_name_valid) {
-    llvm::SmallString<32> thread_name;
-    HostNativeThread::GetName(GetID(), thread_name);
-    m_thread_name = thread_name.c_str();
+    m_thread_name.clear();
+    int pid = GetProcess()->GetID();
+
+    struct kinfo_proc *kp = nullptr, *nkp;
+    size_t len = 0;
+    int error;
+    int ctl[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PID | KERN_PROC_INC_THREAD,
+                  pid};
+
+    while (1) {
+      error = sysctl(ctl, 4, kp, &len, nullptr, 0);
+      if (kp == nullptr || (error != 0 && errno == ENOMEM)) {
+        // Add extra space in case threads are added before next call.
+        len += sizeof(*kp) + len / 10;
+        nkp = (struct kinfo_proc *)realloc(kp, len);
+        if (nkp == nullptr) {
+          free(kp);
+          return nullptr;
+        }
+        kp = nkp;
+        continue;
+      }
+      if (error != 0)
+        len = 0;
+      break;
+    }
+
+    for (size_t i = 0; i < len / sizeof(*kp); i++) {
+      if (kp[i].ki_tid == (lwpid_t)tid) {
+        m_thread_name.append(kp[i].ki_tdname,
+                             kp[i].ki_tdname + strlen(kp[i].ki_tdname));
+        break;
+      }
+    }
+    free(kp);
     m_thread_name_valid = true;
   }
 
   if (m_thread_name.empty())
     return NULL;
   return m_thread_name.c_str();
 }
 

source/Plugins/Process/Linux/NativeThreadLinux.cpp

 
 #include "NativeProcessLinux.h"
 #include "NativeRegisterContextLinux.h"
 #include "SingleStepCheck.h"
 
 #include "lldb/Core/State.h"
 #include "lldb/Host/HostNativeThread.h"
 #include "lldb/Host/linux/Ptrace.h"
+#include "lldb/Host/linux/Support.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/lldb-enumerations.h"
 
 #include "llvm/ADT/SmallString.h"
 
 #include "Plugins/Process/POSIX/CrashReason.h"
 

 }
 
 NativeThreadLinux::NativeThreadLinux(NativeProcessLinux *process,
                                      lldb::tid_t tid)
     : NativeThreadProtocol(process, tid), m_state(StateType::eStateInvalid),
       m_stop_info(), m_reg_context_sp(), m_stop_description() {}
 
 std::string NativeThreadLinux::GetName() {
-  NativeProcessProtocolSP process_sp = m_process_wp.lock();
-  if (!process_sp)
-    return "<unknown: no process>";
-
-  // const NativeProcessLinux *const process =
-  // reinterpret_cast<NativeProcessLinux*> (process_sp->get ());
-  llvm::SmallString<32> thread_name;
-  HostNativeThread::GetName(GetID(), thread_name);
-  return thread_name.c_str();
+  NativeProcessLinux &process = GetProcess();
+
+  auto BufferOrError = getProcFile(process.GetID(), GetID(), "comm");
+  if (!BufferOrError)
+    return "";
+  return BufferOrError.get()->getBuffer().rtrim('\n');
 }
 
 lldb::StateType NativeThreadLinux::GetState() { return m_state; }
 
 bool NativeThreadLinux::GetStopReason(ThreadStopInfo &stop_info,
                                       std::string &description) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));