pthread_setname_np first appeared in glibc 2.12

[jroelofs]

Summary

Alternatively, I could teach llvm:set_thread_name() how to take a thread parameter, and use that here instead.

[zturner]

Hmm.. I thought this code was gone? Are you at tip of trunk? If so then I must be crazy. I added llvm::set_thread_name() which already handles all this correctly, and I thought I converted all of LLDB's clients over to using the LLVM function.

[jroelofs]

Yeah, I'm on tip of trunk as of a few minutes ago.

llvm::set_thread_name() exists, but it doesn't take a thread parameter like this one does (no idea if that's (still?) used or not in lldb).

[zturner]

It looks like I just forgot to remove those functions. All the callers have been updated to use the llvm method. Can you just delete this method (and the one in HostThreadFreeBSD?

[jroelofs]

sure

[jroelofs]

looks like lldb.xcodeproj refers to the files which I'll be deleting... do I need to worry about that?

[zturner]

I don't think you need to delete the entire files. Just the SetName function.

[zturner]

Oh wait, there's nothing else in those files.... Usually someone on the Apple team picks up the slack when changes are required to the Xcode project. If you use Xcode, you're welcome to remove them yourself.

(Goes without saying, but make sure the build still succeeds after removing them)

[jroelofs]

erm, nevermind, I misread.

[jroelofs]

Built successfully on linux (lots of tests fail, not sure if that's expected or not?). I don't have a FreeBSD machine, but it looks "obviously" the same.

[zturner]

labath@ will need to comment on this. I wasn't aware that we were getting the thread name anywhere else. Im not sure if this code is even important, but hopefully it is not.

The test suite is full of flakiness, but as long as it's the same before and after your change, it doesn't seem like anything is broken.

[jroelofs]

@labath thoughts?

[jroelofs]

In D30844#698294, @zturner wrote:

The test suite is full of flakiness, but as long as it's the same before and after your change, it doesn't seem like anything is broken.

Results are pretty much the same, with 1 or 2 twinkling tests (couldn't say which ones it is though: I'm not at all familiar with lldb's test harness).

Before:

===================
Test Result Summary
===================
Test Methods:       1897
Reruns:               14
Success:             877
Expected Failure:    109
Failure:             306
Error:                21
Exceptional Exit:      0
Unexpected Success:   15
Skip:                568
Timeout:               1
Expected Timeout:      0

After:

===================
Test Result Summary
===================
Test Methods:       1897
Reruns:               14
Success:             876
Expected Failure:    108
Failure:             307
Error:                21
Exceptional Exit:      0
Unexpected Success:   16
Skip:                568
Timeout:               1
Expected Timeout:      0

[labath]

We need the getter code to get the name of the threads of the process we are debugging, so this cannot go away. It also probably does not make sense to move this code into llvm, as it is quite debugger-specific and very unportable.

We probably don't have a test that we read the thread name correctly, so the test suite would not have caught this. I've added a todo for myself to add one.

If you want to get your build working, I suggest you just remove the setting code. It's conceivable that we may want to change the inferior thread name from a debugger, but we don't have that functionality now, and the code is wrong anyway, so there's no harm in removing it.

source/Plugins/Process/Linux/NativeThreadLinux.cpp

100 ↗

(On Diff #91419)

This modification is incorrect. The code is supposed to read the name of the thread we are debugging, not our own thread name.

[jroelofs]

If you want to get your build working, I suggest you just remove the setting code.

@labath I have my local build working, but I don't want to carry local patches if possible. How about the original patch, which adds the glibc 2.12 check:

 void HostThreadLinux::SetName(lldb::thread_t thread, llvm::StringRef name) {
-#if (defined(__GLIBC__) && defined(_GNU_SOURCE)) || defined(__ANDROID__)
+#if (((__GLIBC__ > 2) || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) && \
+     defined(_GNU_SOURCE)) || defined(__ANDROID__)
   ::pthread_setname_np(thread, name.data());
 #else
   (void)thread;

It's no worse than what was already there, makes forward progress in supporting this particular host platform, and avoids a bunch more yak shaving.

[labath]

In D30844#699265, @jroelofs wrote:

If you want to get your build working, I suggest you just remove the setting code.

@labath I have my local build working, but I don't want to carry local patches if possible.

Yes, that's what I meant. :)

How about the original patch, which adds the glibc 2.12 check:

 void HostThreadLinux::SetName(lldb::thread_t thread, llvm::StringRef name) {
-#if (defined(__GLIBC__) && defined(_GNU_SOURCE)) || defined(__ANDROID__)
+#if (((__GLIBC__ > 2) || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) && \
+     defined(_GNU_SOURCE)) || defined(__ANDROID__)
   ::pthread_setname_np(thread, name.data());
 #else
   (void)thread;

Just delete that function altogether and keep the GetName as-is. If you don't want to do the inlining zachary suggested, that's fine by me. I was planning to kill the ProcFileReader class soon, so I can do that then.

[jroelofs]

It builds, so the code was dead anyway. Didn't re-run the tests.

[labath]

thank you.

[jroelofs]

r297626

include/lldb/Host/linux/HostThreadLinux.h

   HostThreadLinux();
   HostThreadLinux(lldb::thread_t thread);
 
-  static void SetName(lldb::thread_t thread, llvm::StringRef name);
   static void GetName(lldb::thread_t thread, llvm::SmallVectorImpl<char> &name);
 };
 }

source/Host/linux/HostThreadLinux.cpp

 
 #include "llvm/ADT/SmallVector.h"
 
-#include <pthread.h>
-
 using namespace lldb_private;
 
 HostThreadLinux::HostThreadLinux() : HostThreadPosix() {}

 HostThreadLinux::HostThreadLinux(lldb::thread_t thread)
     : HostThreadPosix(thread) {}
 
-void HostThreadLinux::SetName(lldb::thread_t thread, llvm::StringRef name) {
-#if (defined(__GLIBC__) && defined(_GNU_SOURCE)) || defined(__ANDROID__)
-  ::pthread_setname_np(thread, name.data());
-#else
-  (void)thread;
-  (void)name;
-#endif
-}
-
 void HostThreadLinux::GetName(lldb::thread_t thread,
                               llvm::SmallVectorImpl<char> &name) {
   // Read /proc/$TID/comm file.