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

[Rich Felker <dalias@libc.org>]

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 <sched.h>
> +#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 <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.

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

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

Rich