This work was originally done in bug 720778. The proposed patch adds the PR_SetThreadName and PR_GetThreadName functions for setting and getting the name of the *current thread*. It was reviewed by Honza Bambas in bug 720778 comment 39.

Patch checked in on the NSPR trunk (NSPR 4.9.1). Checking in pr/include/prthread.h; /cvsroot/mozilla/nsprpub/pr/include/prthread.h,v <-- prthread.h new revision: 3.15; previous revision: 3.14 done Checking in pr/include/md/_win95.h; /cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v <-- _win95.h new revision: 3.41; previous revision: 3.40 done Checking in pr/include/md/_winnt.h; /cvsroot/mozilla/nsprpub/pr/include/md/_winnt.h,v <-- _winnt.h new revision: 3.37; previous revision: 3.36 done Checking in pr/include/private/primpl.h; /cvsroot/mozilla/nsprpub/pr/include/private/primpl.h,v <-- primpl.h new revision: 3.95; previous revision: 3.94 done Checking in pr/src/nspr.def; /cvsroot/mozilla/nsprpub/pr/src/nspr.def,v <-- nspr.def new revision: 1.18; previous revision: 1.17 done Checking in pr/src/md/windows/ntthread.c; /cvsroot/mozilla/nsprpub/pr/src/md/windows/ntthread.c,v <-- ntthread.c new revision: 3.20; previous revision: 3.19 done Checking in pr/src/md/windows/w95thred.c; /cvsroot/mozilla/nsprpub/pr/src/md/windows/w95thred.c,v <-- w95thred.c new revision: 3.21; previous revision: 3.20 done Checking in pr/src/pthreads/ptthread.c; /cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v <-- ptthread.c new revision: 3.92; previous revision: 3.91 done Checking in pr/src/threads/prcthr.c; /cvsroot/mozilla/nsprpub/pr/src/threads/prcthr.c,v <-- prcthr.c new revision: 3.22; previous revision: 3.21 done Checking in pr/src/threads/combined/pruthr.c; /cvsroot/mozilla/nsprpub/pr/src/threads/combined/pruthr.c,v <-- pruthr.c new revision: 3.43; previous revision: 3.42 done

Thank you.

We decided to release NSPR 4.9.1 without the PR_SetThreadName and PR_GetThreadName functions. Change the symbol export version to NSPR_4.9.2. Patch checked in on the NSPR trunk (NSPR 4.9.2). Checking in nspr.def; /cvsroot/mozilla/nsprpub/pr/src/nspr.def,v <-- nspr.def new revision: 1.19; previous revision: 1.18 done

Honza: I'm going to rename the functions PR_SetCurrentThreadName and PR_GetCurrentThreadName to stress the fact that they operate on the current thread. We can replace PR_GetCurrentThreadName with PR_GetThreadName like this: PR_IMPLEMENT(const char *) PR_GetThreadName(PRThread *thread) { if (!thread) return NULL; return thread->name; } Do you want me to do that? I assume we can't implement PRStatus PR_SetThreadName(PRThread *thread, const char *name); on all platforms, specifically Mac OS X and Windows. Correct?

(In reply to Wan-Teh Chang from comment #4) > Honza: I'm going to rename the functions PR_SetCurrentThreadName and > PR_GetCurrentThreadName to stress the fact that they operate on the > current thread. > > We can replace PR_GetCurrentThreadName with PR_GetThreadName like > this: > PR_IMPLEMENT(const char *) PR_GetThreadName(PRThread *thread) > { > if (!thread) > return NULL; > return thread->name; > } > > Do you want me to do that? Definitely. Getting a name on a thread different from the current one is useful. > > I assume we can't implement > PRStatus PR_SetThreadName(PRThread *thread, const char *name); > on all platforms, specifically Mac OS X and Windows. Correct? This is not available only on Mac. Windows is able to set names on threads other then the current one.

Honza: this patch implements the changes I described in comment 4. Please review. Thanks.

Comment on attachment 630357 [details] [diff] [review] Rename PR_SetThreadName to PR_SetCurrentThreadName and change PR_GetThreadName to take a thread argument Review of attachment 630357 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab

Comment on attachment 630357 [details] [diff] [review] Rename PR_SetThreadName to PR_SetCurrentThreadName and change PR_GetThreadName to take a thread argument Honza: thank you for the review. If you don't need any other change, I'll create a NSPR_4_9_2_BETA1 CVS tag and push it to mozilla-central. Patch checked in on the NSPR trunk (NSPR 4.9.2). Checking in include/prthread.h; /cvsroot/mozilla/nsprpub/pr/include/prthread.h,v <-- prthread.h new revision: 3.16; previous revision: 3.15 done Checking in src/nspr.def; /cvsroot/mozilla/nsprpub/pr/src/nspr.def,v <-- nspr.def new revision: 1.20; previous revision: 1.19 done Checking in src/pthreads/ptthread.c; /cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v <-- ptthread.c new revision: 3.93; previous revision: 3.92 done Checking in src/threads/combined/pruthr.c; /cvsroot/mozilla/nsprpub/pr/src/threads/combined/pruthr.c,v <-- pruthr.c new revision: 3.44; previous revision: 3.43 done

(In reply to Wan-Teh Chang from comment #8) > If you don't need any other > change, I'll create a NSPR_4_9_2_BETA1 CVS tag and push it to > mozilla-central. Please go forward.

Pushed NSPR_4_9_2_BETA1 to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/838a1a9bad2b

https://hg.mozilla.org/mozilla-central/rev/838a1a9bad2b (Merged by Ed Morley)

Despite I don't like having patches for different features at the same bug I'm putting it here on the Wan-Teh's request. The name reduction is needed only on pthread platforms.

Honza: could you review and test my changes to your patch? Your patch assumes that SETNAME_LENGTH_CONSTRAINT is an odd number. I removed that assumption. I checked in the patch now to get some testing on the NSPR tinderboxes. I will make any changes you suggest. Checking in ptthread.c; /cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v <-- ptthread.c new revision: 3.94; previous revision: 3.93 done

Comment on attachment 634589 [details] [diff] [review] thread names reduction, v2, by Honza Bambas Review of attachment 634589 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/nsprpub/pr/src/pthreads/ptthread.c @@ +1665,5 @@ > + memcpy(name_dup, name, SETNAME_FRAGMENT1_LENGTH); > + name_dup[SETNAME_FRAGMENT1_LENGTH] = '.'; > + name_dup[SETNAME_FRAGMENT1_LENGTH + 1] = '.'; > + memcpy(name_dup + SETNAME_FRAGMENT1_LENGTH + 2, > + name + nameLen - SETNAME_FRAGMENT2_LENGTH, nameLen is strlen() + 1 ! You are missing - 1 at the end of this expression. This way you start copying one more char after and also copy the leading zero of the source string to last-1 position of the dup string. That is no catastrophic mistake, but you are wasting one character. I left that code (strlen() + 1) that way just to make this patch smaller and easier to review, but it seems like we should change nameLen to be just strlen() and add +1 where needed when allocating the dup string at the top of this function. I'll create the patch.

Honza: I think you're right about the off-by-one error. I believe the error is also in your patch because I did a straightforward transformation of your patch by making the following substitutions: SETNAME_FRAGMENT_LENGTH => SETNAME_FRAGMENT1_LENGTH SETNAME_FRAGMENT_LENGTH - 1 => SETNAME_FRAGMENT2_LENGTH

- changed nameLength to just strlen() - changed the split to be made by '~' instead of '..' - this is a full patch, so you need to revert what has landed and apply this new patch

Comment on attachment 634907 [details] [diff] [review] thread names reduction v3 r=wtc. Thanks. Patch checked in on the NSPR trunk (NSPR 4.9.2). Checking in pthreads/ptthread.c; /cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v <-- ptthread.c new revision: 3.95; previous revision: 3.94 done Checking in threads/combined/pruthr.c; /cvsroot/mozilla/nsprpub/pr/src/threads/combined/pruthr.c,v <-- pruthr.c new revision: 3.45; previous revision: 3.44 done

Comment on attachment 634907 [details] [diff] [review] thread names reduction v3 Review of attachment 634907 [details] [diff] [review]: ----------------------------------------------------------------- Two more comments. ::: nsprpub/pr/src/pthreads/ptthread.c @@ +1655,5 @@ > dlsym(RTLD_DEFAULT, "pthread_setname_np"); > if (!dynamic_pthread_setname_np) > return PR_SUCCESS; > > #define SETNAME_LENGTH_CONSTRAINT 15 It would be nice to add a comment to explain where this 15 character limit comes from. For example, which function (pthread_setname?) on which version of which OS has this limitation. @@ +1670,1 @@ > name_dup[SETNAME_LENGTH_CONSTRAINT] = '\0'; Nit: since 'name' is null-terminated, we can use the second memcpy call to copy the null terminator in 'name' to name_dup. memcpy(name_dup + SETNAME_FRAGMENT1_LENGTH + 1, name + nameLen - SETNAME_FRAGMENT2_LENGTH, SETNAME_FRAGMENT2_LENGTH + 1); Then we don't need to do name_dup[SETNAME_LENGTH_CONSTRAINT] = '\0'; This is similar to the memcpy(thread->name, name, nameLen + 1); technique we do above. Do you think this is worth doing?

(In reply to Wan-Teh Chang from comment #20) > Then we don't need to do > name_dup[SETNAME_LENGTH_CONSTRAINT] = '\0'; > > This is similar to the > memcpy(thread->name, name, nameLen + 1); > technique we do above. > > Do you think this is worth doing? Yes, I was thinking about that too. I think it is safe, since we are always working with zero-terminated strings.

NSPR_4_9_2_BETA2 pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/560d16727ee8

Honza: can you add a comment to document the source of the 15 character thread name limit? Thanks.

if that is ok to just put them here, in the bug, then it could be: "The 15 characters length is experimentally determined length of a null terminated string that most linux distributions and darwin accept as an argument to pthread_setname_np call. Otherwise E2BIG error is threw by the function."

Honza: thank you for providing the words for the comment. Please review this patch. Thanks.

Comment on attachment 635514 [details] [diff] [review] Explain the source of the 15-char name length limit, copy the null terminator from name Review of attachment 635514 [details] [diff] [review]: ----------------------------------------------------------------- Perfect, thanks! ::: mozilla/nsprpub/pr/src/pthreads/ptthread.c @@ +1670,5 @@ > memcpy(name_dup, name, SETNAME_FRAGMENT1_LENGTH); > name_dup[SETNAME_FRAGMENT1_LENGTH] = '~'; > memcpy(name_dup + SETNAME_FRAGMENT1_LENGTH + 1, > name + nameLen - SETNAME_FRAGMENT2_LENGTH, > + SETNAME_FRAGMENT2_LENGTH + 1); Maybe add a comment before this function call that it also copies the terminating \0.

Honza: thank you for the review. I added the comment you suggested. Patch checked in on the NSPR trunk (NSPR 4.9.2). Checking in ptthread.c; /cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v <-- ptthread.c new revision: 3.96; previous revision: 3.95 done

I attached the wrong patch. This is the correct one.

The fix for this bug caused a build failure with an older compiler on Linux. There was no good reason for declarations to be in the middle of code blocks. Here is a diff that fixes this on top of NSPR 4.9.5. mozilla/nsprpub/pr/src/pthreads/ptthread.c @@ -1619,6 +1619,17 @@ PR_IMPLEMENT(PRStatus) PR_SetCurrentThreadName(const char *name) { +#if defined(DARWIN) + int (*dynamic_pthread_setname_np)(const char*); +#else + int (*dynamic_pthread_setname_np)(pthread_t, const char*); +#endif +#define SETNAME_LENGTH_CONSTRAINT 15 +#define SETNAME_FRAGMENT1_LENGTH (SETNAME_LENGTH_CONSTRAINT >> 1) +#define SETNAME_FRAGMENT2_LENGTH \ + (SETNAME_LENGTH_CONSTRAINT - SETNAME_FRAGMENT1_LENGTH - 1) + char name_dup[SETNAME_LENGTH_CONSTRAINT + 1]; PRThread *thread; size_t nameLen; int result; @@ -1646,11 +1657,6 @@ * On OSX, pthread_setname_np is only available in 10.6 or later, so test * for it at runtime. It also may not be available on all linux distros. */ -#if defined(DARWIN) - int (*dynamic_pthread_setname_np)(const char*); -#else - int (*dynamic_pthread_setname_np)(pthread_t, const char*); -#endif *(void**)(&dynamic_pthread_setname_np) = dlsym(RTLD_DEFAULT, "pthread_setname_np"); @@ -1663,11 +1669,6 @@ * accept as an argument to pthread_setname_np. Otherwise the E2BIG * error is returned by the function. */ -#define SETNAME_LENGTH_CONSTRAINT 15 -#define SETNAME_FRAGMENT1_LENGTH (SETNAME_LENGTH_CONSTRAINT >> 1) -#define SETNAME_FRAGMENT2_LENGTH \ - (SETNAME_LENGTH_CONSTRAINT - SETNAME_FRAGMENT1_LENGTH - 1) - char name_dup[SETNAME_LENGTH_CONSTRAINT + 1]; if (nameLen > SETNAME_LENGTH_CONSTRAINT) { memcpy(name_dup, name, SETNAME_FRAGMENT1_LENGTH); name_dup[SETNAME_FRAGMENT1_LENGTH] = '~';

(In reply to Julien Pierre from comment #30) > The fix for this bug caused a build failure with an older compiler on Linux. Julien, please open a new bug for this issue and make it blocking this one. Thanks. Please also submit your patches as an attachments (https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Attaching_the_patch).