Date: Sun, 4 Jan 2015 19:20:27 +0100
From: Jilles Tjoelker <jilles@stack.nl>
To: Konstantin Belousov <kostikbel@gmail.com>
Cc: threads@freebsd.org, arch@freebsd.org
Subject: Re: Fixing dlopen("libpthread.so")
Message-ID: <20150104182026.GA61250@stack.nl>
In-Reply-To: <20150104114600.GC42409@kib.kiev.ua>
References: <20141226165337.GJ1754@kib.kiev.ua> <20150103212837.GC46373@stack.nl> <20150104114600.GC42409@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jan 04, 2015 at 01:46:00PM +0200, Konstantin Belousov wrote:
> On Sat, Jan 03, 2015 at 10:28:37PM +0100, Jilles Tjoelker wrote:
> > I think this can work, conceptually.
> The changes were already committed, right before I got your mail.
> Still, thank you for the useful review.
> > 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.
OK.
> > 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.
> > 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 ?) .
SI_LWP already occurs with libthr's raise(), and with
pthread_kill(pthread_self(), sig); which POSIX says is equivalent.
> > 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 ?
In libc, define the below wait3(). All code in libc that allows libthr to
interpose wait3() and in libthr that interposes wait3() can then go
away.
pid_t
wait3(int *istat, int options, struct rusage *rup)
{
return (((pid_t (*)(pid_t, int *, int, struct rusage *))
__libc_interposing[INTERPOS_wait4])(WAIT_ANY, istat, options, rup));
}
This change is also possible for wait() and waitpid(), but not for e.g.
sigwait() which has different [EINTR] behaviour than sigtimedwait().
> > 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.
> > 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.
> > In lib/libthr/thread/thr_private.h, __error_threaded() can be declared
> > __hidden.
> Done.
The changes look good to me.
> > 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.
Your interpretation is correct.
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150104182026.GA61250>