nptl: Move pthread_atfork to libc_nonshared.a

Message ID 20180228151604.8CAF243994575@oldenburg.str.redhat.com
State New
Headers show
Series
  • nptl: Move pthread_atfork to libc_nonshared.a
Related show

Commit Message

Florian Weimer Feb. 28, 2018, 3:16 p.m.
libpthread_nonshared.a is unused after this, so remove it from the
build.

There is no ABI impact because pthread_atfork was implemented using
__register_atfork in libc even before this change.

pthread_atfork has to be a weak alias because pthread_* names are not
reserved in libc.

2018-02-28  Florian Weimer  <fweimer@redhat.com>

	Move pthread_atfork to libc.  Remove libpthread_nonshared.a.
	* nptl/Makefile (routines): Add pthread_atfork.
	(static-only-routines): Set to pthread_atfork.
	(libpthread-routines): Remove pthread_atfork.
	(libpthread-static-only-routines): Remove.
	(install): Update comment.
	(libpthread.so): Do not install libpthread_nonshared.a.
	(tests): Do not link with libpthread_nonshared.a.
	(generated): Remove libpthread_nonshared.a.
	* nptl/pthread_atfork.c (pthread_atfork): Turn into weak alias.
	* sysdeps/nptl/Makeconfig (shared-thread-library): Do not link
	with libpthread_nonshared.a.

Comments

Carlos O'Donell Feb. 28, 2018, 11:17 p.m. | #1
On 02/28/2018 07:16 AM, Florian Weimer wrote:
> libpthread_nonshared.a is unused after this, so remove it from the

> build.


Thanks for pushing this into upstream. This is more elegant than my
proposal in 2013: https://sourceware.org/ml/libc-alpha/2013-10/msg00065.html

> There is no ABI impact because pthread_atfork was implemented using

> __register_atfork in libc even before this change.


So just to be clear the __pthread_atfork moves to libc_nonshared.a?
Thus making it available for all applications using libc?

> pthread_atfork has to be a weak alias because pthread_* names are not

> reserved in libc.


OK.

> 2018-02-28  Florian Weimer  <fweimer@redhat.com>

> 

> 	Move pthread_atfork to libc.  Remove libpthread_nonshared.a.

> 	* nptl/Makefile (routines): Add pthread_atfork.

> 	(static-only-routines): Set to pthread_atfork.

> 	(libpthread-routines): Remove pthread_atfork.

> 	(libpthread-static-only-routines): Remove.

> 	(install): Update comment.

> 	(libpthread.so): Do not install libpthread_nonshared.a.

> 	(tests): Do not link with libpthread_nonshared.a.

> 	(generated): Remove libpthread_nonshared.a.

> 	* nptl/pthread_atfork.c (pthread_atfork): Turn into weak alias.

> 	* sysdeps/nptl/Makeconfig (shared-thread-library): Do not link

> 	with libpthread_nonshared.a.


This is less code than my original patch and that makes it better :-)

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


> diff --git a/nptl/Makefile b/nptl/Makefile

> index de37fb4680..ba586c247e 100644

> --- a/nptl/Makefile

> +++ b/nptl/Makefile

> @@ -30,8 +30,9 @@ install-lib-ldscripts := libpthread.so

>  

>  routines = alloca_cutoff forward libc-lowlevellock libc-cancellation \

>  	   libc-cleanup libc_pthread_init libc_multiple_threads \

> -	   register-atfork pthread_self

> +	   register-atfork pthread_atfork pthread_self


OK.

>  shared-only-routines = forward

> +static-only-routines = pthread_atfork


OK.

>  

>  # We need to provide certain routines for compatibility with existing

>  # binaries.

> @@ -106,7 +107,7 @@ libpthread-routines = nptl-init vars events version pt-interp \

>  		      pthread_cancel pthread_testcancel \

>  		      pthread_setcancelstate pthread_setcanceltype \

>  		      pthread_once \

> -		      old_pthread_atfork pthread_atfork \

> +		      old_pthread_atfork \


OK.

>  		      pthread_getcpuclockid \

>  		      pthread_clock_gettime pthread_clock_settime \

>  		      shm-directory \

> @@ -147,7 +148,6 @@ libpthread-routines = nptl-init vars events version pt-interp \

>  

>  libpthread-shared-only-routines = version pt-interp pt-allocrtsig \

>  				  unwind-forcedunwind

> -libpthread-static-only-routines = pthread_atfork


OK.

>  

>  # Since cancellation handling is in large parts handled using exceptions

>  # we have to compile some files with exception handling enabled, some

> @@ -476,16 +476,12 @@ lib-noranlib: $(addprefix $(objpfx),$(extra-objs))

>  

>  # What we install as libpthread.so for programs to link against is in fact a

>  # link script.  It contains references for the various libraries we need.

> -# The libpthread.so object is not complete since some functions are only

> -# defined in libpthread_nonshared.a.

>  # We need to use absolute paths since otherwise local copies (if they exist)

>  # of the files are taken by the linker.

>  install: $(inst_libdir)/libpthread.so

>  

>  $(inst_libdir)/libpthread.so: $(common-objpfx)format.lds \

>  			      $(objpfx)libpthread.so$(libpthread.so-version) \

> -			      $(inst_libdir)/$(patsubst %,$(libtype.oS),\

> -							$(libprefix)pthread) \


OK.

>  			      $(+force)

>  	(echo '/* GNU ld script';\

>  	 echo '   Use the shared library, but some functions are only in';\

> @@ -644,14 +640,12 @@ $(addprefix $(objpfx), \

>    $(filter-out $(tests-static) $(xtests-static) $(tests-reverse) \

>      $(tests-nolibpthread), \

>      $(tests) $(tests-internal) $(xtests) $(test-srcs))): \

> -	$(objpfx)libpthread.so \

> -	$(objpfx)libpthread_nonshared.a

> +	$(objpfx)libpthread.so


OK.

>  $(objpfx)tst-unload: $(libdl)

>  # $(objpfx)../libc.so is used instead of $(common-objpfx)libc.so,

>  # since otherwise libpthread.so comes before libc.so when linking.

>  $(addprefix $(objpfx), $(tests-reverse)): \

> -  $(objpfx)../libc.so $(objpfx)libpthread.so \

> -  $(objpfx)libpthread_nonshared.a

> +  $(objpfx)../libc.so $(objpfx)libpthread.so


OK.

>  $(objpfx)../libc.so: $(common-objpfx)libc.so ;

>  $(addprefix $(objpfx),$(tests-static) $(xtests-static)): $(objpfx)libpthread.a

>  

> @@ -681,8 +675,7 @@ $(objpfx)$(multidir)/crtn.o: $(objpfx)crtn.o $(objpfx)$(multidir)/

>  	ln -f $< $@

>  endif

>  

> -generated += libpthread_nonshared.a \

> -	     multidir.mk tst-atfork2.mtrace tst-cancel-wrappers.out \

> +generated += multidir.mk tst-atfork2.mtrace tst-cancel-wrappers.out \


OK.

>  	     tst-tls6.out

>  

>  generated += $(objpfx)tst-atfork2.mtrace \

> diff --git a/nptl/pthread_atfork.c b/nptl/pthread_atfork.c

> index 1c4ba23b8a..5ac62bf879 100644

> --- a/nptl/pthread_atfork.c

> +++ b/nptl/pthread_atfork.c

> @@ -53,5 +53,5 @@ __pthread_atfork (void (*prepare) (void), void (*parent) (void),

>  #ifndef __pthread_atfork

>  extern int pthread_atfork (void (*prepare) (void), void (*parent) (void),

>  			   void (*child) (void)) attribute_hidden;

> -strong_alias (__pthread_atfork, pthread_atfork)

> +weak_alias (__pthread_atfork, pthread_atfork)


OK.

>  #endif

> diff --git a/sysdeps/nptl/Makeconfig b/sysdeps/nptl/Makeconfig

> index 5595f356b1..ce8998bbf5 100644

> --- a/sysdeps/nptl/Makeconfig

> +++ b/sysdeps/nptl/Makeconfig

> @@ -21,8 +21,7 @@

>  

>  have-thread-library = yes

>  

> -shared-thread-library = $(common-objpfx)nptl/libpthread_nonshared.a \

> -			$(common-objpfx)nptl/libpthread.so

> +shared-thread-library = $(common-objpfx)nptl/libpthread.so


OK.

>  static-thread-library = $(common-objpfx)nptl/libpthread.a

>  

>  rpath-dirs += nptl

> 



-- 
Cheers,
Carlos.
Florian Weimer March 1, 2018, 7:19 a.m. | #2
On 03/01/2018 12:17 AM, Carlos O'Donell wrote:
> So just to be clear the __pthread_atfork moves to libc_nonshared.a?

> Thus making it available for all applications using libc?


Yes, that's right.  I will install the patch.

Thanks,
Florian
Zack Weinberg March 1, 2018, 1:12 p.m. | #3
On Wed, Feb 28, 2018 at 10:16 AM, Florian Weimer <fweimer@redhat.com> wrote:
> libpthread_nonshared.a is unused after this, so remove it from the

> build.


Yay!

>  # What we install as libpthread.so for programs to link against is in fact a

>  # link script.  It contains references for the various libraries we need.

> -# The libpthread.so object is not complete since some functions are only

> -# defined in libpthread_nonshared.a.

>  # We need to use absolute paths since otherwise local copies (if they exist)

>  # of the files are taken by the linker.

>  install: $(inst_libdir)/libpthread.so

>

>  $(inst_libdir)/libpthread.so: $(common-objpfx)format.lds \

>                               $(objpfx)libpthread.so$(libpthread.so-version) \

> -                             $(inst_libdir)/$(patsubst %,$(libtype.oS),\

> -                                                       $(libprefix)pthread) \

>                               $(+force)

>         (echo '/* GNU ld script';\

>          echo '   Use the shared library, but some functions are only in';\


Is there still any use for the libpthread.so link script after this?

zw
Florian Weimer March 1, 2018, 1:23 p.m. | #4
On 03/01/2018 02:12 PM, Zack Weinberg wrote:
> Is there still any use for the libpthread.so link script after this?


Only to communicate that such things as linker scripts do exist and you 
cannot assume that .so files are ELF objects.  Replacing the file 
doesn't seem to worthwhile.  But I realize now that I didn't update the 
comment in my follow-up fix.  Oh well, third patch coming up.

Thanks,
Florian
Zack Weinberg March 1, 2018, 1:32 p.m. | #5
On Thu, Mar 1, 2018 at 8:23 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 03/01/2018 02:12 PM, Zack Weinberg wrote:

>>

>> Is there still any use for the libpthread.so link script after this?

>

>

> Only to communicate that such things as linker scripts do exist and you

> cannot assume that .so files are ELF objects.


Well, libc.so is still a linker script?

> Replacing the file doesn't

> seem to worthwhile.


It would simplify nptl/Makefile, that makes it worth it in my book.

zw
Florian Weimer March 1, 2018, 1:42 p.m. | #6
On 03/01/2018 02:32 PM, Zack Weinberg wrote:
>> Replacing the file doesn't seem to worthwhile.

> 

> It would simplify nptl/Makefile, that makes it worth it in my book.


Fair enough.  Here's a patch to use the general logic to install the 
symbolic link.

Thanks,
Florian
Subject: [PATCH] nptl: Turn libpthread.so into a symbolic link to the real DSO
To: libc-alpha@sourceware.org

The linker script is no longer needed.

2018-03-01  Florian Weimer  <fweimer@redhat.com>

	* nptl/Makefile (install-lib-ldscripts): Remove.
	(install): Remove rule.
	($(inst_libdir)/libpthread.so): Likewise.

diff --git a/nptl/Makefile b/nptl/Makefile
index e18f9a6b84..94be92c789 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -26,7 +26,6 @@ headers := pthread.h semaphore.h bits/semaphore.h
 
 extra-libs := libpthread
 extra-libs-others := $(extra-libs)
-install-lib-ldscripts := libpthread.so
 
 routines = alloca_cutoff forward libc-lowlevellock libc-cancellation \
 	   libc-cleanup libc_pthread_init libc_multiple_threads \
@@ -473,24 +472,6 @@ ifeq (yes,$(build-shared))
 # Make sure these things are built in the `make lib' pass so they can be used
 # to run programs during the `make others' pass.
 lib-noranlib: $(addprefix $(objpfx),$(extra-objs))
-
-# What we install as libpthread.so for programs to link against is in fact a
-# link script.  It contains references for the various libraries we need.
-# We need to use absolute paths since otherwise local copies (if they exist)
-# of the files are taken by the linker.
-install: $(inst_libdir)/libpthread.so
-
-$(inst_libdir)/libpthread.so: $(common-objpfx)format.lds \
-			      $(objpfx)libpthread.so$(libpthread.so-version) \
-			      $(+force)
-	(echo '/* GNU ld script';\
-	 echo '   Use the shared library, but some functions are only in';\
-	 echo '   the static library, so try that secondarily.  */';\
-	 cat $<; \
-	 echo 'GROUP ( $(slibdir)/libpthread.so$(libpthread.so-version)' \
-	      ')' \
-	) > $@.new
-	mv -f $@.new $@
 endif
Zack Weinberg March 1, 2018, 3:14 p.m. | #7
On Thu, Mar 1, 2018 at 8:42 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 03/01/2018 02:32 PM, Zack Weinberg wrote:

>>>

>>> Replacing the file doesn't seem to worthwhile.

>> It would simplify nptl/Makefile, that makes it worth it in my book.

> Fair enough.  Here's a patch to use the general logic to install the

> symbolic link.


OK.

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index de37fb4680..ba586c247e 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -30,8 +30,9 @@  install-lib-ldscripts := libpthread.so
 
 routines = alloca_cutoff forward libc-lowlevellock libc-cancellation \
 	   libc-cleanup libc_pthread_init libc_multiple_threads \
-	   register-atfork pthread_self
+	   register-atfork pthread_atfork pthread_self
 shared-only-routines = forward
+static-only-routines = pthread_atfork
 
 # We need to provide certain routines for compatibility with existing
 # binaries.
@@ -106,7 +107,7 @@  libpthread-routines = nptl-init vars events version pt-interp \
 		      pthread_cancel pthread_testcancel \
 		      pthread_setcancelstate pthread_setcanceltype \
 		      pthread_once \
-		      old_pthread_atfork pthread_atfork \
+		      old_pthread_atfork \
 		      pthread_getcpuclockid \
 		      pthread_clock_gettime pthread_clock_settime \
 		      shm-directory \
@@ -147,7 +148,6 @@  libpthread-routines = nptl-init vars events version pt-interp \
 
 libpthread-shared-only-routines = version pt-interp pt-allocrtsig \
 				  unwind-forcedunwind
-libpthread-static-only-routines = pthread_atfork
 
 # Since cancellation handling is in large parts handled using exceptions
 # we have to compile some files with exception handling enabled, some
@@ -476,16 +476,12 @@  lib-noranlib: $(addprefix $(objpfx),$(extra-objs))
 
 # What we install as libpthread.so for programs to link against is in fact a
 # link script.  It contains references for the various libraries we need.
-# The libpthread.so object is not complete since some functions are only
-# defined in libpthread_nonshared.a.
 # We need to use absolute paths since otherwise local copies (if they exist)
 # of the files are taken by the linker.
 install: $(inst_libdir)/libpthread.so
 
 $(inst_libdir)/libpthread.so: $(common-objpfx)format.lds \
 			      $(objpfx)libpthread.so$(libpthread.so-version) \
-			      $(inst_libdir)/$(patsubst %,$(libtype.oS),\
-							$(libprefix)pthread) \
 			      $(+force)
 	(echo '/* GNU ld script';\
 	 echo '   Use the shared library, but some functions are only in';\
@@ -644,14 +640,12 @@  $(addprefix $(objpfx), \
   $(filter-out $(tests-static) $(xtests-static) $(tests-reverse) \
     $(tests-nolibpthread), \
     $(tests) $(tests-internal) $(xtests) $(test-srcs))): \
-	$(objpfx)libpthread.so \
-	$(objpfx)libpthread_nonshared.a
+	$(objpfx)libpthread.so
 $(objpfx)tst-unload: $(libdl)
 # $(objpfx)../libc.so is used instead of $(common-objpfx)libc.so,
 # since otherwise libpthread.so comes before libc.so when linking.
 $(addprefix $(objpfx), $(tests-reverse)): \
-  $(objpfx)../libc.so $(objpfx)libpthread.so \
-  $(objpfx)libpthread_nonshared.a
+  $(objpfx)../libc.so $(objpfx)libpthread.so
 $(objpfx)../libc.so: $(common-objpfx)libc.so ;
 $(addprefix $(objpfx),$(tests-static) $(xtests-static)): $(objpfx)libpthread.a
 
@@ -681,8 +675,7 @@  $(objpfx)$(multidir)/crtn.o: $(objpfx)crtn.o $(objpfx)$(multidir)/
 	ln -f $< $@
 endif
 
-generated += libpthread_nonshared.a \
-	     multidir.mk tst-atfork2.mtrace tst-cancel-wrappers.out \
+generated += multidir.mk tst-atfork2.mtrace tst-cancel-wrappers.out \
 	     tst-tls6.out
 
 generated += $(objpfx)tst-atfork2.mtrace \
diff --git a/nptl/pthread_atfork.c b/nptl/pthread_atfork.c
index 1c4ba23b8a..5ac62bf879 100644
--- a/nptl/pthread_atfork.c
+++ b/nptl/pthread_atfork.c
@@ -53,5 +53,5 @@  __pthread_atfork (void (*prepare) (void), void (*parent) (void),
 #ifndef __pthread_atfork
 extern int pthread_atfork (void (*prepare) (void), void (*parent) (void),
 			   void (*child) (void)) attribute_hidden;
-strong_alias (__pthread_atfork, pthread_atfork)
+weak_alias (__pthread_atfork, pthread_atfork)
 #endif
diff --git a/sysdeps/nptl/Makeconfig b/sysdeps/nptl/Makeconfig
index 5595f356b1..ce8998bbf5 100644
--- a/sysdeps/nptl/Makeconfig
+++ b/sysdeps/nptl/Makeconfig
@@ -21,8 +21,7 @@ 
 
 have-thread-library = yes
 
-shared-thread-library = $(common-objpfx)nptl/libpthread_nonshared.a \
-			$(common-objpfx)nptl/libpthread.so
+shared-thread-library = $(common-objpfx)nptl/libpthread.so
 static-thread-library = $(common-objpfx)nptl/libpthread.a
 
 rpath-dirs += nptl