[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140907142402.GC23797@brightrain.aerifal.cx>
Date: Sun, 7 Sep 2014 10:24:02 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 7/9] add the thrd_xxxxxx functions
On Mon, Sep 01, 2014 at 12:47:26AM +0200, Jens Gustedt wrote:
> The major difficulty with C threads versus pthread is that the thread
> start functions have different signatures. Basing the C threads
> implementation on pthreads therefore needs to do some ugly cast with
> function pointers and function results.
>
> We try to be the least intrusive for the existing pthreads
> implementation, to make sure we don't break anything and also to ease
> maintenance and simultaneous improvement of the code base later on.
>
> Unfortunately this is not completely trivial. Not only are some calling
> conventions different, but also the level of requested detail for error
> returns varies in the two models. Where POSIX maps most possible errors
> of pthread_create to EAGAIN, C threads require the condition of being out
> of memory to be identified.
> ---
> src/internal/pthread_impl.h | 2 ++
> src/sched/thrd_yield.c | 7 +++++++
> src/thread/pthread_create.c | 32 +++++++++++++++++++++-----------
> src/thread/thrd_create.c | 14 ++++++++++++++
> src/thread/thrd_current.c | 11 +++++++++++
> src/thread/thrd_detach.c | 11 +++++++++++
> src/thread/thrd_equal.c | 6 ++++++
> src/thread/thrd_exit.c | 10 ++++++++++
> src/thread/thrd_join.c | 12 ++++++++++++
> src/time/thrd_sleep.c | 14 +++++++-------
> 10 files changed, 101 insertions(+), 18 deletions(-)
> create mode 100644 src/sched/thrd_yield.c
> create mode 100644 src/thread/thrd_create.c
> create mode 100644 src/thread/thrd_current.c
> create mode 100644 src/thread/thrd_detach.c
> create mode 100644 src/thread/thrd_equal.c
> create mode 100644 src/thread/thrd_exit.c
> create mode 100644 src/thread/thrd_join.c
>
> diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
> index 74c62cc..ae6e60b 100644
> --- a/src/internal/pthread_impl.h
> +++ b/src/internal/pthread_impl.h
> @@ -128,4 +128,6 @@ void __restore_sigs(void *);
> #define DEFAULT_STACK_SIZE 81920
> #define DEFAULT_GUARD_SIZE PAGE_SIZE
>
> +#define __ATTRP_C11_THREAD ((void*)(uintptr_t)-1)
> +
I actually had an idea to get rid of this and instead put a
start-wrapper pointer in the attribute structure, so thrd_create could
just pass an attribute with a pointer to a C11 start function in
thrd_create.c. But the savings are sufficiently small that I'm just
sticking with the way we discussed for now, since it's already
working.
> #endif
> diff --git a/src/sched/thrd_yield.c b/src/sched/thrd_yield.c
> new file mode 100644
> index 0000000..09d534b
> --- /dev/null
> +++ b/src/sched/thrd_yield.c
> @@ -0,0 +1,7 @@
> +#include <sched.h>
> +#include "syscall.h"
> +
> +void thrd_yield()
> +{
> + syscall(SYS_sched_yield);
> +}
Using __syscall here to avoid a useless call to __syscall_ret whose
result will be thrown away anyway.
> diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
> index ed265fb..c1feb62 100644
> --- a/src/thread/pthread_create.c
> +++ b/src/thread/pthread_create.c
> @@ -4,10 +4,12 @@
> #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);
> +_Noreturn void __thrd_exit(int);
>
> static void dummy_0()
> {
> @@ -123,6 +125,14 @@ static int start(void *p)
> return 0;
> }
Not visible here, but start was still calling pthread_exit rather than
__pthread_exit. Fixed it. Before release we should do at least one
more check to make sure no other similar mistakes were missed.
> +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;
> +}
I changed this to call __pthread_exit with the right casts to avoid
pulling in an extra TU and needing the extra namespace-safe name for
thrd_exit.
> -static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp, void *(*entry)(void *), void *restrict arg)
> +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 == __ATTRP_C11_THREAD);
> + size_t size, guard = 0;
Removed initialization of guard.
> map = __mmap(0, size, PROT_NONE, MAP_PRIVATE|MAP_ANON, -1, 0);
> - if (map == MAP_FAILED) goto fail;
> + if (map == MAP_FAILED) goto fail_nomem;
> [...]
> @@ -245,7 +255,7 @@ static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restr
> if (ret < 0) {
> a_dec(&libc.threads_minus_1);
> if (map) __munmap(map, size);
> - return EAGAIN;
> + return (!c11 || ret != -ENOMEM) ? EAGAIN : ENOMEM;
> [...]
> -fail:
> +fail_nomem:
> __release_ptc();
> - return EAGAIN;
> + return c11 ? ENOMEM : EAGAIN;
I took these out for now because (1) I still don't see any indication
that the standard imposes a definition on what "no memory could be
allocated" means in the context of creating a thread, or any way it
could, since the types of allocation involved in creating a thread
will be highly implementation-specific, and (2) it's incomplete
anyway, because mmap and mprotect can fail with ENOMEM even in cases
where the cause is not failure to allocate memory, but arbitrary
limitations (much like the arbitrary limit on number of task slots) on
_how_ virtual memory space can be laid out into areas with distinct
permissions.
My feeling is still that returning thrd_nomem is the most useful error
for applications regardless of what specific memory resource (e.g. vm
areas, address space, commit charge, space in process table, etc.) was
unable to be allocated. If there's a definitive statement to the
contrary from the committee or serious objection from users, we can
discuss it further at a later time.
> weak_alias(__pthread_exit, pthread_exit);
> diff --git a/src/thread/thrd_create.c b/src/thread/thrd_create.c
> new file mode 100644
> index 0000000..09c1d9e
> --- /dev/null
> +++ b/src/thread/thrd_create.c
> @@ -0,0 +1,14 @@
> +#include "pthread_impl.h"
> +#include <threads.h>
> +
> +int __pthread_create(pthread_t *restrict, const pthread_attr_t *restrict, void *(*)(void *), void *restrict);
> +
> +int thrd_create(thrd_t *thr, thrd_start_t func, void *arg) {
> + int ret = __pthread_create(thr, __ATTRP_C11_THREAD, (void * (*)(void *))func, arg);
> + switch (ret) {
> + case 0: return thrd_success;
> + case ENOMEM: return thrd_nomem;
> + /* The call may also return ENOSYS or EAGAIN. */
> + default: return thrd_error;
> + }
> +}
Changed this to match.
> diff --git a/src/thread/thrd_current.c b/src/thread/thrd_current.c
> new file mode 100644
> index 0000000..d9dabde
> --- /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. */
> +thrd_t thrd_current()
> +{
> + return __pthread_self();
> +}
Moved this to an alias.
> diff --git a/src/thread/thrd_detach.c b/src/thread/thrd_detach.c
> new file mode 100644
> index 0000000..b0b153c
> --- /dev/null
> +++ b/src/thread/thrd_detach.c
> @@ -0,0 +1,11 @@
> +#include <threads.h>
> +
> +int __pthread_detach(thrd_t t);
> +
> +int thrd_detach(thrd_t t)
> +{
> + /* This internal function never fails, so it always returns
> + * 0. Under the assumption that thrd_success is 0 this is a
> + * tail call. */
> + return __pthread_detach(t);
> +}
Moved this to an alias.
> diff --git a/src/thread/thrd_equal.c b/src/thread/thrd_equal.c
> new file mode 100644
> index 0000000..ac49a44
> --- /dev/null
> +++ b/src/thread/thrd_equal.c
> @@ -0,0 +1,6 @@
> +#include <threads.h>
> +
> +int (thrd_equal)(thrd_t a, thrd_t b)
> +{
> + return a==b;
> +}
And this.
> diff --git a/src/time/thrd_sleep.c b/src/time/thrd_sleep.c
> index 3dbfe47..d8eaafd 100644
> --- a/src/time/thrd_sleep.c
> +++ b/src/time/thrd_sleep.c
> @@ -5,21 +5,21 @@
> #include "threads.h"
>
> /* Roughly __syscall already returns the right thing: 0 if all went
> - well or a negative error indication, otherwise. */
> + * well or a negative error indication, otherwise. */
> int thrd_sleep(const struct timespec *req, struct timespec *rem)
> {
> int ret = __syscall(SYS_nanosleep, req, rem);
> switch (ret) {
> case 0:
> return 0;
> - /* error described by POSIX: */
> - /* EINTR The nanosleep() function was interrupted by a signal. */
> - /* The C11 wording is: */
> - /* -1 if it has been interrupted by a signal */
> + /* error described by POSIX:
> + * EINTR The nanosleep() function was interrupted by a signal.
> + * The C11 wording is:
> + * -1 if it has been interrupted by a signal */
> case -EINTR:
> return -1;
> - /* EINVAL: described by POSIX */
> - /* EFAULT: described for linux */
> + /* EINVAL: described by POSIX
> + * EFAULT: described for linux */
> default:
> return -2;
> }
> --
> 1.7.10.4
This diff didn't apply since I didn't add the original version, but I
think leaving it as two commits like you did was unintended anyway.
I'm applying roughly the new version, but with some fixups to include
directives, and less detail in comments.
BTW looking at the spec again, I see that C11 does not seem to require
the negative return value for other errors to be distinct from -1.
Obviously it's desirable to be distinct though. Perhaps this should be
filed as another bug against C11, if you don't have it on your list
already...
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
