posix: set thread names

posix: set thread names

There's a non-portable function for doing this: pthread_setname_np.
It's supported by OS X >= 10.6 and the Xcode debugger will show the thread
names if they're provided.  On Linux, support has just recently been added
to glibc for the same function.  Since OS coverage of the function is so
spotty, we look for the symbol at runtime; on Linux we fall back to another
implementation of the same functionality if the function isn't available.

[reland of r49212, with Mac fixed and Linux disabled; see
comments in code as to why Linux is disabled]

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49465

[Markus (顧孟勤), 2010-06-08 19:03:02]

LGTM

Can you check what it does to the renderers? The mangling of the commandline
could conceivably interfere as both methods manipulate essentially the same
data. Check what /proc/XXX/cmdline shows afterwards (both for the main
process and for each of the threads).

Also, check whether this still works in the seccomp sandbox or if we
explicitly need to allow it in the sandbox -- in fact, I suspect we need to
do so. But it should be a trivial change
to sandbox/linux/seccomp/syscall_table.c. You might want to include that
change in you change list.


Markus

[Mark Mentovai, 2010-06-08 19:06:20]

http://codereview.chromium.org/2774001/diff/2002/6001
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/2774001/diff/2002/6001#newcode8
base/platform_thread_posix.cc:8: #include <dlfcn.h>
dlfcn.h < errno.h

http://codereview.chromium.org/2774001/diff/2002/6001#newcode16
base/platform_thread_posix.cc:16: #include <sys/syscall.h>
This seems to be OS_LINUX too. Maybe it should be in an OS_LINUX block and not a
!OS_MACOSX block.

http://codereview.chromium.org/2774001/diff/2002/6001#newcode75
base/platform_thread_posix.cc:75: // Because of varying platform support of
pthread_setname_np, we test for it
of->for, and if you’ll indulge me, get rid of the “we.”

http://codereview.chromium.org/2774001/diff/2002/6001#newcode83
base/platform_thread_posix.cc:83: err =
dynamic_pthread_setname_np(pthread_self(), name);
http://repo.or.cz/w/glibc.git/blob/86a4c67fb91b82c7b47c115b88ab01b1a696f10f:/...

Yuck, it bails out (ERANGE) if strlen(name) >= 16.

Maybe you could work with the upstream to truncate instead of fail? (Good luck.)
Since TASK_COM_LEN isn’t exposed, we’re otherwise stuck truncating to a magic
constant 16 on our own in this code.

http://codereview.chromium.org/2774001/diff/2002/6001#newcode91
base/platform_thread_posix.cc:91: }
What’s the stuff that gets written to /proc/self/task/pid/comm? Is it worth
mentioning that glibc does it but you’ve elected not to for ${reason}?

http://codereview.chromium.org/2774001/diff/2002/6001#newcode95
base/platform_thread_posix.cc:95: PLOG(ERROR) << "Setting thread name";
pthread_setname_np doesn’t (necessarily) set errno, it returns an error number,
so PLOG is inappropriate. This is how the entire pthread interface works.

prctl does set errno, so you can PLOG when it fails.

Having to split the LOG up will let you do the more conventional PLOG(ERROR) <<
"prctl";.

[Evan Martin, 2010-06-08 20:32:13]

Ok, new patch up, still going through trybots.

http://codereview.chromium.org/2774001/diff/2002/6001
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/2774001/diff/2002/6001#newcode91
base/platform_thread_posix.cc:91: }
On 2010/06/08 19:06:20, Mark Mentovai wrote:
> What’s the stuff that gets written to /proc/self/task/pid/comm? Is it worth
> mentioning that glibc does it but you’ve elected not to for ${reason}?

That's for threads other than the current thread.  Our PlatformThread API is
only for setting the current thread name anyway, which is why this impl gets to
be so simple.

[Mark Mentovai, 2010-06-08 20:38:10]

http://codereview.chromium.org/2774001/diff/11001/12001
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/2774001/diff/11001/12001#newcode77
base/platform_thread_posix.cc:77: int (*dynamic_pthread_setname_np)(pthread_t,
const char*);
Uh-oh.

On the Mac, this is declared as int pthread_setname_np(const char*);. It doesn’t
accept a pthread_t argument.

(Sorry, I assumed you had checked argument compatibility.)

http://codereview.chromium.org/2774001/diff/11001/12001#newcode78
base/platform_thread_posix.cc:78:
*reinterpret_cast<void**>(&dynamic_pthread_setname_np) =
It would be awesome to cache this in a static, but we have thread-safe statics
off. That sucks.

http://codereview.chromium.org/2774001/diff/11001/12001#newcode83
base/platform_thread_posix.cc:83: // we are forced to hardcode it.
we

http://codereview.chromium.org/2774001/diff/11001/12001#newcode89
base/platform_thread_posix.cc:89: // BSDs are similar.
Heh. Could you send e-mail to our OtherBSD guys to check on this?

[Evan Martin, 2010-06-08 20:46:12]

http://codereview.chromium.org/2774001/diff/11001/12001
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/2774001/diff/11001/12001#newcode77
base/platform_thread_posix.cc:77: int (*dynamic_pthread_setname_np)(pthread_t,
const char*);
On 2010/06/08 20:38:10, Mark Mentovai wrote:
> Uh-oh.
> 
> On the Mac, this is declared as int pthread_setname_np(const char*);. It
doesn’t
> accept a pthread_t argument.
> 
> (Sorry, I assumed you had checked argument compatibility.)

And it has a different name on FreeBSD and OpenBSD.
FFFFFFFFFFFUUUUUUUUU

I'm going to just implement this for Linux/Mac.  If someone cares about FreeBSD
they can implement it.  None of the tools we use on the platforms we do support
even make use of this change.  The complexity of this change is completely out
of control.

[Evan Martin, 2010-06-08 21:09:50]

The code paths now are more different than they're similar, so I split them into
separate functions.  Duplicating the string-shorten is better than having ifdefs
for the type of the function, the length to clip at, the call to the function,
etc.

[Mark Mentovai, 2010-06-08 22:15:42]

Finally. LGTM. Except you still need to provide a no-op implementation for the
non-Linux-non-Mac guys.

http://codereview.chromium.org/2774001/diff/16001/17002
File base/platform_thread_posix.cc (right):

http://codereview.chromium.org/2774001/diff/16001/17002#newcode94
base/platform_thread_posix.cc:94: // don't do the name length clipping as above
because empirically
When I read “empirical” in conjunction with something open-source like “Linux
kernel,” I think “why are you satisfied with that?”

I checked and the kernel is safe and will handle truncation for you.
2.6.34/kernel/sys.c, for example. The limit is 16 - 1 (for '\0'), TASK_COMM_LEN
from include/linux/sched.h. I hate how the world has all these definitions of
the same constant all over the place.

http://codereview.chromium.org/2774001/diff/16001/17002#newcode101
base/platform_thread_posix.cc:101: #endif  // defined(OS_LINUX)
If not Linux and not Mac, there should still be an empty definition of this
function.