PATCH for REVIEW: Implement Host::SetThreadName on Linux

This is my first submission, so any feedback/comments on anything I'm messing up or should do differently are greatly appreciated. I also don't have a Mac yet (on order) - so I haven't tested on that platform just yet...

This patch:
- Host::SetThreadName was declared void, but the comments in Host.h said it returns a bool. It now returns true on success, false on failure.
- Host::SetThreadName is now implemented on Linux.
- I added code to set the lldb thread name in ThreadCreateTrampoline. I've found this helps me when debugging and looking at the output of htop or top as I get something other than 'lldb' for each thread. As an example, ps now looks like this:

mikesart@mikesart-rad:~/data/src/llvm/llvm/tools/lldb$ ps -Leo pid,tid,class,wchan:14,comm | grep `pidof lldb`
18162 18162 TS futex_wait_que lldb
18162 18163 TS poll_schedule_ input
18162 18164 TS poll_schedule_ editline
18162 18165 TS poll_schedule_ editline_output
18162 18166 TS n_tty_read commandline_io
18162 18348 TS poll_schedule_ operation
18162 18350 TS wait wait4(pid=18349
18162 18351 TS poll_schedule_ stdio
18162 18352 TS futex_wait_que internal-state

If anyone has any complaints against naming the lldb threads for any reason, this part could be pulled out or possibly wrapped in a command line argument?

Patch is down below. Also on https://gist.github.com/mikesartain/5550857

Thanks much!
-Mike

Index: include/lldb/Host/Host.h

This is my first submission, so any feedback/comments on anything I'm messing up ...

And, of course, I did screw it up. I just ran across ThreadCreated and noticed that's where thread names are being set for OSX.

Updated patch:
- Host::SetThreadName was declared void, but the comments in Host.h said it returns a bool. It now returns true on success, false on failure.
- Host::SetThreadName is now implemented on Linux.
- Added code to set the lldb thread name in Host::ThreadCreated() in linux/Host.cpp

Path down below, and also: https://gist.github.com/mikesartain/5550857

Thanks!
-Mike

Index: source/Host/common/Host.cpp

Hi Mike,

Thanks for contributing the patch! This will help many of us in debugging lldb. Good to know there are other Linux lldb developers out there. :slight_smile:

Some comments about the patch...

+#elif defined (__linux__)
+ void *fn = dlsym (RTLD_DEFAULT, "pthread_setname_np");
+ if (fn)
+ {

I believe this pthread function can be added by including pthread.h for Linux. That way you don't need dlsym and can just re-use most of the code that is already there for apple.

+ const char *lastdot = ::strrchr( thread_name, '.' );
+
+ if (lastdot && lastdot != thread_name)
+ thread_name = lastdot + 1;
+ ::strncpy (namebuf, thread_name, sizeof(namebuf));
+ namebuf[ sizeof(namebuf) - 1 ] = 0;
+
+ int namebuflen = strlen(namebuf);

It would have been nice to use std::string and its' facilities. It would make this easier to write and understand but what you have is fine.

Let me know what you think. I'll be able to commit the patch for you afterwards.

Thanks,
Matt

Thanks for contributing the patch! This will help many of us in debugging lldb. Good to know there are other Linux lldb developers out there. :slight_smile:

Thanks Matt. We're doing a lot of debugging on Linux and I'm hoping to do whatever I can to make lldb a much better debugger than gdb / cgdb - what most of use are currently using. I'm just now starting to get in the code and poke around and see what's there. If you have any suggestions on where I could help and not duplicate work, please fire away.

Some comments about the patch...

> +#elif defined (__linux__)
> + void *fn = dlsym (RTLD_DEFAULT, "pthread_setname_np");
> + if (fn)
> + {

I believe this pthread function can be added by including pthread.h for Linux. That way you don't need dlsym and can just re-use most of the code that is already there for apple.

pthread_setname_np was added in glibc v2.12, so anything running less than that would run into issues if we called it directly. As an example on the Ubuntu side, I believe 10.04 LTS shipped with 2.11.1. What are your thoughts / requirements for minspecs like this?

> + const char *lastdot = ::strrchr( thread_name, '.' );
> +
> + if (lastdot && lastdot != thread_name)
> + thread_name = lastdot + 1;
> + ::strncpy (namebuf, thread_name, sizeof(namebuf));
> + namebuf[ sizeof(namebuf) - 1 ] = 0;
> +
> + int namebuflen = strlen(namebuf);

It would have been nice to use std::string and its' facilities. It would make this easier to write and understand but what you have is fine.

I'll switch this over to using std::string on Monday and resubmit. Would have done it today but I just got back from a Pioneer Farms fieldtrip. Whee! :slight_smile:

Thanks for the reply. Have a great weekend.
-Mike

> Some comments about the patch...
>
> > +#elif defined (__linux__)
> > + void *fn = dlsym (RTLD_DEFAULT, "pthread_setname_np");
> > + if (fn)
> > + {
>
> I believe this pthread function can be added by including pthread.h for
Linux. That way you don't need dlsym and can just re-use most of the code
that is already there for apple.

pthread_setname_np was added in glibc v2.12, so anything running less than
that would run into issues if we called it directly. As an example on the
Ubuntu side, I believe 10.04 LTS shipped with 2.11.1. What are your
thoughts / requirements for minspecs like this?

...

> > + const char *lastdot = ::strrchr( thread_name, '.' );
> > +
> > + if (lastdot && lastdot != thread_name)
> > + thread_name = lastdot + 1;
> > + ::strncpy (namebuf, thread_name, sizeof(namebuf));
> > + namebuf[ sizeof(namebuf) - 1 ] = 0;
> > +
> > + int namebuflen = strlen(namebuf);
>
> It would have been nice to use std::string and its' facilities. It would
make this easier to write and understand but what you have is fine.

I just wrote this up with std::string, but since Host::ThreadCreated()
takes a regular C string, converting that to a std::string does a memory
allocation. I personally would prefer the above code to it being more
optimal, but that may also just be me not finding it more difficult to
understand than the std::string.

I'll code to whatever the project guidelines are though, so when you get a
chance, let me know what folks on this project prefer and how to proceed
with the glibc v2.12 issue and I'll resubmit all this.

Thanks much.
-Mike

Thanks for contributing the patch! This will help many of us in debugging lldb. Good to know there are other Linux lldb developers out there. :slight_smile:

Thanks Matt. We're doing a lot of debugging on Linux and I'm hoping to do whatever I can to make lldb a much better debugger than gdb / cgdb - what most of use are currently using.

We look forward to getting more people working on LLDB in general, I am glad to see more people getting on board!

I'm just now starting to get in the code and poke around and see what's there. If you have any suggestions on where I could help and not duplicate work, please fire away.

The libedit code in the "lldb" binary seems to be a bit flaky on linux. Sometimes the (lldb) prompt does't show up and other times the newlines after entered commands come out in the command output. Anything that can be done to solidify the libedit based code on linux would be a great help.

Some comments about the patch...

+#elif defined (__linux__)
+ void *fn = dlsym (RTLD_DEFAULT, "pthread_setname_np");
+ if (fn)
+ {

I believe this pthread function can be added by including pthread.h for Linux. That way you don't need dlsym and can just re-use most of the code that is already there for apple.

pthread_setname_np was added in glibc v2.12, so anything running less than that would run into issues if we called it directly. As an example on the Ubuntu side, I believe 10.04 LTS shipped with 2.11.1. What are your thoughts / requirements for minspecs like this?

dlsym() is the right way to go to avoid build issues and distribution issues. We will need to keep this in for at least a few years until we require at least a certain version of glibc on linux.

Welcome Mike,

We're doing a lot of debugging on Linux and I'm hoping to do whatever I can to make lldb a much better debugger than gdb / cgdb - what most of us are currently using.

FYI, the tests decorated with expectedFailureLinux or skipOnLinux are cross-referenced with bugzilla reports. Here is a query that can provide a summary:
  http://llvm.org/bugs/buglist.cgi?query_format=specific&order=relevance%20desc&bug_status=open&product=lldb&content=Linux&list_id=37673

We try to keep the status up to date, and the ones that are likely to change soon are 14600, 14416, 15824, and 14541 which are on our plate currently. We've investigated most of issues a little, such as 14637 that Greg mentioned below. We've also been trying to improve test coverage when running:
  lldb/test$python dotest.py --executable=/path/to/lldb

Note also that many tests fail with 32-bit inferiors that aren't captured in the bugzilla reports yet:
  lldb/test$python dotest.py --executable=/path/to/lldb -A i386

FreeBSD support and 32-bit lldb are also in need of a friend! Cheers,

- Ashok

I committed your patch in r181722. Thanks again!

Some other comments inline...

Thanks for contributing the patch! This will help many of us in debugging lldb. Good to know there are other Linux lldb developers out there. :slight_smile:

Thanks Matt. We're doing a lot of debugging on Linux and I'm hoping to do whatever I can to make lldb a much better debugger than gdb / cgdb - what most of use are currently using.

We look forward to getting more people working on LLDB in general, I am glad to see more people getting on board!

I'm just now starting to get in the code and poke around and see what's there. If you have any suggestions on where I could help and not duplicate work, please fire away.

The libedit code in the "lldb" binary seems to be a bit flaky on linux. Sometimes the (lldb) prompt does't show up and other times the newlines after entered commands come out in the command output. Anything that can be done to solidify the libedit based code on linux would be a great help.

The libedit issue would be a good start. Also, here is the lldb bugzilla tracker: http://llvm.org/bugs/buglist.cgi?product=lldb&component=All%20Bugs&resolution=---
The libedit issue is here: http://llvm.org/bugs/show_bug.cgi?id=14637

Presently, we are working towards fixing multi-threaded debugging on Linux. We've made some progress (can view inferior threads, stop all inferior threads on a breakpoint hit). We also run into higher priority issues from time to time (build breakage, regressions, etc.) that we try to resolve asap.

Hi,

[...]

The libedit code in the "lldb" binary seems to be a bit flaky on linux.
Sometimes the (lldb) prompt does't show up and other times the newlines
after entered commands come out in the command output. Anything that can
be done to solidify the libedit based code on linux would be a great help.

I think one of the causes for the libedit related issues is that the libedit
package on Debian/Ubuntu is quite old (2.11-20080614-5). It looks like a
snapshot of NetBSD's version on 2008-06-14 with some Debian/Ubuntu patches.

For example, for the "(llbd) prompt" bug, I noticed that both lldb and the
python interpreter call into libedit, but the library doesn't seem to
support it. This was apparently fixed in NetBSD's code
(http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/Attic/term.c?rev=1.47&content-type=text/x-cvsweb-markup&sortby=date).

Another example is that the libedit's el_gets() doesn't distinguish
between an error and an EOF:
"Returns the line read if successful, or NULL if characters were read or if
an error occurred."
This doesn't help for detecting a CTRL-D (meaning "quit") from the user.
Looking at NetBSD's man page for editline, this has been fixed in later
versions:
"If an error occurred, count is set to -1 and errno contains the error code
that caused it."

In both cases, a more recent libedit would have helped.

Yacine