working C11 thread implementation

working C11 thread implementation

[Jens Gustedt]

[-- Attachment #1.1: Type: text/plain, Size: 3238 bytes --]

Hi everybody,
for anybody interested I attach a first working version, that is
mildly tested with a relatively big private application that I have
and that uses C11 threads quite intensively.

This implementation is still marked experimental, since it makes some
choices for which we don't know yet if they are final and which impact
binary compatibility.

We discussed several choices with Rich, and the result is a solution
that should only marginally affect existing code. This "marginally" is

  - many pthread interfaces now are weak aliases to static functions
    to avoid name space pollution of the C thread implementation. This
    adds some noise to the symbol table of unstripped
    executables. (but those where this is crucial do strip their
    executables anyhow, don't they ?)

  - pthread_create.o has two additional small wrapper functions that
    augment its size by some byte

  - pthread_create (now __pthread_create) has some instructions more
    that do the selection between the POSIX and C calling conventions

the name space of pthread applications shouldn't be polluted.

This principal choice was guided by the idea to reuse maybe 90% or
more of existing code from pthreads, to minimize the risk of
introducing bugs and to ease maintenance.

The choices that affect the ABI that I mentioned are values of some
constants and the sizes of the types that are introduced. These may
change depending on glibc's choices and also according to future
developments.

For the choices of the constants, for this version they are such that
most wrapper calls result in being tail calls. I verified this by
looking into the assembler that is produced. As they are now, most of
these tail functions could also be just provided as weak aliases. We
chose to implement them as functions such that in case of change of
the constants we only have to recompile and no other maintenance is
required.

For the types, this just uses the pthread types for the moment,
nothing fancy. In the near future I will look into replacing some by
new implementations, to take advantage of the looser requirements for
C threads (no process shared control structures, no robust mutexes, no
cancelation).

The choices here will also depend on the restrictions that
POSIX will impose on compatibility between the two thread models, in
particular if some C thread functions will be considered cancelation
points for pthread.

The idea of all of this is that C threads should give way to a very
compact and comprehensive thread implementation. In this sense I think
for many users of musl this could be a valid alternative to
pthreads.

Also one should have in mind that C11 allows the use of its threads by
the library itself, as long as it follows the as-if rule. So a C
library could provide a parallel implementation of qsort or similar
compute intensive functions.

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 #1.2: thrd11.patch --]
[-- Type: text/x-patch, Size: 34802 bytes --]

diff --git a/include/alltypes.h.in b/include/alltypes.h.in
index 8478fb4..4cd2cb1 100644
--- a/include/alltypes.h.in
+++ b/include/alltypes.h.in
@@ -58,6 +58,10 @@ TYPEDEF struct { unsigned __attr; } pthread_mutexattr_t;
 TYPEDEF struct { unsigned __attr; } pthread_condattr_t;
 TYPEDEF struct { unsigned __attr; } pthread_barrierattr_t;
 TYPEDEF struct { unsigned __attr[2]; } pthread_rwlockattr_t;
+TYPEDEF pthread_cond_t cnd_t;
+TYPEDEF pthread_mutex_t mtx_t;
+
+
 
 TYPEDEF struct _IO_FILE FILE;
 
diff --git a/include/threads.h b/include/threads.h
new file mode 100644
index 0000000..e131781
--- /dev/null
+++ b/include/threads.h
@@ -0,0 +1,169 @@
+#ifndef _THREADS_H
+#define _THREADS_H
+
+/* This one is explicitly allowed to be included. */
+#include <time.h>
+
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define __NEED_cnd_t
+#define __NEED_mtx_t
+/* Until we come up with a better naming scheme, we need to expose
+   some pthread types. */
+#define __NEED_pthread_cond_t
+#define __NEED_pthread_mutex_t
+#define __NEED_pthread_once_t
+#define __NEED_pthread_t
+#define __NEED_pthread_key_t
+
+#include <bits/alltypes.h>
+#include <bits/errno.h>
+
+typedef pthread_once_t once_flag;
+typedef pthread_t thrd_t;
+typedef pthread_key_t tss_t;
+typedef int (*thrd_start_t)(void*);
+typedef void (*tss_dtor_t)(void*);
+
+
+#ifndef __THRD_EXPERIMENTAL
+# define __THRD_EXPERIMENTAL 1
+#endif
+
+  /* The following list of 9 integer constants makes up for the binary
+     compatibility of this C thread implementation. You must never
+     link code against versions of the C library that do not agree
+     upon these ABI parameters.
+
+     Additionally this implementation assumes that the 5 types have
+     the same size across C libraries and that these types can be
+     initialized by the default initializer.
+
+     The values for the 9 parameters are not fixed for now. Depending
+     on the choices of other implementations and the evolution of the
+     C standard they may still change. This should happen rarely, but
+     you have to consider the C thread feature to be experimental
+     until then, and be prepared to recompile your binary when linking
+     against a different implementation or a new version.
+
+     The macro __THRD_EXPERIMENTAL will be defined as long as we
+     consider this ABI to be unstable. This comes with some link time
+     checks and an overhead of some bytes. At your own risk you may
+     switch this check off by defining __THRD_EXPERIMENTAL macro to
+     0. */
+
+#define __THRD_SUCCESS      0
+#define __THRD_BUSY         EBUSY
+#define __THRD_ERROR        EINVAL
+#define __THRD_NOMEM        ENOMEM
+#define __THRD_TIMEDOUT     ETIMEDOUT
+#define __MTX_PLAIN         0
+#define __MTX_RECURSIVE     1
+#define __MTX_TIMED         2
+#define TSS_DTOR_ITERATIONS 4
+
+#define __THRD_CONCAT10_(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9)         \
+_0 ## _ ## _1 ## _ ## _2 ## _ ## _3 ## _ ## _4 ## _ ## _5 ## _ ## _6 ## _ ## _7 ## _ ## _8 ## _ ## _9
+
+#define __THRD_CONCAT10(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9) \
+  __THRD_CONCAT10_(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9)
+
+
+#define __THRD_ABI                              \
+__THRD_CONCAT10(__thrd_abi,                     \
+                __THRD_SUCCESS,                 \
+                __THRD_BUSY,                    \
+                __THRD_ERROR,                   \
+                __THRD_NOMEM,                   \
+                __THRD_TIMEDOUT,                \
+                __MTX_PLAIN,                    \
+                __MTX_RECURSIVE,                \
+                __MTX_TIMED,                    \
+                TSS_DTOR_ITERATIONS             \
+                )
+
+#define __THRD_SHFT(X, Y) (((X) << 3) ^ (Y))
+
+enum {
+  __thrd_abi =
+  __THRD_SHFT(sizeof(cnd_t),
+              __THRD_SHFT(sizeof(mtx_t),
+                          __THRD_SHFT(sizeof(once_flag),
+                                      __THRD_SHFT(sizeof(thrd_t),
+                                                  sizeof(tss_t))))),
+};
+
+extern unsigned const __THRD_ABI;
+
+#define __THRD_ABI_CHECK (1/(__THRD_ABI == __thrd_abi))
+
+#if __THRD_EXPERIMENTAL
+# define __THRD_ABI_MARK __attribute__((used)) static unsigned const*const __thrd_abi_mark = &__THRD_ABI
+#else
+# define __THRD_ABI_MARK typedef unsigned __thrd_abi_dummy_type
+#endif
+
+enum {
+  thrd_success = __THRD_SUCCESS,
+  thrd_busy = __THRD_BUSY,
+  thrd_error = __THRD_ERROR,
+  thrd_nomem = __THRD_NOMEM,
+  thrd_timedout = __THRD_TIMEDOUT,
+};
+
+enum {
+  mtx_plain = __MTX_PLAIN,
+  mtx_recursive = __MTX_RECURSIVE,
+  // all mutexes are timed, here. so this is a no-op
+  mtx_timed = __MTX_TIMED,
+};
+
+#define ONCE_FLAG_INIT { 0 }
+#define thread_local _Thread_local
+
+int thrd_create(thrd_t *, thrd_start_t, void *);
+_Noreturn void thrd_exit(int);
+
+int thrd_detach(thrd_t);
+int thrd_join(thrd_t, int *);
+
+int thrd_sleep(const struct timespec *, struct timespec *);
+void thrd_yield(void);
+
+thrd_t thrd_current(void);
+int thrd_equal(thrd_t, thrd_t);
+#define thrd_equal(A, B) ((A) == (B))
+
+void call_once(once_flag *, void (*)(void));
+
+int mtx_init(mtx_t *, int);
+void mtx_destroy(mtx_t *);
+
+int mtx_lock(mtx_t *);
+int mtx_timedlock(mtx_t *restrict, const struct timespec *restrict);
+int mtx_trylock(mtx_t *);
+int mtx_unlock(mtx_t *);
+
+int cnd_init(cnd_t *);
+void cnd_destroy(cnd_t *);
+
+int cnd_broadcast(cnd_t *);
+int cnd_signal(cnd_t *);
+
+int cnd_timedwait(cnd_t *restrict, mtx_t *restrict, const struct timespec *restrict);
+int cnd_wait(cnd_t *, mtx_t *);
+
+int tss_create(tss_t *, tss_dtor_t);
+void tss_delete(tss_t key);
+
+int tss_set(tss_t, void *);
+void *tss_get(tss_t);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
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);
+}
diff --git a/src/thread/__thrd_abi.c b/src/thread/__thrd_abi.c
new file mode 100644
index 0000000..e5674e6
--- /dev/null
+++ b/src/thread/__thrd_abi.c
@@ -0,0 +1,3 @@
+#include <threads.h>
+
+unsigned const __THRD_ABI =  __thrd_abi;
diff --git a/src/thread/__timedwait.c b/src/thread/__timedwait.c
index 302273a..168007a 100644
--- a/src/thread/__timedwait.c
+++ b/src/thread/__timedwait.c
@@ -4,6 +4,8 @@
 #include "futex.h"
 #include "syscall.h"
 
+int __pthread_setcancelstate(int new, int *old);
+
 static int do_wait(volatile int *addr, int val,
 	clockid_t clk, const struct timespec *at, int priv)
 {
@@ -33,13 +35,13 @@ int __timedwait(volatile int *addr, int val,
 {
 	int r, cs;
 
-	if (!cleanup) pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+	if (!cleanup) __pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 	pthread_cleanup_push(cleanup, arg);
 
 	r = do_wait(addr, val, clk, at, priv);
 
 	pthread_cleanup_pop(0);
-	if (!cleanup) pthread_setcancelstate(cs, 0);
+	if (!cleanup) __pthread_setcancelstate(cs, 0);
 
 	return r;
 }
diff --git a/src/thread/call_once.c b/src/thread/call_once.c
new file mode 100644
index 0000000..742a707
--- /dev/null
+++ b/src/thread/call_once.c
@@ -0,0 +1,9 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_once(once_flag *, void (*)(void));
+
+void (call_once)(once_flag *flag, void (*func)(void)) {
+	__THRD_ABI_MARK;
+	(void)__pthread_once(flag, func);
+}
diff --git a/src/thread/cnd_broadcast.c b/src/thread/cnd_broadcast.c
new file mode 100644
index 0000000..945018f
--- /dev/null
+++ b/src/thread/cnd_broadcast.c
@@ -0,0 +1,12 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_cond_broadcast(pthread_cond_t *);
+
+int cnd_broadcast(cnd_t * cnd) {
+	/* In the best of all worlds this is a tail call. */
+	int ret = __pthread_cond_broadcast(cnd);
+	if (thrd_success)
+		return ret ? thrd_error : thrd_success;
+	return ret;
+}
diff --git a/src/thread/cnd_destroy.c b/src/thread/cnd_destroy.c
new file mode 100644
index 0000000..11cfc19
--- /dev/null
+++ b/src/thread/cnd_destroy.c
@@ -0,0 +1,17 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_cond_destroy(pthread_cond_t *);
+
+/* The behavior of cnd_destroy is undefined if cnd is still in
+   use. The choice for pthread_cond_destroy in that situation is to
+   wake up all users before destroying. I am not sure that we should
+   do it like that here, too. Alternatives would be:
+   - complain by using perror or equivalent
+   - assert that there is no waiter
+   - abort when there is a waiter
+   - do nothing
+   */
+void (cnd_destroy)(cnd_t *cnd) {
+	(void)__pthread_cond_destroy(cnd);
+}
diff --git a/src/thread/cnd_init.c b/src/thread/cnd_init.c
new file mode 100644
index 0000000..c7f1edf
--- /dev/null
+++ b/src/thread/cnd_init.c
@@ -0,0 +1,10 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int cnd_init(cnd_t * c)
+{
+	*c = (cnd_t) {
+		0
+	};
+	return thrd_success;
+}
diff --git a/src/thread/cnd_signal.c b/src/thread/cnd_signal.c
new file mode 100644
index 0000000..10241c5
--- /dev/null
+++ b/src/thread/cnd_signal.c
@@ -0,0 +1,12 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_cond_signal(pthread_cond_t *);
+
+int cnd_signal(cnd_t * cnd) {
+	/* In the best of all worlds this is a tail call. */
+	int ret = __pthread_cond_signal(cnd);
+	if (thrd_success)
+		return ret ? thrd_error : thrd_success;
+	return ret;
+}
diff --git a/src/thread/cnd_timedwait.c b/src/thread/cnd_timedwait.c
new file mode 100644
index 0000000..9397706
--- /dev/null
+++ b/src/thread/cnd_timedwait.c
@@ -0,0 +1,22 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m, const struct timespec *restrict ts);
+
+int cnd_timedwait(cnd_t *restrict cond, mtx_t *restrict mutex, const struct timespec *restrict ts) {
+	/* In the best of all worlds this is a tail call. */
+	int ret = __pthread_cond_timedwait(cond, mutex, ts);
+	switch (ret) {
+	case 0:
+		if (thrd_success == 0) break;
+		else return thrd_success;
+	case ETIMEDOUT:
+		if (thrd_timedout == ETIMEDOUT) break;
+		else return thrd_timedout;
+	case EINVAL:
+		if (thrd_error == EINVAL) break;
+		else return thrd_error;
+	}
+	/* In case of UB may also return EPERM. */
+	return ret;
+}
diff --git a/src/thread/cnd_wait.c b/src/thread/cnd_wait.c
new file mode 100644
index 0000000..39d0a0d
--- /dev/null
+++ b/src/thread/cnd_wait.c
@@ -0,0 +1,11 @@
+#include <threads.h>
+
+int cnd_wait(cnd_t *cond, mtx_t *mutex)
+{
+	__THRD_ABI_MARK;
+	/* Calling cnd_timedwait with a null pointer is an
+	   extension. Such a call is convenient, here since it avoids to
+	   repeat the case analysis that is already done for
+	   cnd_timedwait. */
+	return cnd_timedwait(cond, mutex, 0);
+}
diff --git a/src/thread/mtx_destroy.c b/src/thread/mtx_destroy.c
new file mode 100644
index 0000000..482b781
--- /dev/null
+++ b/src/thread/mtx_destroy.c
@@ -0,0 +1,5 @@
+#include <threads.h>
+
+void (mtx_destroy)(mtx_t *mtx) {
+	/* empty */
+}
diff --git a/src/thread/mtx_init.c b/src/thread/mtx_init.c
new file mode 100644
index 0000000..dcee8c8
--- /dev/null
+++ b/src/thread/mtx_init.c
@@ -0,0 +1,10 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int mtx_init(mtx_t * m, int type)
+{
+	*m = (pthread_mutex_t) {
+		._m_type = ((type&mtx_recursive) ? PTHREAD_MUTEX_RECURSIVE : 0),
+	};
+	return 0;
+}
diff --git a/src/thread/mtx_lock.c b/src/thread/mtx_lock.c
new file mode 100644
index 0000000..89730f1
--- /dev/null
+++ b/src/thread/mtx_lock.c
@@ -0,0 +1,14 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int mtx_lock(mtx_t *m)
+{
+	__THRD_ABI_MARK;
+	if (m->_m_type == PTHREAD_MUTEX_NORMAL && !a_cas(&m->_m_lock, 0, EBUSY))
+		return thrd_success;
+	/* Calling mtx_timedlock with a null pointer is an
+	   extension. Such a call is convenient, here, since it avoids
+	   to repeat the case analysis that is already done for
+	   mtx_timedlock. */
+	return mtx_timedlock(m, 0);
+}
diff --git a/src/thread/mtx_timedlock.c b/src/thread/mtx_timedlock.c
new file mode 100644
index 0000000..1e3fd37
--- /dev/null
+++ b/src/thread/mtx_timedlock.c
@@ -0,0 +1,26 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec *restrict ts);
+
+int mtx_timedlock(mtx_t *restrict mutex, const struct timespec *restrict ts) {
+	/* In the best of all worlds this is a tail call. */
+	int ret = __pthread_mutex_timedlock(mutex, ts);
+	switch (ret) {
+	case 0:
+		if (thrd_success == 0) break;
+		else return thrd_success;
+	case ETIMEDOUT:
+		if (thrd_timedout == ETIMEDOUT) break;
+		else return thrd_timedout;
+	case EINVAL:
+		if (thrd_error == EINVAL) break;
+		else return thrd_error;
+	}
+	/* In case of UB may also return EPERM, EDEADLK or EAGAIN. EAGAIN is
+	   specially tricky since C11 doesn't define how many recursive
+	   calls can be done. (this *isn't* the maximum amount of nested
+	   calls!) This implementation here deals this with a counter and
+	   detects overflow, so this is definitively UB. */
+	return ret;
+}
diff --git a/src/thread/mtx_trylock.c b/src/thread/mtx_trylock.c
new file mode 100644
index 0000000..4b01bf7
--- /dev/null
+++ b/src/thread/mtx_trylock.c
@@ -0,0 +1,29 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_mutex_trylock(pthread_mutex_t *restrict m);
+
+int mtx_trylock(mtx_t *restrict m) {
+	if (m->_m_type == PTHREAD_MUTEX_NORMAL)
+		return a_cas(&m->_m_lock, 0, EBUSY) & EBUSY;
+
+	/* In the best of all worlds this is a tail call. */
+	int ret = __pthread_mutex_trylock(m);
+	switch (ret) {
+	case 0:
+		if (thrd_success == 0) break;
+		else return thrd_success;
+	case EBUSY:
+		if (thrd_busy == EBUSY) break;
+		else return thrd_busy;
+	case EINVAL:
+		if (thrd_error == EINVAL) break;
+		else return thrd_error;
+	}
+	/* In case of UB may also return EPERM, EDEADLK or EAGAIN. EAGAIN is
+	   specially tricky since C11 doesn't define how many recursive
+	   calls can be done. (this *isn't* the maximum amount of nested
+	   calls!) This implementation here deals this with a counter and
+	   detects overflow, so this is definitively UB. */
+	return ret;
+}
diff --git a/src/thread/mtx_unlock.c b/src/thread/mtx_unlock.c
new file mode 100644
index 0000000..d774b81
--- /dev/null
+++ b/src/thread/mtx_unlock.c
@@ -0,0 +1,13 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_mutex_unlock(pthread_mutex_t *);
+
+int (mtx_unlock)(mtx_t *mtx) {
+	/* In the best of all worlds this is a tail call. */
+	int ret = __pthread_mutex_unlock(mtx);
+	if (thrd_success)
+		return ret ? thrd_error : thrd_success;
+	/* In case of UB may also return EPERM. */
+	return ret;
+}
diff --git a/src/thread/pthread_cond_broadcast.c b/src/thread/pthread_cond_broadcast.c
index 0901daf..7a9cfd9 100644
--- a/src/thread/pthread_cond_broadcast.c
+++ b/src/thread/pthread_cond_broadcast.c
@@ -1,6 +1,6 @@
 #include "pthread_impl.h"
 
-int pthread_cond_broadcast(pthread_cond_t *c)
+int __pthread_cond_broadcast(pthread_cond_t *c)
 {
 	pthread_mutex_t *m;
 
@@ -37,3 +37,5 @@ out:
 
 	return 0;
 }
+
+weak_alias(__pthread_cond_broadcast, pthread_cond_broadcast);
diff --git a/src/thread/pthread_cond_destroy.c b/src/thread/pthread_cond_destroy.c
index a096c55..5d99e88 100644
--- a/src/thread/pthread_cond_destroy.c
+++ b/src/thread/pthread_cond_destroy.c
@@ -1,6 +1,6 @@
 #include "pthread_impl.h"
 
-int pthread_cond_destroy(pthread_cond_t *c)
+int __pthread_cond_destroy(pthread_cond_t *c)
 {
 	int priv = c->_c_mutex != (void *)-1;
 	int cnt;
@@ -11,3 +11,5 @@ int pthread_cond_destroy(pthread_cond_t *c)
 		__wait(&c->_c_waiters, 0, cnt, priv);
 	return 0;
 }
+
+weak_alias(__pthread_cond_destroy, pthread_cond_destroy);
diff --git a/src/thread/pthread_cond_signal.c b/src/thread/pthread_cond_signal.c
index 71bcdcd..38754f2 100644
--- a/src/thread/pthread_cond_signal.c
+++ b/src/thread/pthread_cond_signal.c
@@ -1,9 +1,11 @@
 #include "pthread_impl.h"
 
-int pthread_cond_signal(pthread_cond_t *c)
+int __pthread_cond_signal(pthread_cond_t *c)
 {
 	if (!c->_c_waiters) return 0;
 	a_inc(&c->_c_seq);
 	if (c->_c_waiters) __wake(&c->_c_seq, 1, 0);
 	return 0;
 }
+
+weak_alias(__pthread_cond_signal, pthread_cond_signal);
diff --git a/src/thread/pthread_cond_timedwait.c b/src/thread/pthread_cond_timedwait.c
index 99d62cc..819a7af 100644
--- a/src/thread/pthread_cond_timedwait.c
+++ b/src/thread/pthread_cond_timedwait.c
@@ -1,5 +1,9 @@
 #include "pthread_impl.h"
 
+void __pthread_testcancel(void);
+int __pthread_mutex_lock(pthread_mutex_t *);
+int __pthread_mutex_unlock(pthread_mutex_t *m);
+
 struct cm {
 	pthread_cond_t *c;
 	pthread_mutex_t *m;
@@ -33,10 +37,10 @@ static void cleanup(void *p)
 {
 	struct cm *cm = p;
 	unwait(cm->c, cm->m);
-	pthread_mutex_lock(cm->m);
+	__pthread_mutex_lock(cm->m);
 }
 
-int pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m, const struct timespec *restrict ts)
+int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m, const struct timespec *restrict ts)
 {
 	struct cm cm = { .c=c, .m=m };
 	int r, e=0, seq;
@@ -47,7 +51,7 @@ int pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict
 	if (ts && ts->tv_nsec >= 1000000000UL)
 		return EINVAL;
 
-	pthread_testcancel();
+	__pthread_testcancel();
 
 	a_inc(&c->_c_waiters);
 
@@ -62,7 +66,7 @@ int pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict
 
 	seq = c->_c_seq;
 
-	pthread_mutex_unlock(m);
+	__pthread_mutex_unlock(m);
 
 	do e = __timedwait(&c->_c_seq, seq, c->_c_clock, ts, cleanup, &cm, 0);
 	while (c->_c_seq == seq && (!e || e==EINTR));
@@ -70,7 +74,9 @@ int pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict
 
 	unwait(c, m);
 
-	if ((r=pthread_mutex_lock(m))) return r;
+	if ((r=__pthread_mutex_lock(m))) return r;
 
 	return e;
 }
+
+weak_alias(__pthread_cond_timedwait, pthread_cond_timedwait);
diff --git a/src/thread/pthread_cond_wait.c b/src/thread/pthread_cond_wait.c
index 8735bf1..58656f7 100644
--- a/src/thread/pthread_cond_wait.c
+++ b/src/thread/pthread_cond_wait.c
@@ -1,6 +1,8 @@
 #include "pthread_impl.h"
 
+int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m, const struct timespec *restrict ts);
+
 int pthread_cond_wait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m)
 {
-	return pthread_cond_timedwait(c, m, 0);
+	return __pthread_cond_timedwait(c, m, 0);
 }
diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index e77e54a..0b24a1c 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -3,6 +3,9 @@
 #include "stdio_impl.h"
 #include "libc.h"
 #include <sys/mman.h>
+#include <threads.h>
+
+#define __THRD_C11 ((void*)(uintptr_t)-1)
 
 static void dummy_0()
 {
@@ -11,7 +14,7 @@ weak_alias(dummy_0, __acquire_ptc);
 weak_alias(dummy_0, __release_ptc);
 weak_alias(dummy_0, __pthread_tsd_run_dtors);
 
-_Noreturn void pthread_exit(void *result)
+_Noreturn void __pthread_exit(void *result)
 {
 	pthread_t self = __pthread_self();
 	sigset_t set;
@@ -113,6 +116,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 */
@@ -133,10 +149,10 @@ static void init_file_lock(FILE *f)
 
 void *__copy_tls(unsigned char *);
 
-int pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp, void *(*entry)(void *), void *restrict arg)
+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;
 	struct pthread *self, *new;
 	unsigned char *map = 0, *stack = 0, *tsd = 0, *stack_limit;
 	unsigned flags = CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND
@@ -157,6 +173,9 @@ int pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp
 		self->tsd = (void **)__pthread_tsd_main;
 		libc.threaded = 1;
 	}
+        if (c11) {
+          attrp = 0;
+        }
 	if (attrp) attr = *attrp;
 
 	__acquire_ptc();
@@ -223,7 +242,10 @@ int pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp
 	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);
 
 	__release_ptc();
 
@@ -251,3 +273,19 @@ fail:
 	__release_ptc();
 	return EAGAIN;
 }
+
+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;
+}
+
+weak_alias(__pthread_exit, pthread_exit);
+weak_alias(__pthread_create, pthread_create);
+weak_alias(__thrd_create, thrd_create);
+weak_alias(__thrd_exit, thrd_exit);
diff --git a/src/thread/pthread_detach.c b/src/thread/pthread_detach.c
index 651c38e..5ca7855 100644
--- a/src/thread/pthread_detach.c
+++ b/src/thread/pthread_detach.c
@@ -1,11 +1,15 @@
 #include "pthread_impl.h"
 
-int pthread_detach(pthread_t t)
+int __pthread_join(pthread_t t, void **res);
+
+int __pthread_detach(pthread_t t)
 {
 	/* Cannot detach a thread that's already exiting */
 	if (a_swap(t->exitlock, 1))
-		return pthread_join(t, 0);
+		return __pthread_join(t, 0);
 	t->detached = 2;
 	__unlock(t->exitlock);
 	return 0;
 }
+
+weak_alias(__pthread_detach, pthread_detach);
diff --git a/src/thread/pthread_equal.c b/src/thread/pthread_equal.c
index 3e3df4f..38fb45e 100644
--- a/src/thread/pthread_equal.c
+++ b/src/thread/pthread_equal.c
@@ -1,4 +1,4 @@
-#include <pthread.h>
+#include "pthread_impl.h"
 
 int (pthread_equal)(pthread_t a, pthread_t b)
 {
diff --git a/src/thread/pthread_getspecific.c b/src/thread/pthread_getspecific.c
index b2a282c..bfc4294 100644
--- a/src/thread/pthread_getspecific.c
+++ b/src/thread/pthread_getspecific.c
@@ -1,7 +1,10 @@
 #include "pthread_impl.h"
 
-void *pthread_getspecific(pthread_key_t k)
+static void *__pthread_getspecific(pthread_key_t k)
 {
 	struct pthread *self = __pthread_self();
 	return self->tsd[k];
 }
+
+weak_alias(__pthread_getspecific, pthread_getspecific);
+weak_alias(__pthread_getspecific, tss_get);
diff --git a/src/thread/pthread_join.c b/src/thread/pthread_join.c
index 719c91c..1b59489 100644
--- a/src/thread/pthread_join.c
+++ b/src/thread/pthread_join.c
@@ -5,7 +5,7 @@ static void dummy(void *p)
 {
 }
 
-int pthread_join(pthread_t t, void **res)
+int __pthread_join(pthread_t t, void **res)
 {
 	int tmp;
 	while ((tmp = t->tid)) __timedwait(&t->tid, tmp, 0, 0, dummy, 0, 0);
@@ -13,3 +13,5 @@ int pthread_join(pthread_t t, void **res)
 	if (t->map_base) munmap(t->map_base, t->map_size);
 	return 0;
 }
+
+weak_alias(__pthread_join, pthread_join);
diff --git a/src/thread/pthread_key_create.c b/src/thread/pthread_key_create.c
index a9187f7..bfcd597 100644
--- a/src/thread/pthread_key_create.c
+++ b/src/thread/pthread_key_create.c
@@ -9,7 +9,7 @@ static void nodtor(void *dummy)
 {
 }
 
-int pthread_key_create(pthread_key_t *k, void (*dtor)(void *))
+int __pthread_key_create(pthread_key_t *k, void (*dtor)(void *))
 {
 	unsigned i = (uintptr_t)&k / 16 % PTHREAD_KEYS_MAX;
 	unsigned j = i;
@@ -31,7 +31,7 @@ int pthread_key_create(pthread_key_t *k, void (*dtor)(void *))
 	return EAGAIN;
 }
 
-int pthread_key_delete(pthread_key_t k)
+int __pthread_key_delete(pthread_key_t k)
 {
 	keys[k] = 0;
 	return 0;
@@ -53,3 +53,6 @@ void __pthread_tsd_run_dtors()
 		}
 	}
 }
+
+weak_alias(__pthread_key_delete, pthread_key_delete);
+weak_alias(__pthread_key_create, pthread_key_create);
diff --git a/src/thread/pthread_mutex_lock.c b/src/thread/pthread_mutex_lock.c
index 42b5af6..9ea4a97 100644
--- a/src/thread/pthread_mutex_lock.c
+++ b/src/thread/pthread_mutex_lock.c
@@ -1,9 +1,13 @@
 #include "pthread_impl.h"
 
-int pthread_mutex_lock(pthread_mutex_t *m)
+int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec *restrict at);
+
+int __pthread_mutex_lock(pthread_mutex_t *m)
 {
 	if (m->_m_type == PTHREAD_MUTEX_NORMAL && !a_cas(&m->_m_lock, 0, EBUSY))
 		return 0;
 
-	return pthread_mutex_timedlock(m, 0);
+	return __pthread_mutex_timedlock(m, 0);
 }
+
+weak_alias(__pthread_mutex_lock, pthread_mutex_lock);
diff --git a/src/thread/pthread_mutex_timedlock.c b/src/thread/pthread_mutex_timedlock.c
index 7b1afc0..8238e70 100644
--- a/src/thread/pthread_mutex_timedlock.c
+++ b/src/thread/pthread_mutex_timedlock.c
@@ -1,6 +1,6 @@
 #include "pthread_impl.h"
 
-int pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec *restrict at)
+int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec *restrict at)
 {
 	int r, t;
 
@@ -22,3 +22,5 @@ int pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec *
 	}
 	return r;
 }
+
+weak_alias(__pthread_mutex_timedlock, pthread_mutex_timedlock);
diff --git a/src/thread/pthread_mutex_trylock.c b/src/thread/pthread_mutex_trylock.c
index 00ad65d..8085bc6 100644
--- a/src/thread/pthread_mutex_trylock.c
+++ b/src/thread/pthread_mutex_trylock.c
@@ -1,6 +1,6 @@
 #include "pthread_impl.h"
 
-int pthread_mutex_trylock(pthread_mutex_t *m)
+int __pthread_mutex_trylock(pthread_mutex_t *m)
 {
 	int tid, old, own;
 	pthread_t self;
@@ -50,3 +50,5 @@ int pthread_mutex_trylock(pthread_mutex_t *m)
 
 	return 0;
 }
+
+weak_alias(__pthread_mutex_trylock, pthread_mutex_trylock);
diff --git a/src/thread/pthread_mutex_unlock.c b/src/thread/pthread_mutex_unlock.c
index b4bd74b..3f6ce4a 100644
--- a/src/thread/pthread_mutex_unlock.c
+++ b/src/thread/pthread_mutex_unlock.c
@@ -3,7 +3,7 @@
 void __vm_lock_impl(int);
 void __vm_unlock_impl(void);
 
-int pthread_mutex_unlock(pthread_mutex_t *m)
+int __pthread_mutex_unlock(pthread_mutex_t *m)
 {
 	pthread_t self;
 	int waiters = m->_m_waiters;
@@ -35,3 +35,5 @@ int pthread_mutex_unlock(pthread_mutex_t *m)
 		__wake(&m->_m_lock, 1, 0);
 	return 0;
 }
+
+weak_alias(__pthread_mutex_unlock, pthread_mutex_unlock);
diff --git a/src/thread/pthread_once.c b/src/thread/pthread_once.c
index e01f6d4..a678597 100644
--- a/src/thread/pthread_once.c
+++ b/src/thread/pthread_once.c
@@ -6,7 +6,7 @@ static void undo(void *control)
 	__wake(control, 1, 0);
 }
 
-int pthread_once(pthread_once_t *control, void (*init)(void))
+int __pthread_once(pthread_once_t *control, void (*init)(void))
 {
 	static int waiters;
 
@@ -34,3 +34,5 @@ int pthread_once(pthread_once_t *control, void (*init)(void))
 		return 0;
 	}
 }
+
+weak_alias(__pthread_once, pthread_once);
diff --git a/src/thread/pthread_self.c b/src/thread/pthread_self.c
index 5f9e651..04f3e87 100644
--- a/src/thread/pthread_self.c
+++ b/src/thread/pthread_self.c
@@ -1,5 +1,7 @@
 #include "pthread_impl.h"
 
+/* This is needed because not all arch have __pthread_self as a static
+   symbol. */
 pthread_t pthread_self()
 {
 	return __pthread_self();
diff --git a/src/thread/pthread_setcancelstate.c b/src/thread/pthread_setcancelstate.c
index 060bcdc..2268217 100644
--- a/src/thread/pthread_setcancelstate.c
+++ b/src/thread/pthread_setcancelstate.c
@@ -1,6 +1,6 @@
 #include "pthread_impl.h"
 
-int pthread_setcancelstate(int new, int *old)
+int __pthread_setcancelstate(int new, int *old)
 {
 	if (new > 1U) return EINVAL;
 	if (!libc.has_thread_pointer) return ENOSYS;
@@ -9,3 +9,5 @@ int pthread_setcancelstate(int new, int *old)
 	self->canceldisable = new;
 	return 0;
 }
+
+weak_alias(__pthread_setcancelstate, pthread_setcancelstate);
diff --git a/src/thread/pthread_setcanceltype.c b/src/thread/pthread_setcanceltype.c
index bf0a3f3..d493c1b 100644
--- a/src/thread/pthread_setcanceltype.c
+++ b/src/thread/pthread_setcanceltype.c
@@ -1,11 +1,13 @@
 #include "pthread_impl.h"
 
+void __pthread_testcancel(void);
+
 int pthread_setcanceltype(int new, int *old)
 {
 	struct pthread *self = __pthread_self();
 	if (new > 1U) return EINVAL;
 	if (old) *old = self->cancelasync;
 	self->cancelasync = new;
-	if (new) pthread_testcancel();
+	if (new) __pthread_testcancel();
 	return 0;
 }
diff --git a/src/thread/pthread_setspecific.c b/src/thread/pthread_setspecific.c
index 55e46a8..dd18b96 100644
--- a/src/thread/pthread_setspecific.c
+++ b/src/thread/pthread_setspecific.c
@@ -1,6 +1,6 @@
 #include "pthread_impl.h"
 
-int pthread_setspecific(pthread_key_t k, const void *x)
+int __pthread_setspecific(pthread_key_t k, const void *x)
 {
 	struct pthread *self = __pthread_self();
 	/* Avoid unnecessary COW */
@@ -10,3 +10,5 @@ int pthread_setspecific(pthread_key_t k, const void *x)
 	}
 	return 0;
 }
+
+weak_alias(__pthread_setspecific, pthread_setspecific);
diff --git a/src/thread/pthread_testcancel.c b/src/thread/pthread_testcancel.c
index ba5f7c6..ee48e6d 100644
--- a/src/thread/pthread_testcancel.c
+++ b/src/thread/pthread_testcancel.c
@@ -7,7 +7,9 @@ static void dummy()
 
 weak_alias(dummy, __testcancel);
 
-void pthread_testcancel()
+void __pthread_testcancel()
 {
 	__testcancel();
 }
+
+weak_alias(__pthread_testcancel, pthread_testcancel);
diff --git a/src/thread/thrd_current.c b/src/thread/thrd_current.c
new file mode 100644
index 0000000..39f8b89
--- /dev/null
+++ b/src/thread/thrd_current.c
@@ -0,0 +1,9 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+/* This is needed because not all arch have __pthread_self as a static
+   symbol. */
+pthread_t thrd_current()
+{
+	return __pthread_self();
+}
diff --git a/src/thread/thrd_detach.c b/src/thread/thrd_detach.c
new file mode 100644
index 0000000..12dff14
--- /dev/null
+++ b/src/thread/thrd_detach.c
@@ -0,0 +1,13 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_detach(pthread_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;
+}
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;
+}
diff --git a/src/thread/thrd_join.c b/src/thread/thrd_join.c
new file mode 100644
index 0000000..8d69839
--- /dev/null
+++ b/src/thread/thrd_join.c
@@ -0,0 +1,14 @@
+#include "pthread_impl.h"
+#include <sys/mman.h>
+#include <threads.h>
+
+/* 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 0;
+}
diff --git a/src/thread/tss_create.c b/src/thread/tss_create.c
new file mode 100644
index 0000000..0cbadc8
--- /dev/null
+++ b/src/thread/tss_create.c
@@ -0,0 +1,13 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_key_create(pthread_key_t *k, void (*dtor)(void *));
+
+int tss_create(tss_t *tss, tss_dtor_t dtor)
+{
+	__THRD_ABI_MARK;
+	/* Different error returns are possible. C glues them together into
+	   just failure notification. Can't be optimized to a tail call,
+	   unless thrd_error equals EAGAIN. */
+	return __pthread_key_create(tss, dtor) ? thrd_error : thrd_success;
+}
diff --git a/src/thread/tss_delete.c b/src/thread/tss_delete.c
new file mode 100644
index 0000000..d50731f
--- /dev/null
+++ b/src/thread/tss_delete.c
@@ -0,0 +1,8 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int __pthread_key_delete(pthread_key_t k);
+
+void (tss_delete)(tss_t key) {
+	(void)__pthread_key_delete(key);
+}
diff --git a/src/thread/tss_set.c b/src/thread/tss_set.c
new file mode 100644
index 0000000..7e4659c
--- /dev/null
+++ b/src/thread/tss_set.c
@@ -0,0 +1,13 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int tss_set(tss_t k, void *x)
+{
+	struct pthread *self = __pthread_self();
+	/* Avoid unnecessary COW */
+	if (self->tsd[k] != x) {
+		self->tsd[k] = x;
+		self->tsd_used = 1;
+	}
+	return 0;
+}
diff --git a/src/time/thrd_sleep.c b/src/time/thrd_sleep.c
new file mode 100644
index 0000000..c595032
--- /dev/null
+++ b/src/time/thrd_sleep.c
@@ -0,0 +1,23 @@
+#include <time.h>
+#include <errno.h>
+#include "syscall.h"
+#include "libc.h"
+#include "threads.h"
+
+/* Roughly __syscall already returns the right thing: 0 if all went
+   well or a negative error indication, otherwise. Unfortunately the C
+   standard foresees the special case of an interrupted call and fixes
+   that error return to -1 (instead of introducing EINTR). */
+int thrd_sleep(const struct timespec *req, struct timespec *rem)
+{
+  int ret = __syscall(SYS_nanosleep, req, rem);
+  // compile time comparison, constant propagated
+  if (EINTR != 1) {
+    switch (ret) {
+    case -EINTR: return -1;
+    case -1: return -EINTR;
+    }
+  }
+  // potentially a tail call
+  return ret;
+}

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

Re: working C11 thread implementation

[Szabolcs Nagy]

* Jens Gustedt <jens.gustedt@inria.fr> [2014-08-01 11:55:31 +0200]:
> for anybody interested I attach a first working version, that is
> mildly tested with a relatively big private application that I have
> and that uses C11 threads quite intensively.

nice

> The choices that affect the ABI that I mentioned are values of some
> constants and the sizes of the types that are introduced. These may
> change depending on glibc's choices and also according to future
> developments.
> 
> For the choices of the constants, for this version they are such that
> most wrapper calls result in being tail calls. I verified this by
> looking into the assembler that is produced. As they are now, most of
> these tail functions could also be just provided as weak aliases. We
> chose to implement them as functions such that in case of change of
> the constants we only have to recompile and no other maintenance is
> required.
> 

i'd assume the constants to be right and fix up the
code only in case of later breakage

> +  /* The following list of 9 integer constants makes up for the binary
> +     compatibility of this C thread implementation. You must never
> +     link code against versions of the C library that do not agree
> +     upon these ABI parameters.

nice

> +int cnd_signal(cnd_t * cnd) {
> +	/* In the best of all worlds this is a tail call. */
> +	int ret = __pthread_cond_signal(cnd);
> +	if (thrd_success)
> +		return ret ? thrd_error : thrd_success;
> +	return ret;
> +}

this is a bit weird

i think it's better to just assume thrd_success==0
and static assert it somewhere as a reminder

> +int mtx_init(mtx_t * m, int type)
> +{
> +	*m = (pthread_mutex_t) {
> +		._m_type = ((type&mtx_recursive) ? PTHREAD_MUTEX_RECURSIVE : 0),
> +	};
> +	return 0;
> +}

return thrd_success


> +/* This is needed because not all arch have __pthread_self as a static
> +   symbol. */
>  pthread_t pthread_self()
>  {
>  	return __pthread_self();

...

> +/* This is needed because not all arch have __pthread_self as a static
> +   symbol. */
> +pthread_t thrd_current()
> +{
> +	return __pthread_self();
> +}


i dont understand these comments

you mean that they could be weak aliases otherwise?

> +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 0;
> +}

thrd_success

> +int tss_set(tss_t k, void *x)
> +{
> +	struct pthread *self = __pthread_self();
> +	/* Avoid unnecessary COW */
> +	if (self->tsd[k] != x) {
> +		self->tsd[k] = x;
> +		self->tsd_used = 1;
> +	}
> +	return 0;
> +}

thrd_success

> +/* Roughly __syscall already returns the right thing: 0 if all went
> +   well or a negative error indication, otherwise. Unfortunately the C
> +   standard foresees the special case of an interrupted call and fixes
> +   that error return to -1 (instead of introducing EINTR). */
> +int thrd_sleep(const struct timespec *req, struct timespec *rem)
> +{
> +  int ret = __syscall(SYS_nanosleep, req, rem);
> +  // compile time comparison, constant propagated
> +  if (EINTR != 1) {
> +    switch (ret) {
> +    case -EINTR: return -1;
> +    case -1: return -EINTR;
> +    }
> +  }
> +  // potentially a tail call
> +  return ret;
> +}

"The thrd_sleep function returns zero if the requested
 time has elapsed, -1 if it has been interrupted by a
 signal, or a negative value if it fails."

this is confusing

(either should not have the name thrd_ or follow
the error enum convention of other thrd_ functions)

Re: working C11 thread implementation

[Jens Gustedt]

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

Hello,
thanks for the quick review!

Am Freitag, den 01.08.2014, 23:08 +0200 schrieb Szabolcs Nagy:
> > For the choices of the constants, for this version they are such that
> > most wrapper calls result in being tail calls. I verified this by
> > looking into the assembler that is produced. As they are now, most of
> > these tail functions could also be just provided as weak aliases. We
> > chose to implement them as functions such that in case of change of
> > the constants we only have to recompile and no other maintenance is
> > required.
> > 
> 
> i'd assume the constants to be right and fix up the
> code only in case of later breakage

Well, the problem is that these constants are not "constants" but come
from errno.h. This has two disadvantages:

 - currently this pollutes the name space of the C thread
   implementation with the E-names

 - these constants are arch dependent, we would need another mechanism
   to get them visible

My preferred solution to this is to make a TR to the thread
specification, by forcing the obvious choices and by allowing
threads.h to include errno.h. (The optional Annex K sets a precedent
by having errno codes as return value for functions.)

> > +int cnd_signal(cnd_t * cnd) {
> > +	/* In the best of all worlds this is a tail call. */
> > +	int ret = __pthread_cond_signal(cnd);
> > +	if (thrd_success)
> > +		return ret ? thrd_error : thrd_success;
> > +	return ret;
> > +}
> 
> this is a bit weird
> 
> i think it's better to just assume thrd_success==0
> and static assert it somewhere as a reminder

_Static_assert would need a compiler that implements this. It would be
easier just not to introduce a dependency for the tool chain. The code
as it is, now, will get optimized out by any decent compiler, I hope.

> > +int mtx_init(mtx_t * m, int type)
> > +{
> > +	*m = (pthread_mutex_t) {
> > +		._m_type = ((type&mtx_recursive) ? PTHREAD_MUTEX_RECURSIVE : 0),
> > +	};
> > +	return 0;
> > +}
> 
> return thrd_success

nice catch, I'll change that

(same for the other similar cases below)

> > +/* This is needed because not all arch have __pthread_self as a static
> > +   symbol. */
> >  pthread_t pthread_self()
> >  {
> >  	return __pthread_self();
> 
> ...
> 
> > +/* This is needed because not all arch have __pthread_self as a static
> > +   symbol. */
> > +pthread_t thrd_current()
> > +{
> > +	return __pthread_self();
> > +}
> 
> 
> i dont understand these comments
> you mean that they could be weak aliases otherwise?

yes something like that, I'll amend the comment

> > +/* Roughly __syscall already returns the right thing: 0 if all went
> > +   well or a negative error indication, otherwise. Unfortunately the C
> > +   standard foresees the special case of an interrupted call and fixes
> > +   that error return to -1 (instead of introducing EINTR). */
> > +int thrd_sleep(const struct timespec *req, struct timespec *rem)
> > +{
> > +  int ret = __syscall(SYS_nanosleep, req, rem);
> > +  // compile time comparison, constant propagated
> > +  if (EINTR != 1) {
> > +    switch (ret) {
> > +    case -EINTR: return -1;
> > +    case -1: return -EINTR;
> > +    }
> > +  }
> > +  // potentially a tail call
> > +  return ret;
> > +}
> 
> "The thrd_sleep function returns zero if the requested
>  time has elapsed, -1 if it has been interrupted by a
>  signal, or a negative value if it fails."
> 
> this is confusing

you mean the C standards text is confusing? yes, definitively

if you mean the code is confusing, then I should explain it better :)

The idea is that if SYS_nanosleep returns -EINTR, thrd_sleep in turn
must return -1. If EINTR is in fact 1 on the arch we just can return
the value (and 0 if everthing is ok and some undefined value in other
UB cases.)

If EINTR isn't 1, we have to be careful not to return -1 for some
other error code of SYS_nanosleep, whatever happens to be error number
1. So the second case captures such an accidental -1 and sends -EINTR
(which we know not to be -1.).

> (either should not have the name thrd_ or follow
> the error enum convention of other thrd_ functions)

I am not sure that I understand what you try to say.

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

Re: working C11 thread implementation

[Rich Felker]

On Sat, Aug 02, 2014 at 12:21:12AM +0200, Jens Gustedt wrote:
> Hello,
> thanks for the quick review!
> 
> Am Freitag, den 01.08.2014, 23:08 +0200 schrieb Szabolcs Nagy:
> > > For the choices of the constants, for this version they are such that
> > > most wrapper calls result in being tail calls. I verified this by
> > > looking into the assembler that is produced. As they are now, most of
> > > these tail functions could also be just provided as weak aliases. We
> > > chose to implement them as functions such that in case of change of
> > > the constants we only have to recompile and no other maintenance is
> > > required.
> > > 
> > 
> > i'd assume the constants to be right and fix up the
> > code only in case of later breakage
> 
> Well, the problem is that these constants are not "constants" but come
> from errno.h. This has two disadvantages:
> 
>  - currently this pollutes the name space of the C thread
>    implementation with the E-names
> 
>  - these constants are arch dependent, we would need another mechanism
>    to get them visible
> 
> My preferred solution to this is to make a TR to the thread
> specification, by forcing the obvious choices

I'm not a fan of this solution, and I'd prefer the constants be
completely independent, especially since they're not semantically
identical to the POSIX ones. I don't see how C could mandate that they
be equal to the POSIX ones when it doesn't even define the POSIX ones
in errno.h. And from a practical standpoint, the fact that the errno
values vary by arch makes them awkward to use.

Of course the most important thing in choosing these values is to
avoid an ABI divergence with glibc. And unfortunately I haven't gotten
any response from their side on how they want to define them.

> and by allowing
> threads.h to include errno.h. (The optional Annex K sets a precedent
> by having errno codes as return value for functions.)

There's no precedent for any other header including errno.h
implicitly, and I'm against adding any such requirement or even
allowance. There's a lot more value in keeping the headers independent
and requiring explicit inclusion of headers you want to use.

> > > +int cnd_signal(cnd_t * cnd) {
> > > +	/* In the best of all worlds this is a tail call. */
> > > +	int ret = __pthread_cond_signal(cnd);
> > > +	if (thrd_success)
> > > +		return ret ? thrd_error : thrd_success;
> > > +	return ret;
> > > +}
> > 
> > this is a bit weird
> > 
> > i think it's better to just assume thrd_success==0
> > and static assert it somewhere as a reminder
> 
> _Static_assert would need a compiler that implements this. It would be
> easier just not to introduce a dependency for the tool chain. The code
> as it is, now, will get optimized out by any decent compiler, I hope.

I'm fine with just assuming thrd_success is zero as long as we
actually end up defining it as such.

> > > +/* This is needed because not all arch have __pthread_self as a static
> > > +   symbol. */
> > >  pthread_t pthread_self()
> > >  {
> > >  	return __pthread_self();
> > 
> > ...
> > 
> > > +/* This is needed because not all arch have __pthread_self as a static
> > > +   symbol. */
> > > +pthread_t thrd_current()
> > > +{
> > > +	return __pthread_self();
> > > +}
> > 
> > 
> > i dont understand these comments
> > you mean that they could be weak aliases otherwise?
> 
> yes something like that, I'll amend the comment

From a standpoint of minimizing the patch for adding C11 threads and
making it C11-threads-only (rather than unrelated changes) I think the
comment for pthread_self() should be omitted. But the comment is
confusing as written anyway. Is your point that you're not making it
an alias for __pthread_self because the latter is not necessarily a
function?

> > > +/* Roughly __syscall already returns the right thing: 0 if all went
> > > +   well or a negative error indication, otherwise. Unfortunately the C
> > > +   standard foresees the special case of an interrupted call and fixes
> > > +   that error return to -1 (instead of introducing EINTR). */
> > > +int thrd_sleep(const struct timespec *req, struct timespec *rem)
> > > +{
> > > +  int ret = __syscall(SYS_nanosleep, req, rem);
> > > +  // compile time comparison, constant propagated
> > > +  if (EINTR != 1) {
> > > +    switch (ret) {
> > > +    case -EINTR: return -1;
> > > +    case -1: return -EINTR;
> > > +    }
> > > +  }
> > > +  // potentially a tail call
> > > +  return ret;
> > > +}
> > 
> > "The thrd_sleep function returns zero if the requested
> >  time has elapsed, -1 if it has been interrupted by a
> >  signal, or a negative value if it fails."
> > 
> > this is confusing
> 
> you mean the C standards text is confusing? yes, definitively
> 
> if you mean the code is confusing, then I should explain it better :)
> 
> The idea is that if SYS_nanosleep returns -EINTR, thrd_sleep in turn
> must return -1. If EINTR is in fact 1 on the arch we just can return
> the value (and 0 if everthing is ok and some undefined value in other
> UB cases.)
> 
> If EINTR isn't 1, we have to be careful not to return -1 for some
> other error code of SYS_nanosleep, whatever happens to be error number
> 1. So the second case captures such an accidental -1 and sends -EINTR
> (which we know not to be -1.).

This is kind of clever, but it might just be nicer to hard-code values
like -1 and -2 rather than swapping -1/-EINTR so that the result looks
like an errno value but really isn't one...

> > (either should not have the name thrd_ or follow
> > the error enum convention of other thrd_ functions)
> 
> I am not sure that I understand what you try to say.

Probably just that the spec is poorly designed.

Rich

C11 error codes

[Jens Gustedt]

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

Hello,
starting a new thread (!). I want to single this out, and to
later have good arguments for the discussion with the committee.

Am Freitag, den 01.08.2014, 18:57 -0400 schrieb Rich Felker:
> On Sat, Aug 02, 2014 at 12:21:12AM +0200, Jens Gustedt wrote:
> > Well, the problem is that these constants are not "constants" but come
> > from errno.h. This has two disadvantages:
> > 
> >  - currently this pollutes the name space of the C thread
> >    implementation with the E-names
> > 
> >  - these constants are arch dependent, we would need another mechanism
> >    to get them visible
> > 
> > My preferred solution to this is to make a TR to the thread
> > specification, by forcing the obvious choices
> 
> I'm not a fan of this solution, and I'd prefer the constants be
> completely independent, especially since they're not semantically
> identical to the POSIX ones. I don't see how C could mandate that they
> be equal to the POSIX ones when it doesn't even define the POSIX ones
> in errno.h. And from a practical standpoint, the fact that the errno
> values vary by arch makes them awkward to use.

I would like to see point by point how the semantics would be
different for each constant. There are not too many constants and if
there is a divergence in semantics, I think it is more likely that
this is due to the underspecification of the whole C thread
business. See below, please.

> Of course the most important thing in choosing these values is to
> avoid an ABI divergence with glibc. And unfortunately I haven't gotten
> any response from their side on how they want to define them.
> 
> > and by allowing
> > threads.h to include errno.h. (The optional Annex K sets a precedent
> > by having errno codes as return value for functions.)
> 
> There's no precedent for any other header including errno.h
> implicitly, and I'm against adding any such requirement or even
> allowance.

Ok, you are right this would go too far. What is custom though in
POSIX is to *use* the E constants as error returns everywhere. The
current practice is: any user of libc functions who wants to check
error returns (and basically everybody should) has to include errno.h
to get hands on the constant. By that POSIX ensures that everybody has
to include errno.h knowingly to their TU.

> There's a lot more value in keeping the headers independent
> and requiring explicit inclusion of headers you want to use.

right

> > > > +/* Roughly __syscall already returns the right thing: 0 if all went
> > > > +   well or a negative error indication, otherwise. Unfortunately the C
> > > > +   standard foresees the special case of an interrupted call and fixes
> > > > +   that error return to -1 (instead of introducing EINTR). */
> > > > +int thrd_sleep(const struct timespec *req, struct timespec *rem)
> > > > +{
> > > > +  int ret = __syscall(SYS_nanosleep, req, rem);
> > > > +  // compile time comparison, constant propagated
> > > > +  if (EINTR != 1) {
> > > > +    switch (ret) {
> > > > +    case -EINTR: return -1;
> > > > +    case -1: return -EINTR;
> > > > +    }
> > > > +  }
> > > > +  // potentially a tail call
> > > > +  return ret;
> > > > +}
> > > 
> > > "The thrd_sleep function returns zero if the requested
> > >  time has elapsed, -1 if it has been interrupted by a
> > >  signal, or a negative value if it fails."
> > > 
> > > this is confusing
> > 
> > you mean the C standards text is confusing? yes, definitively
> > 
> > if you mean the code is confusing, then I should explain it better :)
> > 
> > The idea is that if SYS_nanosleep returns -EINTR, thrd_sleep in turn
> > must return -1. If EINTR is in fact 1 on the arch we just can return
> > the value (and 0 if everthing is ok and some undefined value in other
> > UB cases.)
> > 
> > If EINTR isn't 1, we have to be careful not to return -1 for some
> > other error code of SYS_nanosleep, whatever happens to be error number
> > 1. So the second case captures such an accidental -1 and sends -EINTR
> > (which we know not to be -1.).
> 
> This is kind of clever, but it might just be nicer to hard-code values
> like -1 and -2 rather than swapping -1/-EINTR so that the result looks
> like an errno value but really isn't one...

(right I try to avoid such hackery)

This is another example where C11 gratiously gives up easy integration
with POSIX (which is a standard that is issued by the same
institution, ISO).

To make a list of the error constants for specific error situations
that are introduced by C11:

thrd_timeout
thrd_busy
thre_nomem
-1

(I hope I don't overlook anything)

I think these resemble a lot POSIX'

ETIMEOUT
EBUSY
ENOMEM
EINTR

and if there is a semantic difference with any of them, we should
discuss that. If there is a problem, my first suspicion would be that
C11 didn't do its homework and got things wrong. (For -1 as return of
thrd_sleep this is obvious.)

To my opinion, C11 should have introduced these four new values to the
section about errno.h.

Now let us come to the two remaining values, thrd_success and
thrd_error.

For thrd_success, I think that all our discussions clearly show that
it should just be fixed to 0. There is no point in having it
differently. This puts it in line with POSIX and Annex K. This should
not be problematic.

thrd_error is more difficult. In the best of all worlds this would not
have been defined as it is but in places where it is returned by a
function the formulation should have been something like:

   ... returns thrd_success in case of success or any positive error
   value in case of failure. The failure value should be a value
   defined in errno.h or an implementation defined macro value as
   specified in Section 7.5

   (and then eventually according to the specific function precise
   description of the errors, such as when nomem is returned, e.g.)

Now we don't have that, and it is a bit late to bury thrd_error
completely, but I will think of a way and formulation to get this
towards such a usage.

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

Re: working C11 thread implementation

[Jens Gustedt]

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

Am Freitag, den 01.08.2014, 18:57 -0400 schrieb Rich Felker:
> On Sat, Aug 02, 2014 at 12:21:12AM +0200, Jens Gustedt wrote:
> > > > +int cnd_signal(cnd_t * cnd) {
> > > > +	/* In the best of all worlds this is a tail call. */
> > > > +	int ret = __pthread_cond_signal(cnd);
> > > > +	if (thrd_success)
> > > > +		return ret ? thrd_error : thrd_success;
> > > > +	return ret;
> > > > +}
> > > 
> > > this is a bit weird
> > > 
> > > i think it's better to just assume thrd_success==0
> > > and static assert it somewhere as a reminder
> > 
> > _Static_assert would need a compiler that implements this. It would be
> > easier just not to introduce a dependency for the tool chain. The code
> > as it is, now, will get optimized out by any decent compiler, I hope.
> 
> I'm fine with just assuming thrd_success is zero as long as we
> actually end up defining it as such.

I thought of this, but I am not in favor. History does not always turn
to the reasonable and obvious solution and there are many places where
the code would have to be updated if that assumption wouldn't hold. I
want these places clearly marked and grepable.

The day the C standard is amended to fix that constant to anyting (0
or non-0), I'll submit a patch that eliminates all of its uses. As
long as this is not specified in the standard, I'd like to stay with
it to ease maintenance.

> From a standpoint of minimizing the patch for adding C11 threads and
> making it C11-threads-only (rather than unrelated changes) I think the
> comment for pthread_self() should be omitted.

Generally, this is a matter of style. I often find that implementation
choices in musl could better be explained in place. For somebody new
to the sources (as I am) this often implies a lot of digging to find
the reason for a particular choice.

> But the comment is
> confusing as written anyway. Is your point that you're not making it
> an alias for __pthread_self because the latter is not necessarily a
> function?

yes, I'll try to formulate that better


> > If EINTR isn't 1, we have to be careful not to return -1 for some
> > other error code of SYS_nanosleep, whatever happens to be error number
> > 1. So the second case captures such an accidental -1 and sends -EINTR
> > (which we know not to be -1.).
> 
> This is kind of clever, but it might just be nicer to hard-code values
> like -1 and -2 rather than swapping -1/-EINTR so that the result looks
> like an errno value but really isn't one...

ok, I'll avoid that hackery

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