Patchwork libgcc: refer to pthread_create, not pthread_cancel

login
register
mail settings
Submitter Roland McGrath
Date Jan. 27, 2012, 1:07 a.m.
Message ID <x57jobtqx89w.fsf@frobland.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/138095/
State New
Headers show

Comments

Roland McGrath - Jan. 27, 2012, 1:07 a.m.
For GNU systems, the fact that libgcc/libstdc++ refers to pthread_cancel is
utterly bizarre.  I don't know of any rationale for this that has ever
applied to glibc.  AFAICT the choice was made based on the foibles of
various non-glibc systems.

In practice, things are working fine on GNU systems now, of course.  But
they'd work just as well for all extant glibc versions if the reference
were to pthread_create instead.  That's what makes clear sense on its face:
if the program is multithreaded, pthread_create is the one function that
certainly must have been linked in.

It is actively troublesome for the case of static linking.  It necessitates
linking in a lot of dead code in a program that uses pthreads but doesn't
use pthread_cancel.  It only actually works right at all because glibc
added a special hack to cope with this bizarre reference to pthread_cancel.
It would be nice if we could remove that someday.  (I don't intend to start
any general discussion here about the level of support for static linking.
It's just an example of what's nonsensical about this usage.)

With any kind of linking, it has whatever side effects linking in
pthread_cancel has.  There is at least one port of glibc (to a non-GNU
system) where pthread_cancel has a link-time warning because it's not fully
supported.  Getting this warning on every link that uses C++ at all is
rather bewildering, especially to application developers who don't use
pthreads directly at all or don't have any idea what pthread_cancel is.

What do folks think about this change?  It obviously should have no effect
whatsoever on builds not using glibc.  I'm pretty confident that it won't
have any bad effect on a build using any extant version of glibc that had
pthreads at all.


Thanks,
Roland


2012-01-26  Roland McGrath  <mcgrathr@google.com>

	* gthr-posix.h [neither FreeBSD nor Solaris] (__gthread_active_p):
	If __GLIBC__ is defined, refer to pthread_create, not pthread_cancel.
Jakub Jelinek - Jan. 27, 2012, 7:42 a.m.
On Thu, Jan 26, 2012 at 05:07:38PM -0800, Roland McGrath wrote:
> What do folks think about this change?  It obviously should have no effect
> whatsoever on builds not using glibc.  I'm pretty confident that it won't
> have any bad effect on a build using any extant version of glibc that had
> pthreads at all.
> 

pthread_create is what has been referenced there up to GCC 3.4.
So please see the rationale for changing it from pthread_create to
pthread_cancel.
http://gcc.gnu.org/ml/gcc-patches/2005-01/msg01093.html
http://gcc.gnu.org/ml/gcc-patches/2004-10/msg02517.html
(and lots of gcc-patches mails in between the two).

	Jakub
Roland McGrath - Jan. 27, 2012, 6:02 p.m.
I see.  There certainly should have been a comment in the code about why
pthread_cancel was chosen.  I still think it's a particularly poor choice.

For glibc, I think pthread_getspecific or pthread_key_create are better
choices.  Those are much smaller functions that don't bring very much dead
code into the link, and they are things actually used by libstdc++ et al.

For supporting the interception cases, it's really not entirely safe to use
any of the public API functions.  Someone might have intercepted any of
them in the same way as was cited for pthread_create.

In glibc, we have exported __ names for various things, including
__pthread_getspecific and __pthread_key_create.  IMHO one of those is the
best choice.  They are part of the permanent public ABI and so won't ever
go away, but they are not part of the public API that anyone writing an
interceptor library would ever want to touch.


Thanks,
Roland

Patch

diff --git a/libgcc/gthr-posix.h b/libgcc/gthr-posix.h
index 46054f6..688253d 100644
--- a/libgcc/gthr-posix.h
+++ b/libgcc/gthr-posix.h
@@ -241,18 +241,24 @@  __gthread_active_p (void)
 
 #else /* neither FreeBSD nor Solaris */
 
+/* The bionic (Android) C library does not provide pthread_cancel.
+   The GNU C library provides pthread_cancel, but it is a poor
+   choice there.  For static linking, referring to pthread_cancel
+   causes it to be linked in even when it's never actually used.
+   For a program to be multi-threaded the only thing that it
+   certainly must be using is pthread_create.  */
+
+#if defined(__GLIBC__) || defined(__BIONIC__)
+# define GTHR_ACTIVE_PROXY	__gthrw_(pthread_create)
+#else
+# define GTHR_ACTIVE_PROXY	__gthrw_(pthread_cancel)
+#endif
+
 static inline int
 __gthread_active_p (void)
 {
-/* Android's C library does not provide pthread_cancel, check for
-   `pthread_create' instead.  */
-#ifndef __BIONIC__
   static void *const __gthread_active_ptr
-    = __extension__ (void *) &__gthrw_(pthread_cancel);
-#else
-  static void *const __gthread_active_ptr
-    = __extension__ (void *) &__gthrw_(pthread_create);
-#endif
+    = __extension__ (void *) &GTHR_ACTIVE_PROXY;
   return __gthread_active_ptr != 0;
 }