Page MenuHomePhabricator

pthread_setname_np first appeared in glibc 2.12
ClosedPublic

Authored by jroelofs on Mar 10 2017, 1:23 PM.

Details

Summary

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

Diff Detail

Event Timeline

jroelofs created this revision.Mar 10 2017, 1:23 PM
zturner edited edge metadata.Mar 10 2017, 1:53 PM

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.

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).

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?

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

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

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)

erm, nevermind, I misread.

jroelofs updated this revision to Diff 91419.Mar 10 2017, 2:40 PM

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.

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.

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 requested changes to this revision.Mar 13 2017, 4:28 AM

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.

This revision now requires changes to proceed.Mar 13 2017, 4:28 AM

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.

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 updated this revision to Diff 91566.Mar 13 2017, 8:26 AM
jroelofs edited edge metadata.

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

labath accepted this revision.Mar 13 2017, 8:29 AM

thank you.

This revision is now accepted and ready to land.Mar 13 2017, 8:29 AM
jroelofs closed this revision.Mar 13 2017, 8:37 AM

r297626