Re: [PATCH 7/8] add the thrd_xxxxxx functions

[Jens Gustedt <jens.gustedt@inria.fr>]

[-- Attachment #1: Type: text/plain, Size: 8877 bytes --]

Am Samstag, den 30.08.2014, 20:46 -0400 schrieb Rich Felker:
> > 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 <sys/mman.h>
> >  #include <string.h>
> > +#include <threads.h>
> >  
> >  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.

I don't remember everything exactly, but during my tries there was a
situation where it was necessary.

The fact is that there is one code path in which guard isn't set. In
that case normally tsd is set, so the usage of guard below is
fine. Only the setting of tsd is

tsd = stack - __pthread_tsd_size;

which is sufficiently complicated that not all compilers might
correctly detect that it is non-zero.

But I have no problem to leave that initialization out. (If we leave
it in, we should probably also remove the one `guard = 0;` line.)

> >  	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.

sorry

> >  	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?

I think that the ternary expression together with the other
parenthesized paramenter and the length of the line would make the
line barely readable.

This are some of the pivots lines of the implementation, I'd rather
have them stick out.

Also the assembler that is produced should be identical.

> > +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.

ok, I'll look into that

> Also, since it doesn't depend on anything in pthread_create.c,

It does, __THRD_C11 :)

> 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.

I'd really prefer to keep all the logic of thrd_create together in one
file. All the other additions at this point are tightly bound to be in
the same TU as pthread_create, for all this weak symbol stuff that is
going on with tsd and friends.

Also see that as an incentive to accept patch 8/8 to separate both
implementations more cleanly :)

> > 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 <threads.h>
> > +
> > +/* 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. :)

right, the compiler didn't notice since these are really typedefs to
the same thing.

> > 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 <threads.h>
> > +
> > +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.

Ok, I'll look

> 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 <sys/mman.h>
> > +#include <threads.h>
> > +
> > +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.

No this ain't a duplicate. The cast here is necessary and plain use of
pthread_join would have us interpret an int* as void**, so the
assignment could potentially overwrite the second half of the word res
is pointing to.

I'll have that visually stick out with a comment

Jens


-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]