Date: Sun, 4 Jan 2015 13:46:00 +0200
From: Konstantin Belousov <kostikbel@gmail.com>
To: Jilles Tjoelker <jilles@stack.nl>
Cc: threads@freebsd.org, arch@freebsd.org
Subject: Re: Fixing dlopen("libpthread.so")
Message-ID: <20150104114600.GC42409@kib.kiev.ua>
In-Reply-To: <20150103212837.GC46373@stack.nl>
References: <20141226165337.GJ1754@kib.kiev.ua> <20150103212837.GC46373@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150104114600.GC42409>