From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/5997 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 7/8] add the thrd_xxxxxx functions Date: Sat, 30 Aug 2014 20:46:43 -0400 Message-ID: <20140831004643.GP12888@brightrain.aerifal.cx> References: <22215ff2f880382340930f78cc746565a625a806.1409423162.git.Jens.Gustedt@inria.fr> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1409446023 19447 80.91.229.3 (31 Aug 2014 00:47:03 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 31 Aug 2014 00:47:03 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-6004-gllmg-musl=m.gmane.org@lists.openwall.com Sun Aug 31 02:46:56 2014 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1XNtIG-0000qv-9C for gllmg-musl@plane.gmane.org; Sun, 31 Aug 2014 02:46:56 +0200 Original-Received: (qmail 24494 invoked by uid 550); 31 Aug 2014 00:46:55 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 24479 invoked from network); 31 Aug 2014 00:46:55 -0000 Content-Disposition: inline In-Reply-To: <22215ff2f880382340930f78cc746565a625a806.1409423162.git.Jens.Gustedt@inria.fr> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:5997 Archived-At: On Sat, Aug 30, 2014 at 08:47:32PM +0200, Jens Gustedt wrote: > diff --git a/src/sched/thrd_yield.c b/src/sched/thrd_yield.c > new file mode 100644 > index 0000000..301e953 > --- /dev/null > +++ b/src/sched/thrd_yield.c > @@ -0,0 +1,7 @@ > +#include > +#include "syscall.h" > + > +void thrd_yield() > +{ > + (void)syscall(SYS_sched_yield); > +} Cast-to-void isn't used in musl. > diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c > index ed265fb..3212502 100644 > --- a/src/thread/pthread_create.c > +++ b/src/thread/pthread_create.c > @@ -4,11 +4,14 @@ > #include "libc.h" > #include > #include > +#include > > void *__mmap(void *, size_t, int, int, int, off_t); > int __munmap(void *, size_t); > int __mprotect(void *, size_t, int); > > +#define __THRD_C11 ((void*)(uintptr_t)-1) > + > static void dummy_0() > { > } > @@ -123,6 +126,19 @@ static int start(void *p) > return 0; > } > > +static _Noreturn void __thrd_exit(int result) { > + __pthread_exit((void*)(intptr_t)result); > +} > + > + > +static int start_c11(void *p) > +{ > + thrd_t self = p; > + int (*start)(void*) = (int(*)(void*)) self->start; > + __thrd_exit(start(self->start_arg)); > + return 0; > +} > + > #define ROUND(x) (((x)+PAGE_SIZE-1)&-PAGE_SIZE) > > /* pthread_key_create.c overrides this */ > @@ -145,8 +161,8 @@ void *__copy_tls(unsigned char *); > > static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp, void *(*entry)(void *), void *restrict arg) > { > - int ret; > - size_t size, guard; > + int ret, c11 = (attrp == __THRD_C11); > + size_t size, guard = 0; Is there a reason guard needs to be initialized to 0 here? I'm interested in case you think there's a bug now (that would be silently fixed) but also since it's nice not to initialize when we don't have to, so that static analysis can catch code paths that don't explicitly set a value. > struct pthread *self, *new; > unsigned char *map = 0, *stack = 0, *tsd = 0, *stack_limit; > unsigned flags = CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND > @@ -167,6 +183,9 @@ static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restr > self->tsd = (void **)__pthread_tsd_main; > libc.threaded = 1; > } > + if (c11) { > + attrp = 0; > + } Mixed spaces/tabs. > if (attrp) attr = *attrp; > > __acquire_ptc(); > @@ -234,7 +253,10 @@ static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restr > new->canary = self->canary; > > a_inc(&libc.threads_minus_1); > - ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid); > + if (c11) > + ret = __clone(start_c11, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid); > + else > + ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid); Couldn't this be "c11 ? start_c11 : start" to avoid duplicating the rest of the call? > +static int __thrd_create(thrd_t *thr, > + thrd_start_t func, > + void *arg) { > + /* In the best of all worlds this is a tail call. */ > + int ret = __pthread_create(thr, __THRD_C11, (void * (*)(void *))func, arg); > + if (thrd_success) > + return ret ? thrd_error : thrd_success; > + /* In case of UB may also return ENOSYS or EAGAIN. */ > + return ret; > +} This error logic looks wrong. In the case where thrd_success is nonzero, you're wrongly mapping all errors (including EAGAIN which should be thrd_nomem) to thrd_error. In the case where thrd_success is zero, you're returning errno codes directly rather than mapping them. I think this just needs to be an unconditional switch. Also, since it doesn't depend on anything in pthread_create.c, it would be nice to put it in a separate thrd_create.c. It's not a big deal but it shaves off a small function in POSIX programs that don't use thrd_create. > diff --git a/src/thread/thrd_current.c b/src/thread/thrd_current.c > new file mode 100644 > index 0000000..1728535 > --- /dev/null > +++ b/src/thread/thrd_current.c > @@ -0,0 +1,11 @@ > +#include "pthread_impl.h" > +#include > + > +/* Not all arch have __pthread_self as a symbol. On some this results > + in some magic. So this "call" to __pthread_self is not necessary a > + tail call. In particular, other than the appearance, thrd_current > + can not be implemented as a weak symbol. */ > +pthread_t thrd_current() > +{ > + return __pthread_self(); > +} Shouldn't this be thrd_t? It doesn't matter since they're the same underlying type, but it "looks wrong" anyway. :) > diff --git a/src/thread/thrd_detach.c b/src/thread/thrd_detach.c > new file mode 100644 > index 0000000..72cdfba > --- /dev/null > +++ b/src/thread/thrd_detach.c > @@ -0,0 +1,12 @@ > +#include > + > +int __pthread_detach(thrd_t t); > + > +int thrd_detach(thrd_t t) > +{ > + /* In the best of all worlds this is a tail call. */ > + int ret = __pthread_detach(t); > + if (thrd_success) > + return ret ? thrd_error : thrd_success; > + return ret; > +} This seems to have the same bug, in principle, as thrd_create. Since there are no defined errors for pthread_detach, I think this should just be: return thrd_success ? thrd_success : ret; I'd really just prefer that all of these can't-fail cases be a straight tail call with no support for nonzero thrd_success values. But as long as the code is correct and the inefficiency is trivially optimized out, I'm not going to spend a lot of time arguing about it. I do think it's telling, though, that the (albeit minimal) complexity of trying to handle the case where thrd_success is nonzero seems to have caused a couple bugs -- this is part of why I don't like having multiple code paths where one path is untestable/untested. To me, a code path that's never going to get tested is a much bigger offense than an assumption that a constant has the value we decided it has. > diff --git a/src/thread/thrd_join.c b/src/thread/thrd_join.c > new file mode 100644 > index 0000000..a8c7aed > --- /dev/null > +++ b/src/thread/thrd_join.c > @@ -0,0 +1,16 @@ > +#include "pthread_impl.h" > +#include > +#include > + > +int __munmap(void *, size_t); > + > +/* C11 threads cannot be canceled, so there is no need for a > + cancelation function pointer, here. */ > +int thrd_join(thrd_t t, int *res) > +{ > + int tmp; > + while ((tmp = t->tid)) __timedwait(&t->tid, tmp, 0, 0, 0, 0, 0); > + if (res) *res = (int)(intptr_t)t->result; > + if (t->map_base) __munmap(t->map_base, t->map_size); > + return thrd_success; > +} I'd rather avoid duplicating this function too. Rich