From owner-freebsd-arch@FreeBSD.ORG Sun Jan 4 11:46:06 2015 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id D24262C9; Sun, 4 Jan 2015 11:46:06 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 71897381E; Sun, 4 Jan 2015 11:46:06 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id t04Bk0Nb019720 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 4 Jan 2015 13:46:00 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua t04Bk0Nb019720 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id t04Bk0UZ019718; Sun, 4 Jan 2015 13:46:00 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 4 Jan 2015 13:46:00 +0200 From: Konstantin Belousov To: Jilles Tjoelker Subject: Re: Fixing dlopen("libpthread.so") Message-ID: <20150104114600.GC42409@kib.kiev.ua> References: <20141226165337.GJ1754@kib.kiev.ua> <20150103212837.GC46373@stack.nl> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yudcn1FV7Hsu/q59" Content-Disposition: inline In-Reply-To: <20150103212837.GC46373@stack.nl> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: threads@freebsd.org, arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 04 Jan 2015 11:46:06 -0000 --yudcn1FV7Hsu/q59 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jan 03, 2015 at 10:28:37PM +0100, Jilles Tjoelker wrote: >=20 > I think this can work, conceptually. The changes were already committed, right before I got your mail. Still, thank you for the useful review. >=20 > Does this also mean that the order of libc and libthr on the link line > no longer matters? (so SVN r265003 could be reverted?) I think yes, it could. I have similar queries already. IMO it makes sense to wait some time for the changes to settle before even asking for testing of such reversals. >=20 > It looks like this change may introduce more indirection in common paths > which might harm performance. The effect is slight and on the other hand > more single-threaded applications can benefit from non-libthr > performance. >=20 > I noticed a problem in lib/libc/gen/raise.c: it calls the INTERPOS_pause > entry instead of the INTERPOS_raise entry. Alternatively, libc could > call thr_self() and thr_kill() so libthr need not do anything (much like > how lib/libc/gen/sem_new.c works). This was noted already and corrected raise.c committed. I have mixed feeling about the proposal to change raise(3) to use threading syscalls. On the one hand, it indeed works, on the other, there are subtle differences, not all important, WRT the change. E.g., puzzling ktrace output, changed si_code from SI_USER to SI_LWP (is this a bug on its own ?) . >=20 > The number of interposers could be reduced slightly by doing simple work > like wait(), waitpid() and wait3() before instead of after > interposition. This would be an additional change and the current patch > is more like the existing code. Could you, please, explain this ? What exactly should be done before what, and how would it help ? >=20 > In lib/libc/sys/__error.c, __error_selector should be declared static or > __hidden and an accessor function added for libthr, to avoid an > additional pointer chase in a common path. One more PLT and GOT > reference in some situations may be avoided by making a __hidden alias > for __error_unthreaded and making that __error_selector's initial value. __error_unthreaded can be made static, it seems. I added __set_error_selector() and made __error_selector static. >=20 > Although __libc_interposing is properly not exported (with > __libc_interposing_slot accessor), the full benefit is not obtained > because __libc_interposing is not declared __hidden in > lib/libc/include/libc_private.h, so the compiler will generate an > indirect access anyway (and ld will fix it up using a relative > relocation). Done. >=20 > In lib/libthr/thread/thr_private.h, __error_threaded() can be declared > __hidden. Done. >=20 > Sharing __sys_* code between libc and libthr is a net loss (the two > copies of the name, GOT entry, PLT entry and longer instruction > sequences are almost certainly larger than a syscall stub (32 bytes on > amd64)), so I think the best fix for most indirection from > __libc_interposing entries is to give libthr its own copy and mark both > copies hidden, instead of adding hidden aliases now. Do you mean linking syscall stubs (__sys_* trampolines) statically into the libthr, and referencing them directly from the C code ? Yes, this will be huge benefitial, both for libthr and ld-elf.so.1, for rtld it is even kind of required, see r276646 and other uses of __sys. I thought about it. But we need something like libsyscalls.a and libsyscalls_pic.a extracted from libc first. If I am misinterpreting your suggestion, please explain it in more details. diff --git a/lib/libc/include/libc_private.h b/lib/libc/include/libc_privat= e.h index adf342d..ea1a97b 100644 --- a/lib/libc/include/libc_private.h +++ b/lib/libc/include/libc_private.h @@ -173,13 +173,13 @@ typedef pthread_func_t pthread_func_entry_t[2]; =20 extern pthread_func_entry_t __thr_jtable[]; =20 -extern int *(*__error_selector)(void); +void __set_error_selector(int *(*arg)(void)); int _pthread_mutex_init_calloc_cb_stub(pthread_mutex_t *mutex, void *(calloc_cb)(__size_t, __size_t)); =20 typedef int (*interpos_func_t)(void); interpos_func_t *__libc_interposing_slot(int interposno); -extern interpos_func_t __libc_interposing[]; +extern interpos_func_t __libc_interposing[] __hidden; =20 enum { INTERPOS_accept, diff --git a/lib/libc/sys/Symbol.map b/lib/libc/sys/Symbol.map index 7d82361..f1f57ba 100644 --- a/lib/libc/sys/Symbol.map +++ b/lib/libc/sys/Symbol.map @@ -1045,8 +1045,7 @@ FBSDprivate_1.0 { __sys_write; _writev; __sys_writev; - __error_unthreaded; - __error_selector; + __set_error_selector; nlm_syscall; gssd_syscall; __libc_interposing_slot; diff --git a/lib/libc/sys/__error.c b/lib/libc/sys/__error.c index 329d124..28cc31d 100644 --- a/lib/libc/sys/__error.c +++ b/lib/libc/sys/__error.c @@ -32,13 +32,21 @@ __FBSDID("$FreeBSD$"); =20 extern int errno; =20 -int * +static int * __error_unthreaded(void) { - return(&errno); + + return (&errno); } =20 -int *(*__error_selector)(void) =3D __error_unthreaded; +static int *(*__error_selector)(void) =3D __error_unthreaded; + +void +__set_error_selector(int *(*arg)(void)) +{ + + __error_selector =3D arg; +} =20 int * __error(void) diff --git a/lib/libthr/thread/thr_private.h b/lib/libthr/thread/thr_privat= e.h index e8edbe1..0c16b2a 100644 --- a/lib/libthr/thread/thr_private.h +++ b/lib/libthr/thread/thr_private.h @@ -914,7 +914,7 @@ void _thr_tsd_unload(struct dl_phdr_info *phdr_info) __= hidden; void _thr_sigact_unload(struct dl_phdr_info *phdr_info) __hidden; void _thr_stack_fix_protection(struct pthread *thrd); =20 -int *__error_threaded(void); +int *__error_threaded(void) __hidden; void __thr_interpose_libc(void) __hidden; pid_t __thr_fork(void); int __thr_pause(void) __hidden; diff --git a/lib/libthr/thread/thr_syscalls.c b/lib/libthr/thread/thr_sysca= lls.c index 60427d8..a4fe7e8 100644 --- a/lib/libthr/thread/thr_syscalls.c +++ b/lib/libthr/thread/thr_syscalls.c @@ -692,7 +692,7 @@ void __thr_interpose_libc(void) { =20 - __error_selector =3D __error_threaded; + __set_error_selector(__error_threaded); #define SLOT(name) \ *(__libc_interposing_slot(INTERPOS_##name)) =3D \ (interpos_func_t)__thr_##name; --yudcn1FV7Hsu/q59 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJUqSf4AAoJEJDCuSvBvK1BltwQAKbcDFHdJ2kXYZkyrdhTqKRd mBqYUz6xvSeNrfNGmGnK+PpVq5+Oky6vkuE5J0K6eS3mQphzwAflumQake6T13xe RSUz7bmef8y9mSq4fMuJMLI0k3ppFQMV7+tjdORoVOMXV9FB6FUpYNZr51mTEhrg WPtsDOo7vbGZjiDP3WFzsbWkQRftq7G7YpVdA3Ya8FNOSrTDzxdQ1yVMjex/4rP0 9i3GDs1GjgP5tOAjU2XC26/tBU+qvnAgLr0sMPbVkutSx0FZVsiHu4voGcg4zfKd RD0UOe1wF0+GW1NBlfh/aH095ay+V/qs8RFkspIBJ9P+U6mr1VmU+wF1uINyL151 W7VL7AaLxlfhIz2UUsKflN/NKAmSfV6DHo4gIC0eHxVbGOzwbXgb9Kzu2rbj1as2 hFqPZ41A4uMxCSUIXvybkvkOJs7g3eN/v7kHb/n/23eRomQ84yATvopioqhPGxna hdTzqOUlPfIYdudVX4mhUZCYYtoDkz9EmVheIEhXAXMucM/OJWuUhnimw8OX3Qae 9Du6NhNS/catg7RU+Vbc7Z6Rt/69Fd1HbWb4dc01jcFOV96BpGmUiOc0/C2qstIg +X4c9Y5QvbpUxwjPX4pcJOyHL7CoLDTUIlQ2Jzm4weanLlE6hUx/Xq92Dq9MdhZS meOJXHCDL3UMuwWZgHQ0 =JGXq -----END PGP SIGNATURE----- --yudcn1FV7Hsu/q59--