* [PATCH 1/8] interface additions for the C thread implementation
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
@ 2014-08-30 18:46 ` Jens Gustedt
2014-08-30 18:46 ` [PATCH 2/8] additions to src/time Jens Gustedt
` (6 subsequent siblings)
7 siblings, 0 replies; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:46 UTC (permalink / raw)
To: musl
This adds all the constant, type and function interfaces.
It makes pthread_mutex_t, mtx_t, pthread_cond_t and cnd_t different
types.
This only works because
- under hood the corresponding pairs of types use exactly the same
definition for the type
- the types are a struct types without tag name
- no comparison or assignment is allowed for any of these types. For
the POSIX types this interdiction is written in the standard. For the
C thread types, this is an extension that this implementation
imposes, but which might be integrated in a later version of the C
standard.
- initialization is default initialization of an array of int. For the
POSIX types, initialization expressions are provided. For C thread
types the only initialization foreseen by the standard are the init
functions.
- any calls to standard functions use pointers, and because pointer
representations for struct types are the same.
For the C++ API/ABI, these also are different types, now, with type names
(that are used for name mangling, e.g) as listed above.
Somebody better versed in C++ could perhaps contribute code that
overloads the comparison and assignment operators such that a compilation
that tries to compare or copy these types fails.
---
arch/arm/bits/alltypes.h.in | 10 +++-
arch/i386/bits/alltypes.h.in | 10 +++-
arch/microblaze/bits/alltypes.h.in | 10 +++-
arch/mips/bits/alltypes.h.in | 10 +++-
arch/or1k/bits/alltypes.h.in | 10 +++-
arch/powerpc/bits/alltypes.h.in | 10 +++-
arch/sh/bits/alltypes.h.in | 10 +++-
arch/x32/bits/alltypes.h.in | 10 +++-
arch/x86_64/bits/alltypes.h.in | 10 +++-
include/alltypes.h.in | 10 ++++
include/threads.h | 110 ++++++++++++++++++++++++++++++++++++
include/time.h | 11 ++++
12 files changed, 212 insertions(+), 9 deletions(-)
create mode 100644 include/threads.h
diff --git a/arch/arm/bits/alltypes.h.in b/arch/arm/bits/alltypes.h.in
index 183c4c4..1dcb920 100644
--- a/arch/arm/bits/alltypes.h.in
+++ b/arch/arm/bits/alltypes.h.in
@@ -18,8 +18,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[8]; void *__p[8]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[5]; void *__p[5]; } __u; } pthread_barrier_t;
diff --git a/arch/i386/bits/alltypes.h.in b/arch/i386/bits/alltypes.h.in
index 8ba8f6f..6fe9d05 100644
--- a/arch/i386/bits/alltypes.h.in
+++ b/arch/i386/bits/alltypes.h.in
@@ -32,8 +32,16 @@ TYPEDEF struct { _Alignas(8) long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[8]; void *__p[8]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[5]; void *__p[5]; } __u; } pthread_barrier_t;
diff --git a/arch/microblaze/bits/alltypes.h.in b/arch/microblaze/bits/alltypes.h.in
index a03e1b8..ad4d898 100644
--- a/arch/microblaze/bits/alltypes.h.in
+++ b/arch/microblaze/bits/alltypes.h.in
@@ -18,8 +18,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[8]; void *__p[8]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[5]; void *__p[5]; } __u; } pthread_barrier_t;
diff --git a/arch/mips/bits/alltypes.h.in b/arch/mips/bits/alltypes.h.in
index a03e1b8..ad4d898 100644
--- a/arch/mips/bits/alltypes.h.in
+++ b/arch/mips/bits/alltypes.h.in
@@ -18,8 +18,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[8]; void *__p[8]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[5]; void *__p[5]; } __u; } pthread_barrier_t;
diff --git a/arch/or1k/bits/alltypes.h.in b/arch/or1k/bits/alltypes.h.in
index 183c4c4..1dcb920 100644
--- a/arch/or1k/bits/alltypes.h.in
+++ b/arch/or1k/bits/alltypes.h.in
@@ -18,8 +18,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[8]; void *__p[8]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[5]; void *__p[5]; } __u; } pthread_barrier_t;
diff --git a/arch/powerpc/bits/alltypes.h.in b/arch/powerpc/bits/alltypes.h.in
index ee7f137..63f88ca 100644
--- a/arch/powerpc/bits/alltypes.h.in
+++ b/arch/powerpc/bits/alltypes.h.in
@@ -18,8 +18,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[8]; void *__p[8]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[5]; void *__p[5]; } __u; } pthread_barrier_t;
diff --git a/arch/sh/bits/alltypes.h.in b/arch/sh/bits/alltypes.h.in
index ee7f137..63f88ca 100644
--- a/arch/sh/bits/alltypes.h.in
+++ b/arch/sh/bits/alltypes.h.in
@@ -18,8 +18,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[8]; void *__p[8]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[5]; void *__p[5]; } __u; } pthread_barrier_t;
diff --git a/arch/x32/bits/alltypes.h.in b/arch/x32/bits/alltypes.h.in
index 8e396c9..3e8f086 100644
--- a/arch/x32/bits/alltypes.h.in
+++ b/arch/x32/bits/alltypes.h.in
@@ -23,8 +23,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long long time_t;
TYPEDEF long long suseconds_t;
-TYPEDEF struct { union { int __i[14]; unsigned long __s[7]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[10]; volatile void *volatile __p[5]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[10]; volatile void *volatile __p[5]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[6]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[6]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[14]; unsigned long __s[7]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[14]; void *__p[7]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[8]; void *__p[4]; } __u; } pthread_barrier_t;
diff --git a/arch/x86_64/bits/alltypes.h.in b/arch/x86_64/bits/alltypes.h.in
index 7b4f3e7..c0169c7 100644
--- a/arch/x86_64/bits/alltypes.h.in
+++ b/arch/x86_64/bits/alltypes.h.in
@@ -23,8 +23,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
TYPEDEF long time_t;
TYPEDEF long suseconds_t;
-TYPEDEF struct { union { int __i[14]; unsigned long __s[7]; } __u; } pthread_attr_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync.
+ *
+ * Also this only works because the underlying struct has no struct
+ * tag. Don't introduce one. */
TYPEDEF struct { union { int __i[10]; volatile void *volatile __p[5]; } __u; } pthread_mutex_t;
+TYPEDEF struct { union { int __i[10]; volatile void *volatile __p[5]; } __u; } mtx_t;
TYPEDEF struct { union { int __i[12]; void *__p[6]; } __u; } pthread_cond_t;
+TYPEDEF struct { union { int __i[12]; void *__p[6]; } __u; } cnd_t;
+
+TYPEDEF struct { union { int __i[14]; unsigned long __s[7]; } __u; } pthread_attr_t;
TYPEDEF struct { union { int __i[14]; void *__p[7]; } __u; } pthread_rwlock_t;
TYPEDEF struct { union { int __i[8]; void *__p[4]; } __u; } pthread_barrier_t;
diff --git a/include/alltypes.h.in b/include/alltypes.h.in
index c4ca5d5..94364e8 100644
--- a/include/alltypes.h.in
+++ b/include/alltypes.h.in
@@ -43,13 +43,23 @@ TYPEDEF unsigned gid_t;
TYPEDEF int key_t;
TYPEDEF unsigned useconds_t;
+/* The pairs of equivalent definitions for pthread and C thread types
+ * should always be kept in sync. */
#ifdef __cplusplus
TYPEDEF unsigned long pthread_t;
+TYPEDEF unsigned long thrd_t;
#else
TYPEDEF struct __pthread * pthread_t;
+TYPEDEF struct __pthread * thrd_t;
#endif
TYPEDEF int pthread_once_t;
+TYPEDEF int once_flag;
TYPEDEF unsigned pthread_key_t;
+TYPEDEF unsigned tss_t;
+
+TYPEDEF pthread_cond_t cnd_t;
+TYPEDEF pthread_mutex_t mtx_t;
+
TYPEDEF int pthread_spinlock_t;
TYPEDEF struct { unsigned __attr; } pthread_mutexattr_t;
TYPEDEF struct { unsigned __attr; } pthread_condattr_t;
diff --git a/include/threads.h b/include/threads.h
new file mode 100644
index 0000000..0e99443
--- /dev/null
+++ b/include/threads.h
@@ -0,0 +1,110 @@
+#ifndef _THREADS_H
+#define _THREADS_H
+
+/* This one is explicitly allowed to be included. */
+#include <time.h>
+
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* These may or may not be implemented to be the same as some POSIX
+ * types. Don't rely on any assumption about that, in particular if
+ * you happen to use both interfaces in the same code. */
+
+#define __NEED_cnd_t
+#define __NEED_mtx_t
+#define __NEED_once_flag
+#define __NEED_thrd_t
+#define __NEED_tss_t
+
+#include <bits/alltypes.h>
+
+typedef int (*thrd_start_t)(void*);
+typedef void (*tss_dtor_t)(void*);
+
+/* This should be a keyword for C++11 */
+#ifndef __cplusplus
+# define thread_local _Thread_local
+#endif
+
+ /* The following list of 10 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 10 parameters are those that we proposed to
+ glibc, in the hope that they will agree.
+ */
+
+#define TSS_DTOR_ITERATIONS 4
+
+enum {
+ thrd_success = 0,
+ thrd_busy = 1,
+ thrd_error = 2,
+ thrd_nomem = 3,
+ thrd_timedout = 4,
+};
+
+enum {
+ mtx_plain = 0,
+ mtx_recursive = 1,
+ // all mutexes are timed, here. so this is a no-op
+ mtx_timed = 2,
+};
+
+#define ONCE_FLAG_INIT { 0 }
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+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/include/time.h b/include/time.h
index dc88070..9ea9c73 100644
--- a/include/time.h
+++ b/include/time.h
@@ -129,6 +129,17 @@ int stime(const time_t *);
time_t timegm(struct tm *);
#endif
+ /* Implementation specific choice: The epoch that the TIME_UTC clock
+ is based upon is the Unix Epoch, that is a struct timespec
+ represents the number of seconds that have elapsed since 00:00:00
+ Coordinated Universal Time (UTC), Thursday, 1 January
+ 1970. Because of differences in leap seconds this is not
+ completely equivalent to UTC. */
+ /* Beware that the TIME_UTC constant itself per the standard must be
+ greater than 0. */
+#define TIME_UTC 1
+int timespec_get(struct timespec *, int);
+
#ifdef __cplusplus
}
#endif
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH 2/8] additions to src/time
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
2014-08-30 18:46 ` [PATCH 1/8] interface additions for the C thread implementation Jens Gustedt
@ 2014-08-30 18:46 ` Jens Gustedt
2014-08-31 0:13 ` Rich Felker
2014-08-30 18:46 ` [PATCH 3/8] use weak symbols for the POSIX functions that will be used by C threads Jens Gustedt
` (5 subsequent siblings)
7 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:46 UTC (permalink / raw)
To: musl
This adds two functions, thrd_sleep and timespec_get. Both have easy
functional equivalences in POSIX, but these have subtle differences in
the handling of errors.
thrd_sleep forces concrete numerical values as error return
timespec_get has a call interface that is incompatible with POSIX because
it has a bogus coding for its clock constants, and also this clock
constants must be returned in case of success. For the moment we only
implement one single clock, TIME_UTC, and map this to
CLOCK_REALTIME. This is the clock that we later need to measure time for
the timedlock and timedwait.
Also, other than for clock_gettime, C doesn't specify touching errno for
timespec_get. Because CLOCK_REALTIME can be obtained very efficiently
through VDSO and messing with errno is clearly the wrong step to go, we
try to avoid this.
---
src/time/thrd_sleep.c | 26 ++++++++++++++++++++++++++
src/time/timespec_get.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
create mode 100644 src/time/thrd_sleep.c
create mode 100644 src/time/timespec_get.c
diff --git a/src/time/thrd_sleep.c b/src/time/thrd_sleep.c
new file mode 100644
index 0000000..3dbfe47
--- /dev/null
+++ b/src/time/thrd_sleep.c
@@ -0,0 +1,26 @@
+#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. */
+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 */
+ case -EINTR:
+ return -1;
+ /* EINVAL: described by POSIX */
+ /* EFAULT: described for linux */
+ default:
+ return -2;
+ }
+}
diff --git a/src/time/timespec_get.c b/src/time/timespec_get.c
new file mode 100644
index 0000000..20080a0
--- /dev/null
+++ b/src/time/timespec_get.c
@@ -0,0 +1,31 @@
+#include <time.h>
+#include "syscall.h"
+#include "atomic.h"
+
+static int syscall_clock_gettime(clockid_t clk, struct timespec *ts)
+{
+ return __syscall(SYS_clock_gettime, clk, ts);
+}
+
+void *__vdsosym(const char *, const char *);
+
+/* There is no other implemented value than TIME_UTC, all other values
+ are considered erroneous. */
+int timespec_get(struct timespec * ts, int base)
+{
+ if (base != TIME_UTC) return 0;
+ int ret;
+#ifdef VDSO_CGT_SYM
+ static int (*cgt)(clockid_t, struct timespec *);
+ if (!cgt) {
+ void *f = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
+ if (!f) f = (void *)syscall_clock_gettime;
+ a_cas_p(&cgt, 0, f);
+ }
+ /* The vdso variants never fail, and thus never set errno. */
+ ret = cgt(CLOCK_REALTIME, ts);
+#else
+ ret = syscall_clock_gettime(CLOCK_REALTIME, ts);
+#endif
+ return ret < 0 ? 0 : base;
+}
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/8] additions to src/time
2014-08-30 18:46 ` [PATCH 2/8] additions to src/time Jens Gustedt
@ 2014-08-31 0:13 ` Rich Felker
2014-08-31 7:15 ` Jens Gustedt
0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 0:13 UTC (permalink / raw)
To: musl
On Sat, Aug 30, 2014 at 08:46:35PM +0200, Jens Gustedt wrote:
> This adds two functions, thrd_sleep and timespec_get. Both have easy
> functional equivalences in POSIX, but these have subtle differences in
> the handling of errors.
>
> thrd_sleep forces concrete numerical values as error return
>
> timespec_get has a call interface that is incompatible with POSIX because
> it has a bogus coding for its clock constants, and also this clock
> constants must be returned in case of success. For the moment we only
> implement one single clock, TIME_UTC, and map this to
> CLOCK_REALTIME. This is the clock that we later need to measure time for
> the timedlock and timedwait.
>
> Also, other than for clock_gettime, C doesn't specify touching errno for
> timespec_get. Because CLOCK_REALTIME can be obtained very efficiently
> through VDSO and messing with errno is clearly the wrong step to go, we
> try to avoid this.
> ---
> src/time/thrd_sleep.c | 26 ++++++++++++++++++++++++++
> src/time/timespec_get.c | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+)
> create mode 100644 src/time/thrd_sleep.c
> create mode 100644 src/time/timespec_get.c
>
> diff --git a/src/time/thrd_sleep.c b/src/time/thrd_sleep.c
> new file mode 100644
> index 0000000..3dbfe47
> --- /dev/null
> +++ b/src/time/thrd_sleep.c
> @@ -0,0 +1,26 @@
> +#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. */
> +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 */
Purely coding style, but for musl we generally do multi-line comments
in this form:
/* line 1
* line 2
* line 3 */
Using // would be a second-choice alternative. I don't like the
right-alignment of */ with spaces because it will look like a mess to
someone reading it with a non-fixed-width font.
> diff --git a/src/time/timespec_get.c b/src/time/timespec_get.c
> new file mode 100644
> index 0000000..20080a0
> --- /dev/null
> +++ b/src/time/timespec_get.c
> @@ -0,0 +1,31 @@
> +#include <time.h>
> +#include "syscall.h"
> +#include "atomic.h"
> +
> +static int syscall_clock_gettime(clockid_t clk, struct timespec *ts)
> +{
> + return __syscall(SYS_clock_gettime, clk, ts);
> +}
> +
> +void *__vdsosym(const char *, const char *);
> +
> +/* There is no other implemented value than TIME_UTC, all other values
> + are considered erroneous. */
> +int timespec_get(struct timespec * ts, int base)
> +{
> + if (base != TIME_UTC) return 0;
> + int ret;
> +#ifdef VDSO_CGT_SYM
> + static int (*cgt)(clockid_t, struct timespec *);
> + if (!cgt) {
> + void *f = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
> + if (!f) f = (void *)syscall_clock_gettime;
> + a_cas_p(&cgt, 0, f);
> + }
> + /* The vdso variants never fail, and thus never set errno. */
> + ret = cgt(CLOCK_REALTIME, ts);
> +#else
> + ret = syscall_clock_gettime(CLOCK_REALTIME, ts);
> +#endif
> + return ret < 0 ? 0 : base;
I'd rather not duplicate this code but just call __clock_gettime.
Unlike __clock_gettime, your duplicate code does not handle
pre-clock_gettime kernels and will return an error on them.
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/8] additions to src/time
2014-08-31 0:13 ` Rich Felker
@ 2014-08-31 7:15 ` Jens Gustedt
2014-08-31 12:45 ` Rich Felker
0 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 7:15 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]
Am Samstag, den 30.08.2014, 20:13 -0400 schrieb Rich Felker:
> On Sat, Aug 30, 2014 at 08:46:35PM +0200, Jens Gustedt wrote:
> > This adds two functions, thrd_sleep and timespec_get. Both have easy
> > functional equivalences in POSIX, but these have subtle differences in
> > the handling of errors.
> >
> > thrd_sleep forces concrete numerical values as error return
> >
> > timespec_get has a call interface that is incompatible with POSIX because
> > it has a bogus coding for its clock constants, and also this clock
> > constants must be returned in case of success. For the moment we only
> > implement one single clock, TIME_UTC, and map this to
> > CLOCK_REALTIME. This is the clock that we later need to measure time for
> > the timedlock and timedwait.
> >
> > Also, other than for clock_gettime, C doesn't specify touching errno for
> > timespec_get. Because CLOCK_REALTIME can be obtained very efficiently
> > through VDSO and messing with errno is clearly the wrong step to go, we
> > try to avoid this.
> > ---
> > src/time/thrd_sleep.c | 26 ++++++++++++++++++++++++++
> > src/time/timespec_get.c | 31 +++++++++++++++++++++++++++++++
> > 2 files changed, 57 insertions(+)
> > create mode 100644 src/time/thrd_sleep.c
> > create mode 100644 src/time/timespec_get.c
> >
> > diff --git a/src/time/thrd_sleep.c b/src/time/thrd_sleep.c
> > new file mode 100644
> > index 0000000..3dbfe47
> > --- /dev/null
> > +++ b/src/time/thrd_sleep.c
> > @@ -0,0 +1,26 @@
> > +#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. */
> > +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 */
>
> Purely coding style, but for musl we generally do multi-line comments
> in this form:
>
> /* line 1
> * line 2
> * line 3 */
I don't remeber having seen that in the coding style recommendations :)
I'll fix those that I notice, and keep that in mind for future use.
> Using // would be a second-choice alternative. I don't like the
> right-alignment of */ with spaces because it will look like a mess to
> someone reading it with a non-fixed-width font.
>
> > diff --git a/src/time/timespec_get.c b/src/time/timespec_get.c
> > new file mode 100644
> > index 0000000..20080a0
> > --- /dev/null
> > +++ b/src/time/timespec_get.c
> > @@ -0,0 +1,31 @@
> > +#include <time.h>
> > +#include "syscall.h"
> > +#include "atomic.h"
> > +
> > +static int syscall_clock_gettime(clockid_t clk, struct timespec *ts)
> > +{
> > + return __syscall(SYS_clock_gettime, clk, ts);
> > +}
> > +
> > +void *__vdsosym(const char *, const char *);
> > +
> > +/* There is no other implemented value than TIME_UTC, all other values
> > + are considered erroneous. */
> > +int timespec_get(struct timespec * ts, int base)
> > +{
> > + if (base != TIME_UTC) return 0;
> > + int ret;
> > +#ifdef VDSO_CGT_SYM
> > + static int (*cgt)(clockid_t, struct timespec *);
> > + if (!cgt) {
> > + void *f = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
> > + if (!f) f = (void *)syscall_clock_gettime;
> > + a_cas_p(&cgt, 0, f);
> > + }
> > + /* The vdso variants never fail, and thus never set errno. */
> > + ret = cgt(CLOCK_REALTIME, ts);
> > +#else
> > + ret = syscall_clock_gettime(CLOCK_REALTIME, ts);
> > +#endif
> > + return ret < 0 ? 0 : base;
>
> I'd rather not duplicate this code but just call __clock_gettime.
ok
> Unlike __clock_gettime, your duplicate code does not handle
> pre-clock_gettime kernels and will return an error on them.
yeah, somehow I missed the gettimeofday syscall.
Would it be acceptable by any chance to have a common core for these
two functions that doesn't set errno? It would perhaps be appropriate
to use such a function in other places of musl, too.
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/8] additions to src/time
2014-08-31 7:15 ` Jens Gustedt
@ 2014-08-31 12:45 ` Rich Felker
0 siblings, 0 replies; 31+ messages in thread
From: Rich Felker @ 2014-08-31 12:45 UTC (permalink / raw)
To: musl
On Sun, Aug 31, 2014 at 09:15:53AM +0200, Jens Gustedt wrote:
> > Unlike __clock_gettime, your duplicate code does not handle
> > pre-clock_gettime kernels and will return an error on them.
>
> yeah, somehow I missed the gettimeofday syscall.
>
> Would it be acceptable by any chance to have a common core for these
> two functions that doesn't set errno? It would perhaps be appropriate
> to use such a function in other places of musl, too.
As any standard function that's not documented not to is allowed to
clobber errno, no special effort is made to preserve it except where
it's required to do so. This is the case all over the source. The
whole reason the C standard allows this clobbering is so that you can
freely use standard functions to implement each other without having
ugly (and potentially slow) code to save and restore errno all over
the place, or having alternate versions of each function that don't
set errno. So I don't see any value in having one here. There is no
cost to the errno-setting code except in code that's invoking the
error path, which should not happen anyway unless you pass an invalid
argument or have a really outdated kernel.
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/8] use weak symbols for the POSIX functions that will be used by C threads
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
2014-08-30 18:46 ` [PATCH 1/8] interface additions for the C thread implementation Jens Gustedt
2014-08-30 18:46 ` [PATCH 2/8] additions to src/time Jens Gustedt
@ 2014-08-30 18:46 ` Jens Gustedt
2014-08-31 0:17 ` Rich Felker
2014-08-30 18:47 ` [PATCH 4/8] add the functions for tss_t and once_flag Jens Gustedt
` (4 subsequent siblings)
7 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:46 UTC (permalink / raw)
To: musl
The intent of this is to avoid name space pollution of the C threads
implementation.
This has two sides to it. First we have to provide symbols that wouldn't
pollute the name space for the C threads implementation. Second we have
to clean up some internal uses of POSIX functions such that they don't
implicitly drag in such symbols.
---
src/mman/mprotect.c | 4 +++-
src/thread/__timedwait.c | 9 ++++++---
src/thread/pthread_cond_timedwait.c | 12 +++++++++---
src/thread/pthread_create.c | 21 ++++++++++++++-------
src/thread/pthread_detach.c | 8 ++++++--
src/thread/pthread_getspecific.c | 5 ++++-
src/thread/pthread_join.c | 8 ++++++--
src/thread/pthread_key_create.c | 7 +++++--
src/thread/pthread_mutex_lock.c | 8 ++++++--
src/thread/pthread_mutex_timedlock.c | 4 +++-
src/thread/pthread_mutex_trylock.c | 4 +++-
src/thread/pthread_mutex_unlock.c | 4 +++-
src/thread/pthread_once.c | 4 +++-
src/thread/pthread_setcancelstate.c | 4 +++-
src/thread/pthread_testcancel.c | 4 +++-
15 files changed, 77 insertions(+), 29 deletions(-)
diff --git a/src/mman/mprotect.c b/src/mman/mprotect.c
index f488486..535787b 100644
--- a/src/mman/mprotect.c
+++ b/src/mman/mprotect.c
@@ -2,10 +2,12 @@
#include "libc.h"
#include "syscall.h"
-int mprotect(void *addr, size_t len, int prot)
+int __mprotect(void *addr, size_t len, int prot)
{
size_t start, end;
start = (size_t)addr & -PAGE_SIZE;
end = (size_t)((char *)addr + len + PAGE_SIZE-1) & -PAGE_SIZE;
return syscall(SYS_mprotect, start, end-start, prot);
}
+
+weak_alias(__mprotect, mprotect);
diff --git a/src/thread/__timedwait.c b/src/thread/__timedwait.c
index d6f1233..3d62272 100644
--- a/src/thread/__timedwait.c
+++ b/src/thread/__timedwait.c
@@ -4,6 +4,9 @@
#include "futex.h"
#include "syscall.h"
+int __pthread_setcancelstate(int new, int *old);
+int __clock_gettime(clockid_t clk, struct timespec *ts);
+
int __timedwait(volatile int *addr, int val,
clockid_t clk, const struct timespec *at,
void (*cleanup)(void *), void *arg, int priv)
@@ -15,7 +18,7 @@ int __timedwait(volatile int *addr, int val,
if (at) {
if (at->tv_nsec >= 1000000000UL) return EINVAL;
- if (clock_gettime(clk, &to)) return EINVAL;
+ if (__clock_gettime(clk, &to)) return EINVAL;
to.tv_sec = at->tv_sec - to.tv_sec;
if ((to.tv_nsec = at->tv_nsec - to.tv_nsec) < 0) {
to.tv_sec--;
@@ -25,7 +28,7 @@ int __timedwait(volatile int *addr, int val,
top = &to;
}
- if (!cleanup) pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+ if (!cleanup) __pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
pthread_cleanup_push(cleanup, arg);
r = -__syscall_cp(SYS_futex, addr, FUTEX_WAIT|priv, val, top);
@@ -33,7 +36,7 @@ int __timedwait(volatile int *addr, int val,
if (r != EINTR && r != ETIMEDOUT) r = 0;
pthread_cleanup_pop(0);
- if (!cleanup) pthread_setcancelstate(cs, 0);
+ if (!cleanup) __pthread_setcancelstate(cs, 0);
return r;
}
diff --git a/src/thread/pthread_cond_timedwait.c b/src/thread/pthread_cond_timedwait.c
index 2d192b0..393d60e 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 waiter
*
@@ -119,7 +123,7 @@ static void unwait(void *arg)
}
}
-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 waiter node = { .cond = c, .mutex = m };
int e, seq, *fut, clock = c->_c_clock;
@@ -130,7 +134,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();
if (c->_c_shared) {
node.shared = 1;
@@ -151,7 +155,7 @@ int pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict
unlock(&c->_c_lock);
}
- pthread_mutex_unlock(m);
+ __pthread_mutex_unlock(m);
do e = __timedwait(fut, seq, clock, ts, unwait, &node, !node.shared);
while (*fut==seq && (!e || e==EINTR));
@@ -197,3 +201,5 @@ int __private_cond_signal(pthread_cond_t *c, int n)
return 0;
}
+
+weak_alias(__pthread_cond_timedwait, pthread_cond_timedwait);
diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index e441bda..ed265fb 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -5,6 +5,10 @@
#include <sys/mman.h>
#include <string.h>
+void *__mmap(void *, size_t, int, int, int, off_t);
+int __munmap(void *, size_t);
+int __mprotect(void *, size_t, int);
+
static void dummy_0()
{
}
@@ -14,7 +18,7 @@ weak_alias(dummy_0, __pthread_tsd_run_dtors);
weak_alias(dummy_0, __do_private_robust_list);
weak_alias(dummy_0, __do_orphaned_stdio_locks);
-_Noreturn void pthread_exit(void *result)
+_Noreturn void __pthread_exit(void *result)
{
pthread_t self = __pthread_self();
sigset_t set;
@@ -139,7 +143,7 @@ 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;
@@ -191,14 +195,14 @@ int pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp
if (!tsd) {
if (guard) {
- map = mmap(0, size, PROT_NONE, MAP_PRIVATE|MAP_ANON, -1, 0);
+ map = __mmap(0, size, PROT_NONE, MAP_PRIVATE|MAP_ANON, -1, 0);
if (map == MAP_FAILED) goto fail;
- if (mprotect(map+guard, size-guard, PROT_READ|PROT_WRITE)) {
- munmap(map, size);
+ if (__mprotect(map+guard, size-guard, PROT_READ|PROT_WRITE)) {
+ __munmap(map, size);
goto fail;
}
} else {
- map = mmap(0, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
+ map = __mmap(0, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
if (map == MAP_FAILED) goto fail;
}
tsd = map + size - __pthread_tsd_size;
@@ -240,7 +244,7 @@ int pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp
if (ret < 0) {
a_dec(&libc.threads_minus_1);
- if (map) munmap(map, size);
+ if (map) __munmap(map, size);
return EAGAIN;
}
@@ -258,3 +262,6 @@ fail:
__release_ptc();
return EAGAIN;
}
+
+weak_alias(__pthread_exit, pthread_exit);
+weak_alias(__pthread_create, pthread_create);
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_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..bca89aa 100644
--- a/src/thread/pthread_join.c
+++ b/src/thread/pthread_join.c
@@ -1,15 +1,19 @@
#include "pthread_impl.h"
#include <sys/mman.h>
+int __munmap(void *, size_t);
+
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);
if (res) *res = t->result;
- if (t->map_base) munmap(t->map_base, t->map_size);
+ 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 2a9a3aa..993f54c 100644
--- a/src/thread/pthread_mutex_lock.c
+++ b/src/thread/pthread_mutex_lock.c
@@ -1,10 +1,14 @@
#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&15) == 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 ae883f9..16241ee 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)
{
if ((m->_m_type&15) == PTHREAD_MUTEX_NORMAL
&& !a_cas(&m->_m_lock, 0, EBUSY))
@@ -30,3 +30,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 e851517..9be7930 100644
--- a/src/thread/pthread_mutex_trylock.c
+++ b/src/thread/pthread_mutex_trylock.c
@@ -50,9 +50,11 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m)
return 0;
}
-int pthread_mutex_trylock(pthread_mutex_t *m)
+static int __pthread_mutex_trylock(pthread_mutex_t *m)
{
if ((m->_m_type&15) == PTHREAD_MUTEX_NORMAL)
return a_cas(&m->_m_lock, 0, EBUSY) & EBUSY;
return __pthread_mutex_trylock_owner(m);
}
+
+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 46761d9..a7f39c7 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;
@@ -36,3 +36,5 @@ int pthread_mutex_unlock(pthread_mutex_t *m)
__wake(&m->_m_lock, 1, priv);
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 2eb0f93..05ebe69 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, 1);
}
-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_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_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);
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/8] use weak symbols for the POSIX functions that will be used by C threads
2014-08-30 18:46 ` [PATCH 3/8] use weak symbols for the POSIX functions that will be used by C threads Jens Gustedt
@ 2014-08-31 0:17 ` Rich Felker
2014-08-31 7:18 ` Jens Gustedt
0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 0:17 UTC (permalink / raw)
To: musl
On Sat, Aug 30, 2014 at 08:46:49PM +0200, Jens Gustedt wrote:
> diff --git a/src/mman/mprotect.c b/src/mman/mprotect.c
> index f488486..535787b 100644
> --- a/src/mman/mprotect.c
> +++ b/src/mman/mprotect.c
> @@ -2,10 +2,12 @@
> #include "libc.h"
> #include "syscall.h"
>
> -int mprotect(void *addr, size_t len, int prot)
> +int __mprotect(void *addr, size_t len, int prot)
> {
> size_t start, end;
> start = (size_t)addr & -PAGE_SIZE;
> end = (size_t)((char *)addr + len + PAGE_SIZE-1) & -PAGE_SIZE;
> return syscall(SYS_mprotect, start, end-start, prot);
> }
> +
> +weak_alias(__mprotect, mprotect);
I'll go ahead and eliminate this one from pthread_create.c like I had
mentioned so we can omit it. Doing so should be a size reduction too,
both locally (syscall is generally cheaper than function call) and for
static linking (since mprotect.o won't get pulled in).
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/8] use weak symbols for the POSIX functions that will be used by C threads
2014-08-31 0:17 ` Rich Felker
@ 2014-08-31 7:18 ` Jens Gustedt
0 siblings, 0 replies; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 7:18 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]
Am Samstag, den 30.08.2014, 20:17 -0400 schrieb Rich Felker:
> On Sat, Aug 30, 2014 at 08:46:49PM +0200, Jens Gustedt wrote:
> > diff --git a/src/mman/mprotect.c b/src/mman/mprotect.c
> > index f488486..535787b 100644
> > --- a/src/mman/mprotect.c
> > +++ b/src/mman/mprotect.c
> > @@ -2,10 +2,12 @@
> > #include "libc.h"
> > #include "syscall.h"
> >
> > -int mprotect(void *addr, size_t len, int prot)
> > +int __mprotect(void *addr, size_t len, int prot)
> > {
> > size_t start, end;
> > start = (size_t)addr & -PAGE_SIZE;
> > end = (size_t)((char *)addr + len + PAGE_SIZE-1) & -PAGE_SIZE;
> > return syscall(SYS_mprotect, start, end-start, prot);
> > }
> > +
> > +weak_alias(__mprotect, mprotect);
>
> I'll go ahead and eliminate this one from pthread_create.c like I had
> mentioned so we can omit it. Doing so should be a size reduction too,
> both locally (syscall is generally cheaper than function call) and for
> static linking (since mprotect.o won't get pulled in).
Ok, I'll drop that part then. Please drop me a line when you did,
because I'd have to rebase 7/8 and 8/8 on top of that.
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/8] add the functions for tss_t and once_flag
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
` (2 preceding siblings ...)
2014-08-30 18:46 ` [PATCH 3/8] use weak symbols for the POSIX functions that will be used by C threads Jens Gustedt
@ 2014-08-30 18:47 ` Jens Gustedt
2014-08-30 18:47 ` [PATCH 5/8] add the functions for mtx_t Jens Gustedt
` (3 subsequent siblings)
7 siblings, 0 replies; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:47 UTC (permalink / raw)
To: musl
These all have POSIX equivalents but with some changes to the interfaces.
---
src/thread/call_once.c | 7 +++++++
src/thread/tss_create.c | 11 +++++++++++
src/thread/tss_delete.c | 7 +++++++
src/thread/tss_set.c | 13 +++++++++++++
4 files changed, 38 insertions(+)
create mode 100644 src/thread/call_once.c
create mode 100644 src/thread/tss_create.c
create mode 100644 src/thread/tss_delete.c
create mode 100644 src/thread/tss_set.c
diff --git a/src/thread/call_once.c b/src/thread/call_once.c
new file mode 100644
index 0000000..26c735c
--- /dev/null
+++ b/src/thread/call_once.c
@@ -0,0 +1,7 @@
+#include <threads.h>
+
+int __pthread_once(once_flag *, void (*)(void));
+
+void (call_once)(once_flag *flag, void (*func)(void)) {
+ (void)__pthread_once(flag, func);
+}
diff --git a/src/thread/tss_create.c b/src/thread/tss_create.c
new file mode 100644
index 0000000..af8772a
--- /dev/null
+++ b/src/thread/tss_create.c
@@ -0,0 +1,11 @@
+#include <threads.h>
+
+int __pthread_key_create(tss_t *k, void (*dtor)(void *));
+
+int tss_create(tss_t *tss, tss_dtor_t dtor)
+{
+ /* 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..7d7aacb
--- /dev/null
+++ b/src/thread/tss_delete.c
@@ -0,0 +1,7 @@
+#include <threads.h>
+
+int __pthread_key_delete(tss_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..70c4fb7
--- /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 thrd_success;
+}
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH 5/8] add the functions for mtx_t
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
` (3 preceding siblings ...)
2014-08-30 18:47 ` [PATCH 4/8] add the functions for tss_t and once_flag Jens Gustedt
@ 2014-08-30 18:47 ` Jens Gustedt
2014-08-30 18:47 ` [PATCH 6/8] add the functions for cnd_t Jens Gustedt
` (2 subsequent siblings)
7 siblings, 0 replies; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:47 UTC (permalink / raw)
To: musl
This is straightforward encapsulation of the corresponding
pthread_mutex_t functions.
---
src/thread/mtx_destroy.c | 5 +++++
src/thread/mtx_init.c | 10 ++++++++++
src/thread/mtx_lock.c | 13 +++++++++++++
src/thread/mtx_timedlock.c | 18 ++++++++++++++++++
src/thread/mtx_trylock.c | 21 +++++++++++++++++++++
src/thread/mtx_unlock.c | 9 +++++++++
6 files changed, 76 insertions(+)
create mode 100644 src/thread/mtx_destroy.c
create mode 100644 src/thread/mtx_init.c
create mode 100644 src/thread/mtx_lock.c
create mode 100644 src/thread/mtx_timedlock.c
create mode 100644 src/thread/mtx_trylock.c
create mode 100644 src/thread/mtx_unlock.c
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..a197898
--- /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 = (mtx_t) {
+ ._m_type = ((type&mtx_recursive) ? PTHREAD_MUTEX_RECURSIVE : PTHREAD_MUTEX_NORMAL),
+ };
+ return thrd_success;
+}
diff --git a/src/thread/mtx_lock.c b/src/thread/mtx_lock.c
new file mode 100644
index 0000000..4d5d75e
--- /dev/null
+++ b/src/thread/mtx_lock.c
@@ -0,0 +1,13 @@
+#include "pthread_impl.h"
+#include <threads.h>
+
+int mtx_lock(mtx_t *m)
+{
+ 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..08359d8
--- /dev/null
+++ b/src/thread/mtx_timedlock.c
@@ -0,0 +1,18 @@
+#include <threads.h>
+#include <errno.h>
+
+int __pthread_mutex_timedlock(mtx_t *restrict m, const struct timespec *restrict ts);
+
+int mtx_timedlock(mtx_t *restrict mutex, const struct timespec *restrict ts) {
+ int ret = __pthread_mutex_timedlock(mutex, ts);
+ switch (ret) {
+ /* May also return EINVAL 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. */
+ default: return thrd_error;
+ case 0: return thrd_success;
+ case ETIMEDOUT: return thrd_timedout;
+ }
+}
diff --git a/src/thread/mtx_trylock.c b/src/thread/mtx_trylock.c
new file mode 100644
index 0000000..4f55796
--- /dev/null
+++ b/src/thread/mtx_trylock.c
@@ -0,0 +1,21 @@
+#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) ? thrd_busy : thrd_success;
+
+ int ret = __pthread_mutex_trylock(m);
+ switch (ret) {
+ /* In case of UB may also return EINVAL 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. */
+ default: return thrd_error;
+ case 0: return thrd_success;
+ case EBUSY: return thrd_busy;
+ }
+}
diff --git a/src/thread/mtx_unlock.c b/src/thread/mtx_unlock.c
new file mode 100644
index 0000000..e673cd5
--- /dev/null
+++ b/src/thread/mtx_unlock.c
@@ -0,0 +1,9 @@
+#include <threads.h>
+
+int __pthread_mutex_unlock(mtx_t *);
+
+int (mtx_unlock)(mtx_t *mtx) {
+ int ret = __pthread_mutex_unlock(mtx);
+ /* In case of UB may also return EPERM. */
+ return ret ? thrd_error : thrd_success;
+}
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH 6/8] add the functions for cnd_t
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
` (4 preceding siblings ...)
2014-08-30 18:47 ` [PATCH 5/8] add the functions for mtx_t Jens Gustedt
@ 2014-08-30 18:47 ` Jens Gustedt
2014-08-31 0:35 ` Rich Felker
2014-08-30 18:47 ` [PATCH 7/8] add the thrd_xxxxxx functions Jens Gustedt
2014-08-30 18:47 ` [PATCH 8/8] Separate pthread_create and thrd_create Jens Gustedt
7 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:47 UTC (permalink / raw)
To: musl
Because of the clear separation for private pthread_cond_t these
interfaces are quite simple and direct.
---
src/thread/cnd_broadcast.c | 9 +++++++++
src/thread/cnd_destroy.c | 5 +++++
src/thread/cnd_init.c | 9 +++++++++
src/thread/cnd_signal.c | 9 +++++++++
src/thread/cnd_timedwait.c | 14 ++++++++++++++
src/thread/cnd_wait.c | 10 ++++++++++
6 files changed, 56 insertions(+)
create mode 100644 src/thread/cnd_broadcast.c
create mode 100644 src/thread/cnd_destroy.c
create mode 100644 src/thread/cnd_init.c
create mode 100644 src/thread/cnd_signal.c
create mode 100644 src/thread/cnd_timedwait.c
create mode 100644 src/thread/cnd_wait.c
diff --git a/src/thread/cnd_broadcast.c b/src/thread/cnd_broadcast.c
new file mode 100644
index 0000000..cf52f6d
--- /dev/null
+++ b/src/thread/cnd_broadcast.c
@@ -0,0 +1,9 @@
+#include <threads.h>
+
+int __private_cond_signal(cnd_t *, int);
+
+int cnd_broadcast(cnd_t * cnd) {
+ /* This internal function never fails. */
+ (void)__private_cond_signal(cnd, -1);
+ return thrd_success;
+}
diff --git a/src/thread/cnd_destroy.c b/src/thread/cnd_destroy.c
new file mode 100644
index 0000000..7c24d1a
--- /dev/null
+++ b/src/thread/cnd_destroy.c
@@ -0,0 +1,5 @@
+#include <threads.h>
+
+void (cnd_destroy)(cnd_t *cnd) {
+ /* For private cv this is a no-op */
+}
diff --git a/src/thread/cnd_init.c b/src/thread/cnd_init.c
new file mode 100644
index 0000000..c8aaee5
--- /dev/null
+++ b/src/thread/cnd_init.c
@@ -0,0 +1,9 @@
+#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..143883c
--- /dev/null
+++ b/src/thread/cnd_signal.c
@@ -0,0 +1,9 @@
+#include <threads.h>
+
+int __private_cond_signal(cnd_t *, int);
+
+int cnd_signal(cnd_t * cnd) {
+ /* This internal function never fails. */
+ (void)__private_cond_signal(cnd, 1);
+ return thrd_success;
+}
diff --git a/src/thread/cnd_timedwait.c b/src/thread/cnd_timedwait.c
new file mode 100644
index 0000000..d69a4f1
--- /dev/null
+++ b/src/thread/cnd_timedwait.c
@@ -0,0 +1,14 @@
+#include <threads.h>
+#include <errno.h>
+
+int __pthread_cond_timedwait(cnd_t *restrict c, mtx_t *restrict m, const struct timespec *restrict ts);
+
+int cnd_timedwait(cnd_t *restrict cond, mtx_t *restrict mutex, const struct timespec *restrict ts) {
+ int ret = __pthread_cond_timedwait(cond, mutex, ts);
+ switch (ret) {
+ /* May also return EINVAL or EPERM. */
+ default: return thrd_error;
+ case 0: return thrd_success;
+ case ETIMEDOUT: return thrd_timedout;
+ }
+}
diff --git a/src/thread/cnd_wait.c b/src/thread/cnd_wait.c
new file mode 100644
index 0000000..91e89db
--- /dev/null
+++ b/src/thread/cnd_wait.c
@@ -0,0 +1,10 @@
+#include <threads.h>
+
+int cnd_wait(cnd_t *cond, mtx_t *mutex)
+{
+ /* 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);
+}
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 6/8] add the functions for cnd_t
2014-08-30 18:47 ` [PATCH 6/8] add the functions for cnd_t Jens Gustedt
@ 2014-08-31 0:35 ` Rich Felker
2014-08-31 7:26 ` Jens Gustedt
0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 0:35 UTC (permalink / raw)
To: musl
On Sat, Aug 30, 2014 at 08:47:22PM +0200, Jens Gustedt wrote:
> diff --git a/src/thread/cnd_destroy.c b/src/thread/cnd_destroy.c
> new file mode 100644
> index 0000000..7c24d1a
> --- /dev/null
> +++ b/src/thread/cnd_destroy.c
> @@ -0,0 +1,5 @@
> +#include <threads.h>
> +
> +void (cnd_destroy)(cnd_t *cnd) {
> + /* For private cv this is a no-op */
> +}
Is there a reason for the parens around the name? I'm assuming it was
related to a macro definition at some point, but we don't have one now
and it seems unlikely that it would be desirable to define away the
destroy call to a NOP, in case we ever want to switch to an
implementation where NOP is not suitable.
> diff --git a/src/thread/cnd_init.c b/src/thread/cnd_init.c
> new file mode 100644
> index 0000000..c8aaee5
> --- /dev/null
> +++ b/src/thread/cnd_init.c
> @@ -0,0 +1,9 @@
> +#include <threads.h>
> +
> +int cnd_init(cnd_t * c)
> +{
> + *c = (cnd_t) {
> + 0
> + };
> + return thrd_success;
> +}
Just style, but generally we use {0} or { 0 } all on one line as a
zero-initializer. I would refrain from using multi-line unless (1)
we're using designated initializers, not just a universal zero
initializer, and (2) multi-line is actually needed for clarity or to
avoid exceeding 80 columns.
> diff --git a/src/thread/cnd_signal.c b/src/thread/cnd_signal.c
> new file mode 100644
> index 0000000..143883c
> --- /dev/null
> +++ b/src/thread/cnd_signal.c
> @@ -0,0 +1,9 @@
> +#include <threads.h>
> +
> +int __private_cond_signal(cnd_t *, int);
> +
> +int cnd_signal(cnd_t * cnd) {
> + /* This internal function never fails. */
> + (void)__private_cond_signal(cnd, 1);
Cast-to-void isn't used in musl source except when it's necessary for
types (e.g. in ?:).
> + return thrd_success;
> +}
This would be a candidate for "return thrd_success?thrd_success:ret",
I think.
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 6/8] add the functions for cnd_t
2014-08-31 0:35 ` Rich Felker
@ 2014-08-31 7:26 ` Jens Gustedt
0 siblings, 0 replies; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 7:26 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 2904 bytes --]
Am Samstag, den 30.08.2014, 20:35 -0400 schrieb Rich Felker:
> On Sat, Aug 30, 2014 at 08:47:22PM +0200, Jens Gustedt wrote:
> > diff --git a/src/thread/cnd_destroy.c b/src/thread/cnd_destroy.c
> > new file mode 100644
> > index 0000000..7c24d1a
> > --- /dev/null
> > +++ b/src/thread/cnd_destroy.c
> > @@ -0,0 +1,5 @@
> > +#include <threads.h>
> > +
> > +void (cnd_destroy)(cnd_t *cnd) {
> > + /* For private cv this is a no-op */
> > +}
>
> Is there a reason for the parens around the name? I'm assuming it was
> related to a macro definition at some point, but we don't have one now
> and it seems unlikely that it would be desirable to define away the
> destroy call to a NOP, in case we ever want to switch to an
> implementation where NOP is not suitable.
you are assuming correctly, I'll eliminate these
> > diff --git a/src/thread/cnd_init.c b/src/thread/cnd_init.c
> > new file mode 100644
> > index 0000000..c8aaee5
> > --- /dev/null
> > +++ b/src/thread/cnd_init.c
> > @@ -0,0 +1,9 @@
> > +#include <threads.h>
> > +
> > +int cnd_init(cnd_t * c)
> > +{
> > + *c = (cnd_t) {
> > + 0
> > + };
> > + return thrd_success;
> > +}
>
> Just style, but generally we use {0} or { 0 } all on one line as a
> zero-initializer. I would refrain from using multi-line unless (1)
> we're using designated initializers, not just a universal zero
> initializer, and (2) multi-line is actually needed for clarity or to
> avoid exceeding 80 columns.
yes, this time its not me but astyle that produced that. I was just
tired of cleaning up behind it.
> > diff --git a/src/thread/cnd_signal.c b/src/thread/cnd_signal.c
> > new file mode 100644
> > index 0000000..143883c
> > --- /dev/null
> > +++ b/src/thread/cnd_signal.c
> > @@ -0,0 +1,9 @@
> > +#include <threads.h>
> > +
> > +int __private_cond_signal(cnd_t *, int);
> > +
> > +int cnd_signal(cnd_t * cnd) {
> > + /* This internal function never fails. */
> > + (void)__private_cond_signal(cnd, 1);
>
> Cast-to-void isn't used in musl source except when it's necessary for
> types (e.g. in ?:).
No problem with me, I just thought that there are still compilers out
there that bark on that.
> > + return thrd_success;
> > +}
>
> This would be a candidate for "return thrd_success?thrd_success:ret",
> I think.
Hm, so this would make this a tail call. On the other hand as it is
now this states more clearly that we expect the return value of
__private_cond_signal to be irrelevant.
Ok, I'll go with the tail call and put a comment about that
expectation.
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
` (5 preceding siblings ...)
2014-08-30 18:47 ` [PATCH 6/8] add the functions for cnd_t Jens Gustedt
@ 2014-08-30 18:47 ` Jens Gustedt
2014-08-31 0:46 ` Rich Felker
2014-08-30 18:47 ` [PATCH 8/8] Separate pthread_create and thrd_create Jens Gustedt
7 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:47 UTC (permalink / raw)
To: musl
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.
---
src/sched/thrd_yield.c | 7 +++++++
src/thread/pthread_create.c | 41 ++++++++++++++++++++++++++++++++++++++---
src/thread/thrd_current.c | 11 +++++++++++
src/thread/thrd_detach.c | 12 ++++++++++++
src/thread/thrd_equal.c | 6 ++++++
src/thread/thrd_join.c | 16 ++++++++++++++++
6 files changed, 90 insertions(+), 3 deletions(-)
create mode 100644 src/sched/thrd_yield.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_join.c
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/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;
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;
+ }
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);
__release_ptc();
@@ -263,5 +285,18 @@ fail:
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/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();
+}
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;
+}
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..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;
+}
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-30 18:47 ` [PATCH 7/8] add the thrd_xxxxxx functions Jens Gustedt
@ 2014-08-31 0:46 ` Rich Felker
2014-08-31 7:57 ` Jens Gustedt
0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 0:46 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 0:46 ` Rich Felker
@ 2014-08-31 7:57 ` Jens Gustedt
2014-08-31 9:51 ` Alexander Monakov
2014-08-31 12:57 ` Rich Felker
0 siblings, 2 replies; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 7:57 UTC (permalink / raw)
To: musl
[-- 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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 7:57 ` Jens Gustedt
@ 2014-08-31 9:51 ` Alexander Monakov
2014-08-31 10:50 ` Jens Gustedt
2014-08-31 12:57 ` Rich Felker
1 sibling, 1 reply; 31+ messages in thread
From: Alexander Monakov @ 2014-08-31 9:51 UTC (permalink / raw)
To: musl
On Sun, 31 Aug 2014, Jens Gustedt wrote:
> > > @@ -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.
It probably won't be with GCC. Try:
echo 'int g(int); int f(int y){if (y) return g(0); else return g(1);}' |
gcc -xc - -S -o- -O
And try varying optimization levels. With gcc-4.8 on x86-64 I get two calls
except with -Os, and even with -Os it's worse than with a ternary operator.
Alexander
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 9:51 ` Alexander Monakov
@ 2014-08-31 10:50 ` Jens Gustedt
2014-08-31 11:06 ` Alexander Monakov
0 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 10:50 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]
Am Sonntag, den 31.08.2014, 13:51 +0400 schrieb Alexander Monakov:
> On Sun, 31 Aug 2014, Jens Gustedt wrote:
> > > > @@ -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.
>
> It probably won't be with GCC. Try:
>
> echo 'int g(int); int f(int y){if (y) return g(0); else return g(1);}' |
> gcc -xc - -S -o- -O
>
> And try varying optimization levels. With gcc-4.8 on x86-64 I get two calls
> except with -Os, and even with -Os it's worse than with a ternary operator.
Interesting remark, but your example is a bit flawed. It has the types
of the arguments for f and g the same, int. This has gcc attempt to
save things in the entry register for the first argument of the call.
As expected with the following, more complete example I see no
difference for the assembler for f or h, but for the names of the
labels. This holds for gcc and clang.
echo 'int g(void*); int a; int b; int f(int y){ if (y) return g(&a); else return g(&b);} int h(int y) { return y ? g(&a) : g(&b); }' | gcc -xc - -S -o- -O
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 10:50 ` Jens Gustedt
@ 2014-08-31 11:06 ` Alexander Monakov
2014-08-31 11:31 ` Szabolcs Nagy
0 siblings, 1 reply; 31+ messages in thread
From: Alexander Monakov @ 2014-08-31 11:06 UTC (permalink / raw)
To: musl
On Sun, 31 Aug 2014, Jens Gustedt wrote:
> Interesting remark, but your example is a bit flawed. It has the types
> of the arguments for f and g the same, int. This has gcc attempt to
> save things in the entry register for the first argument of the call.
>
> As expected with the following, more complete example I see no
> difference for the assembler for f or h, but for the names of the
> labels. This holds for gcc and clang.
>
> echo 'int g(void*); int a; int b; int f(int y){ if (y) return g(&a); else return g(&b);} int h(int y) { return y ? g(&a) : g(&b); }' | gcc -xc - -S -o- -O
No, your h() is not what is intended. It should be '{return g{y ? &a : &b);}',
and it does make a difference.
Alexander
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 11:06 ` Alexander Monakov
@ 2014-08-31 11:31 ` Szabolcs Nagy
0 siblings, 0 replies; 31+ messages in thread
From: Szabolcs Nagy @ 2014-08-31 11:31 UTC (permalink / raw)
To: musl
* Alexander Monakov <amonakov@ispras.ru> [2014-08-31 15:06:02 +0400]:
> On Sun, 31 Aug 2014, Jens Gustedt wrote:
> >
> > echo 'int g(void*); int a; int b; int f(int y){ if (y) return g(&a); else return g(&b);} int h(int y) { return y ? g(&a) : g(&b); }' | gcc -xc - -S -o- -O
>
> No, your h() is not what is intended. It should be '{return g{y ? &a : &b);}',
> and it does make a difference.
>
even with no code gen difference i find
r = f(c?x:y, u, v, w);
cleaner than
if (c)
r = f(x, u, v, w);
else
r = f(y, u, v, w);
because the first one makes it clear that only one arg is
changed based on c, the rest of the logic is the same
(maybe the c?x:y should be done on a separate line if the
call becomes too long)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 7:57 ` Jens Gustedt
2014-08-31 9:51 ` Alexander Monakov
@ 2014-08-31 12:57 ` Rich Felker
2014-08-31 13:19 ` Jens Gustedt
1 sibling, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 12:57 UTC (permalink / raw)
To: musl
On Sun, Aug 31, 2014 at 09:57:34AM +0200, Jens Gustedt wrote:
> > > 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.
Whether or not the output is identical, your code is much less
readable and maintainable: an active effort is required by the reader
to determine that the only difference between the two code paths is
the function pointer being passed. This is why I prefer the use of ?:.
The following looks perfectly readable to me:
ret = __clone(c11 ? start_c11 : start, stack, flags,
new, &new->tid, TP_ADJ(new), &new->tid);
> > Also, since it doesn't depend on anything in pthread_create.c,
>
> It does, __THRD_C11 :)
How about naming it to __ATTRP_C11_THREAD and putting it in
pthread_impl.h then?
> > 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 :)
Yes I'm aware, but I don't want gratuitous incentives for patch 8/8
just because patch 7/8 is done intentionally suboptimally. :-) If the
approaches are being compared, we should be comparing the preferred
efficient versions of both. And I'd like to wait to seriously think
about 8/8 until everything else is fully ready to commit, or
preferably already committed.
> > 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.
Do you have any thoughts on this part?
> > > 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
I'm aware that you can't cast the int* to void**, but you can still
implement the function as a trivial wrapper that doesn't introduce any
duplication of internal logic for cleaning up an exited thread:
int thrd_join(thrd_t t, int *res)
{
void *pthread_res;
__pthread_join(t, &pthread_res);
if (res) *res = (int)(intptr_t)pthread_res;
return thrd_success;
}
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 12:57 ` Rich Felker
@ 2014-08-31 13:19 ` Jens Gustedt
2014-08-31 14:05 ` Rich Felker
0 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 13:19 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 6530 bytes --]
Am Sonntag, den 31.08.2014, 08:57 -0400 schrieb Rich Felker:
> On Sun, Aug 31, 2014 at 09:57:34AM +0200, Jens Gustedt wrote:
> > > > 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.
>
> Whether or not the output is identical, your code is much less
> readable and maintainable: an active effort is required by the reader
> to determine that the only difference between the two code paths is
> the function pointer being passed. This is why I prefer the use of ?:.
>
> The following looks perfectly readable to me:
>
> ret = __clone(c11 ? start_c11 : start, stack, flags,
> new, &new->tid, TP_ADJ(new), &new->tid);
No to me (seriously!) because my builtin parser has difficulties to
capture the end of the ternary expression.
Would it be acceptable to have parenthesis around?
> > > Also, since it doesn't depend on anything in pthread_create.c,
> >
> > It does, __THRD_C11 :)
>
> How about naming it to __ATTRP_C11_THREAD and putting it in
> pthread_impl.h then?
ok, I'll do that
But hopefully we didn't overlook any possible use pattern that would
drag in different versions of the tsd symbols. I am not too
comfortable with that.
> > > 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 :)
>
> Yes I'm aware, but I don't want gratuitous incentives for patch 8/8
> just because patch 7/8 is done intentionally suboptimally. :-) If the
> approaches are being compared, we should be comparing the preferred
> efficient versions of both. And I'd like to wait to seriously think
> about 8/8 until everything else is fully ready to commit, or
> preferably already committed.
I know.
But I just discovered another such incentive :) You were right, that
the error handling for thrd_create was not correct for C11, but it
wasn't my fault :) POSIX (and thus __pthread_create) basically maps
all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
other failures.
Thus I had to integrate this difference into __pthread_create, which
was not difficult, but which intrudes even a little bit more into the
existing code.
> > > 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.
>
> Do you have any thoughts on this part?
ah, I should have deleted that in my reply, since I basically
agree. In the new version that I am preparing there are tail calls
with corresponding comments.
> > > > 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
>
> I'm aware that you can't cast the int* to void**, but you can still
> implement the function as a trivial wrapper that doesn't introduce any
> duplication of internal logic for cleaning up an exited thread:
>
> int thrd_join(thrd_t t, int *res)
> {
> void *pthread_res;
> __pthread_join(t, &pthread_res);
> if (res) *res = (int)(intptr_t)pthread_res;
> return thrd_success;
> }
dunno, doesn't look much simpler to me and drags in one more TU into C
thread applications
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 13:19 ` Jens Gustedt
@ 2014-08-31 14:05 ` Rich Felker
2014-08-31 15:07 ` Jens Gustedt
0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 14:05 UTC (permalink / raw)
To: musl
On Sun, Aug 31, 2014 at 03:19:21PM +0200, Jens Gustedt wrote:
> Am Sonntag, den 31.08.2014, 08:57 -0400 schrieb Rich Felker:
> > On Sun, Aug 31, 2014 at 09:57:34AM +0200, Jens Gustedt wrote:
> > > > > 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.
> >
> > Whether or not the output is identical, your code is much less
> > readable and maintainable: an active effort is required by the reader
> > to determine that the only difference between the two code paths is
> > the function pointer being passed. This is why I prefer the use of ?:.
> >
> > The following looks perfectly readable to me:
> >
> > ret = __clone(c11 ? start_c11 : start, stack, flags,
> > new, &new->tid, TP_ADJ(new), &new->tid);
>
> No to me (seriously!) because my builtin parser has difficulties to
> capture the end of the ternary expression.
>
> Would it be acceptable to have parenthesis around?
I wouldn't even mind if you'd prefer to rename start to start_pthread
and then make start a local variable:
start = c11 ? start_c11 : start_pthread;
ret = __clone(start, stack, flags, new,
&new->tid, TP_ADJ(new), &new->tid);
Would that be nicer?
> > > > Also, since it doesn't depend on anything in pthread_create.c,
> > >
> > > It does, __THRD_C11 :)
> >
> > How about naming it to __ATTRP_C11_THREAD and putting it in
> > pthread_impl.h then?
>
> ok, I'll do that
>
> But hopefully we didn't overlook any possible use pattern that would
> drag in different versions of the tsd symbols. I am not too
> comfortable with that.
What do you mean here? I don't follow.
> > Yes I'm aware, but I don't want gratuitous incentives for patch 8/8
> > just because patch 7/8 is done intentionally suboptimally. :-) If the
> > approaches are being compared, we should be comparing the preferred
> > efficient versions of both. And I'd like to wait to seriously think
> > about 8/8 until everything else is fully ready to commit, or
> > preferably already committed.
>
> I know.
>
> But I just discovered another such incentive :) You were right, that
> the error handling for thrd_create was not correct for C11, but it
> wasn't my fault :) POSIX (and thus __pthread_create) basically maps
> all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
> other failures.
>
> Thus I had to integrate this difference into __pthread_create, which
> was not difficult, but which intrudes even a little bit more into the
> existing code.
I think POSIX's EAGAIN is fully equivalent to C11's thrd_nomem: both
should reflect any condition where the thread could not be created due
to a resource exhaustion type failure. While you could argue that
thrd_nomem should only indicate failure of the mmap, not the clone, I
think this would be a useless distinction to callers (both of them are
fundamentally allocation operations) and then you'd be forced to use
thrd_error for clone failures, whereas I would think thrd_error should
be reserved for either erroneous usage (but that's UB anyway) or more
permanent failures (like lack of thread support on the OS).
> > > > 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.
> >
> > Do you have any thoughts on this part?
>
> ah, I should have deleted that in my reply, since I basically
> agree. In the new version that I am preparing there are tail calls
> with corresponding comments.
OK.
> > I'm aware that you can't cast the int* to void**, but you can still
> > implement the function as a trivial wrapper that doesn't introduce any
> > duplication of internal logic for cleaning up an exited thread:
> >
> > int thrd_join(thrd_t t, int *res)
> > {
> > void *pthread_res;
> > __pthread_join(t, &pthread_res);
> > if (res) *res = (int)(intptr_t)pthread_res;
> > return thrd_success;
> > }
>
> dunno, doesn't look much simpler to me and drags in one more TU into C
> thread applications
What's simpler is that this version does not:
- Need pthread_impl.h
- Have knowledge of the mechanism for waiting for thread exit.
- Have knowledge of how to obtain the exit code out of thread struct.
- Have knowledge of thread deallocation mechanism.
It does encode one piece of semi-implementation-specific knowledge,
that the C11 result code is being returned via casting the int to
void* rather than storing it in some other way. But to me that's
knowledge of the way the wrapping is being performed, which is
appropriate knowledge for a C11 threads implementation file, as
opposed to knowledge of how the underlying pthreads implementation
works, which is "inappropriate knowledge". Please note that I'm not
fully against the latter when there's a good reason for it (like
making something more efficient or less ugly), but I don't like it to
be there gratuitously. Consider what would happen if we were switching
to a mechanism like glibc does of caching and reusing thread stacks
after exit (not something I'd want to do, but a good example); then
we'd have two places to update the code with much more complex logic,
rather than just one. Or as another example that may be more
reasonable, consider that we might want to make exiting threads do
their own unmapping of all but the 1 (or 2 when it straddles) page
containing the struct __pthread, so that memory isn't tied up when
there's a long gap between pthread_exit and pthread_join, and only
leave unmapping of that last page (or 2) to the joining thread. This
would also potentially require big changes in two places with your
approach.
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 14:05 ` Rich Felker
@ 2014-08-31 15:07 ` Jens Gustedt
2014-08-31 16:06 ` Rich Felker
0 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 15:07 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 6491 bytes --]
Am Sonntag, den 31.08.2014, 10:05 -0400 schrieb Rich Felker:
> On Sun, Aug 31, 2014 at 03:19:21PM +0200, Jens Gustedt wrote:
> > Am Sonntag, den 31.08.2014, 08:57 -0400 schrieb Rich Felker:
> > > On Sun, Aug 31, 2014 at 09:57:34AM +0200, Jens Gustedt wrote:
> > > > > > 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.
> > >
> > > Whether or not the output is identical, your code is much less
> > > readable and maintainable: an active effort is required by the reader
> > > to determine that the only difference between the two code paths is
> > > the function pointer being passed. This is why I prefer the use of ?:.
> > >
> > > The following looks perfectly readable to me:
> > >
> > > ret = __clone(c11 ? start_c11 : start, stack, flags,
> > > new, &new->tid, TP_ADJ(new), &new->tid);
> >
> > No to me (seriously!) because my builtin parser has difficulties to
> > capture the end of the ternary expression.
> >
> > Would it be acceptable to have parenthesis around?
>
> I wouldn't even mind if you'd prefer to rename start to start_pthread
> and then make start a local variable:
>
> start = c11 ? start_c11 : start_pthread;
> ret = __clone(start, stack, flags, new,
> &new->tid, TP_ADJ(new), &new->tid);
>
> Would that be nicer?
I opted for the parenthesis version, now.
> > > > > Also, since it doesn't depend on anything in pthread_create.c,
> > > >
> > > > It does, __THRD_C11 :)
> > >
> > > How about naming it to __ATTRP_C11_THREAD and putting it in
> > > pthread_impl.h then?
> >
> > ok, I'll do that
> >
> > But hopefully we didn't overlook any possible use pattern that would
> > drag in different versions of the tsd symbols. I am not too
> > comfortable with that.
>
> What do you mean here? I don't follow.
When trying to separate the two implementations, for a while I
struggled with the fact that the game with the dummy functions
enforces that pthread_exit and pthread_create must be in the same
TU. I found that difficult to deal with, not knowing the code and the
interaction between the different TU too well.
(There might be an independent possibility of improvement in
readability and maintainability in separating pthread_create and
pthread_exit more cleanly.)
> > > Yes I'm aware, but I don't want gratuitous incentives for patch 8/8
> > > just because patch 7/8 is done intentionally suboptimally. :-) If the
> > > approaches are being compared, we should be comparing the preferred
> > > efficient versions of both. And I'd like to wait to seriously think
> > > about 8/8 until everything else is fully ready to commit, or
> > > preferably already committed.
> >
> > I know.
> >
> > But I just discovered another such incentive :) You were right, that
> > the error handling for thrd_create was not correct for C11, but it
> > wasn't my fault :) POSIX (and thus __pthread_create) basically maps
> > all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
> > other failures.
> >
> > Thus I had to integrate this difference into __pthread_create, which
> > was not difficult, but which intrudes even a little bit more into the
> > existing code.
>
> I think POSIX's EAGAIN is fully equivalent to C11's thrd_nomem: both
> should reflect any condition where the thread could not be created due
> to a resource exhaustion type failure. While you could argue that
> thrd_nomem should only indicate failure of the mmap, not the clone, I
> think this would be a useless distinction to callers (both of them are
> fundamentally allocation operations) and then you'd be forced to use
> thrd_error for clone failures, whereas I would think thrd_error should
> be reserved for either erroneous usage (but that's UB anyway) or more
> permanent failures (like lack of thread support on the OS).
Having read up a bit, now, I don't think so, for C threads this
mapping is not correct. clone has several different error returns
that the actual code correctly maps to EAGAIN, but among them it also
has ENOMEM.
So we have possible ENOMEM coming from clone and from mmap, and a
bunch of other obscure errors that should go to thrd_error.
> > > I'm aware that you can't cast the int* to void**, but you can still
> > > implement the function as a trivial wrapper that doesn't introduce any
> > > duplication of internal logic for cleaning up an exited thread:
> > >
> > > int thrd_join(thrd_t t, int *res)
> > > {
> > > void *pthread_res;
> > > __pthread_join(t, &pthread_res);
> > > if (res) *res = (int)(intptr_t)pthread_res;
> > > return thrd_success;
> > > }
> >
> > dunno, doesn't look much simpler to me and drags in one more TU into C
> > thread applications
>
> What's simpler is that this version does not:
>
> - Need pthread_impl.h
> - Have knowledge of the mechanism for waiting for thread exit.
> - Have knowledge of how to obtain the exit code out of thread struct.
> - Have knowledge of thread deallocation mechanism.
Ok, I'll move that separated implementation to patch 8, and use your
version for patch 7.
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 15:07 ` Jens Gustedt
@ 2014-08-31 16:06 ` Rich Felker
2014-08-31 16:36 ` Jens Gustedt
0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 16:06 UTC (permalink / raw)
To: musl
On Sun, Aug 31, 2014 at 05:07:22PM +0200, Jens Gustedt wrote:
> > > > > > Also, since it doesn't depend on anything in pthread_create.c,
> > > > >
> > > > > It does, __THRD_C11 :)
> > > >
> > > > How about naming it to __ATTRP_C11_THREAD and putting it in
> > > > pthread_impl.h then?
> > >
> > > ok, I'll do that
> > >
> > > But hopefully we didn't overlook any possible use pattern that would
> > > drag in different versions of the tsd symbols. I am not too
> > > comfortable with that.
> >
> > What do you mean here? I don't follow.
>
> When trying to separate the two implementations, for a while I
> struggled with the fact that the game with the dummy functions
> enforces that pthread_exit and pthread_create must be in the same
> TU. I found that difficult to deal with, not knowing the code and the
> interaction between the different TU too well.
>
> (There might be an independent possibility of improvement in
> readability and maintainability in separating pthread_create and
> pthread_exit more cleanly.)
pthread_create inherently pulls in pthread_exit because the start
function calls the latter. The other direction is not inherent -- for
example, a single-threaded program could in principle exit via a
direct call to pthread_exit or cancellation of pthread_self().
Regarding the tsd weak symbols, the logic is slightly complicated. The
main thread's tsd pointer can be initialized during the first
pthread_create call, or by pthread_key_create; it merely needs to be
valid before any call to pthread_setspecific or pthread_getspecific.
I'd kind of like to get rid of the initialization path in
pthread_create, but the one in pthread_key_create cannot find the main
thread unless the main thread is the only thread in existence when
it's called.
In any case, my quick analysis of the weak symbols leads me to believe
it's fine to split the ones used by pthread_exit and pthread_create
into separate TUs, but I'm still not convinced this is terribly useful
to do.
> > > But I just discovered another such incentive :) You were right, that
> > > the error handling for thrd_create was not correct for C11, but it
> > > wasn't my fault :) POSIX (and thus __pthread_create) basically maps
> > > all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
> > > other failures.
> > >
> > > Thus I had to integrate this difference into __pthread_create, which
> > > was not difficult, but which intrudes even a little bit more into the
> > > existing code.
> >
> > I think POSIX's EAGAIN is fully equivalent to C11's thrd_nomem: both
> > should reflect any condition where the thread could not be created due
> > to a resource exhaustion type failure. While you could argue that
> > thrd_nomem should only indicate failure of the mmap, not the clone, I
> > think this would be a useless distinction to callers (both of them are
> > fundamentally allocation operations) and then you'd be forced to use
> > thrd_error for clone failures, whereas I would think thrd_error should
> > be reserved for either erroneous usage (but that's UB anyway) or more
> > permanent failures (like lack of thread support on the OS).
>
> Having read up a bit, now, I don't think so, for C threads this
> mapping is not correct. clone has several different error returns
> that the actual code correctly maps to EAGAIN, but among them it also
> has ENOMEM.
>
> So we have possible ENOMEM coming from clone and from mmap, and a
> bunch of other obscure errors that should go to thrd_error.
Like what? I see no possible errors except EAGAIN and ENOMEM. The only
others listed in the man page are EINVAL and EPERM and they pertain to
invalid arguments that aren't being used by pthread_create.
> > > > I'm aware that you can't cast the int* to void**, but you can still
> > > > implement the function as a trivial wrapper that doesn't introduce any
> > > > duplication of internal logic for cleaning up an exited thread:
> > > >
> > > > int thrd_join(thrd_t t, int *res)
> > > > {
> > > > void *pthread_res;
> > > > __pthread_join(t, &pthread_res);
> > > > if (res) *res = (int)(intptr_t)pthread_res;
> > > > return thrd_success;
> > > > }
> > >
> > > dunno, doesn't look much simpler to me and drags in one more TU into C
> > > thread applications
> >
> > What's simpler is that this version does not:
> >
> > - Need pthread_impl.h
> > - Have knowledge of the mechanism for waiting for thread exit.
> > - Have knowledge of how to obtain the exit code out of thread struct.
> > - Have knowledge of thread deallocation mechanism.
>
> Ok, I'll move that separated implementation to patch 8, and use your
> version for patch 7.
OK.
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 16:06 ` Rich Felker
@ 2014-08-31 16:36 ` Jens Gustedt
2014-08-31 17:02 ` Rich Felker
0 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 16:36 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 5001 bytes --]
Am Sonntag, den 31.08.2014, 12:06 -0400 schrieb Rich Felker:
> On Sun, Aug 31, 2014 at 05:07:22PM +0200, Jens Gustedt wrote:
> > > > > > > Also, since it doesn't depend on anything in pthread_create.c,
> > > > > >
> > > > > > It does, __THRD_C11 :)
> > > > >
> > > > > How about naming it to __ATTRP_C11_THREAD and putting it in
> > > > > pthread_impl.h then?
> > > >
> > > > ok, I'll do that
> > > >
> > > > But hopefully we didn't overlook any possible use pattern that would
> > > > drag in different versions of the tsd symbols. I am not too
> > > > comfortable with that.
> > >
> > > What do you mean here? I don't follow.
> >
> > When trying to separate the two implementations, for a while I
> > struggled with the fact that the game with the dummy functions
> > enforces that pthread_exit and pthread_create must be in the same
> > TU. I found that difficult to deal with, not knowing the code and the
> > interaction between the different TU too well.
> >
> > (There might be an independent possibility of improvement in
> > readability and maintainability in separating pthread_create and
> > pthread_exit more cleanly.)
>
> pthread_create inherently pulls in pthread_exit because the start
> function calls the latter. The other direction is not inherent -- for
> example, a single-threaded program could in principle exit via a
> direct call to pthread_exit or cancellation of pthread_self().
so such a hypothetical application would gain some bytes if we'd
separate them :)
> Regarding the tsd weak symbols, the logic is slightly complicated. The
> main thread's tsd pointer can be initialized during the first
> pthread_create call, or by pthread_key_create; it merely needs to be
> valid before any call to pthread_setspecific or pthread_getspecific.
> I'd kind of like to get rid of the initialization path in
> pthread_create, but the one in pthread_key_create cannot find the main
> thread unless the main thread is the only thread in existence when
> it's called.
yes
In any case patch 8 assembles all code that is needed for tsd in one
TU, pthread_exit.c, and should otherwise be no modification to the
existing pthread code.
> In any case, my quick analysis of the weak symbols leads me to believe
> it's fine to split the ones used by pthread_exit and pthread_create
> into separate TUs, but I'm still not convinced this is terribly useful
> to do.
By itself perhaps not, we'll see. As a preparatory step if we want to
separate the two implementations, more likely.
> > > > But I just discovered another such incentive :) You were right, that
> > > > the error handling for thrd_create was not correct for C11, but it
> > > > wasn't my fault :) POSIX (and thus __pthread_create) basically maps
> > > > all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
> > > > other failures.
> > > >
> > > > Thus I had to integrate this difference into __pthread_create, which
> > > > was not difficult, but which intrudes even a little bit more into the
> > > > existing code.
> > >
> > > I think POSIX's EAGAIN is fully equivalent to C11's thrd_nomem: both
> > > should reflect any condition where the thread could not be created due
> > > to a resource exhaustion type failure. While you could argue that
> > > thrd_nomem should only indicate failure of the mmap, not the clone, I
> > > think this would be a useless distinction to callers (both of them are
> > > fundamentally allocation operations) and then you'd be forced to use
> > > thrd_error for clone failures, whereas I would think thrd_error should
> > > be reserved for either erroneous usage (but that's UB anyway) or more
> > > permanent failures (like lack of thread support on the OS).
> >
> > Having read up a bit, now, I don't think so, for C threads this
> > mapping is not correct. clone has several different error returns
> > that the actual code correctly maps to EAGAIN, but among them it also
> > has ENOMEM.
> >
> > So we have possible ENOMEM coming from clone and from mmap, and a
> > bunch of other obscure errors that should go to thrd_error.
>
> Like what? I see no possible errors except EAGAIN and ENOMEM. The only
> others listed in the man page are EINVAL and EPERM and they pertain to
> invalid arguments that aren't being used by pthread_create.
(I withdraw the "bunch of")
EAGAIN from clone is clearly a distinct error condition than when it
is on ENOMEM. I would not see it covered by what C11 expects as that
error condition. So the EAGAIN from clone should go to thrd_error, I
think, and not be merged with ENOMEM for the C threads implementation.
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 16:36 ` Jens Gustedt
@ 2014-08-31 17:02 ` Rich Felker
2014-08-31 19:10 ` Jens Gustedt
0 siblings, 1 reply; 31+ messages in thread
From: Rich Felker @ 2014-08-31 17:02 UTC (permalink / raw)
To: musl
On Sun, Aug 31, 2014 at 06:36:00PM +0200, Jens Gustedt wrote:
> > > > > But I just discovered another such incentive :) You were right, that
> > > > > the error handling for thrd_create was not correct for C11, but it
> > > > > wasn't my fault :) POSIX (and thus __pthread_create) basically maps
> > > > > all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
> > > > > other failures.
> > > > >
> > > > > Thus I had to integrate this difference into __pthread_create, which
> > > > > was not difficult, but which intrudes even a little bit more into the
> > > > > existing code.
> > > >
> > > > I think POSIX's EAGAIN is fully equivalent to C11's thrd_nomem: both
> > > > should reflect any condition where the thread could not be created due
> > > > to a resource exhaustion type failure. While you could argue that
> > > > thrd_nomem should only indicate failure of the mmap, not the clone, I
> > > > think this would be a useless distinction to callers (both of them are
> > > > fundamentally allocation operations) and then you'd be forced to use
> > > > thrd_error for clone failures, whereas I would think thrd_error should
> > > > be reserved for either erroneous usage (but that's UB anyway) or more
> > > > permanent failures (like lack of thread support on the OS).
> > >
> > > Having read up a bit, now, I don't think so, for C threads this
> > > mapping is not correct. clone has several different error returns
> > > that the actual code correctly maps to EAGAIN, but among them it also
> > > has ENOMEM.
> > >
> > > So we have possible ENOMEM coming from clone and from mmap, and a
> > > bunch of other obscure errors that should go to thrd_error.
> >
> > Like what? I see no possible errors except EAGAIN and ENOMEM. The only
> > others listed in the man page are EINVAL and EPERM and they pertain to
> > invalid arguments that aren't being used by pthread_create.
>
> (I withdraw the "bunch of")
>
> EAGAIN from clone is clearly a distinct error condition than when it
> is on ENOMEM. I would not see it covered by what C11 expects as that
> error condition. So the EAGAIN from clone should go to thrd_error, I
> think, and not be merged with ENOMEM for the C threads implementation.
I disagree here, at least unless you have a convincing argument for
this. EAGAIN from clone reflects a condition where a resource could
not be allocated. The reason (policy or inherent limits on a
particular type of resource, as opposed to just generally running out
of memory) doesn't seem to be relevant to applications, and I don't
see how thrd_error is any more appropriate than thrd_nomem for
reporting it. Certainly there are other cases where one type of
allocation (e.g. mmap for a thread) might fail where another (e.g.
malloc reusing freed memory on the heap) might fail, so I don't think
you can say that thrd_nomem is inappropriate if malloc would succeed.
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 17:02 ` Rich Felker
@ 2014-08-31 19:10 ` Jens Gustedt
2014-09-01 0:04 ` Rich Felker
0 siblings, 1 reply; 31+ messages in thread
From: Jens Gustedt @ 2014-08-31 19:10 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]
Am Sonntag, den 31.08.2014, 13:02 -0400 schrieb Rich Felker:
> On Sun, Aug 31, 2014 at 06:36:00PM +0200, Jens Gustedt wrote:
> > EAGAIN from clone is clearly a distinct error condition than when it
> > is on ENOMEM. I would not see it covered by what C11 expects as that
> > error condition. So the EAGAIN from clone should go to thrd_error, I
> > think, and not be merged with ENOMEM for the C threads implementation.
>
> I disagree here, at least unless you have a convincing argument for
> this. EAGAIN from clone reflects a condition where a resource could
> not be allocated. The reason (policy or inherent limits on a
> particular type of resource, as opposed to just generally running out
> of memory) doesn't seem to be relevant to applications, and I don't
> see how thrd_error is any more appropriate than thrd_nomem for
> reporting it. Certainly there are other cases where one type of
> allocation (e.g. mmap for a thread) might fail where another (e.g.
> malloc reusing freed memory on the heap) might fail, so I don't think
> you can say that thrd_nomem is inappropriate if malloc would succeed.
The convincing arguments come from the documentation, I'd say. C11
says
The thrd_create function returns
thrd_success on success, or
thrd_nomem if no memory could be allocated for the thread requested, or
thrd_error if the request could not be honored.
(If this is a reasonable distinction of errors is a completely
different question.)
The man page for clone (whatever authority this has) says, among other
things
EAGAIN Too many processes are already running.
ENOMEM Cannot allocate sufficient memory to allocate a task
structure for the child, or to copy those parts of the
caller's context that need to be copied.
I have difficulties to read "lack of other resources" into the case
for thrd_nomem. I see that EAGAIN is relatively well covered by
thrd_error, but not very well by thrd_nomem. thrd_nomem is the
catch-all-the-rest error, so I think that C11 permits to map EAGAIN
together with eventually happening other exotic stuff on on it.
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 --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 7/8] add the thrd_xxxxxx functions
2014-08-31 19:10 ` Jens Gustedt
@ 2014-09-01 0:04 ` Rich Felker
0 siblings, 0 replies; 31+ messages in thread
From: Rich Felker @ 2014-09-01 0:04 UTC (permalink / raw)
To: musl
On Sun, Aug 31, 2014 at 09:10:46PM +0200, Jens Gustedt wrote:
> Am Sonntag, den 31.08.2014, 13:02 -0400 schrieb Rich Felker:
> > On Sun, Aug 31, 2014 at 06:36:00PM +0200, Jens Gustedt wrote:
> > > EAGAIN from clone is clearly a distinct error condition than when it
> > > is on ENOMEM. I would not see it covered by what C11 expects as that
> > > error condition. So the EAGAIN from clone should go to thrd_error, I
> > > think, and not be merged with ENOMEM for the C threads implementation.
> >
> > I disagree here, at least unless you have a convincing argument for
> > this. EAGAIN from clone reflects a condition where a resource could
> > not be allocated. The reason (policy or inherent limits on a
> > particular type of resource, as opposed to just generally running out
> > of memory) doesn't seem to be relevant to applications, and I don't
> > see how thrd_error is any more appropriate than thrd_nomem for
> > reporting it. Certainly there are other cases where one type of
> > allocation (e.g. mmap for a thread) might fail where another (e.g.
> > malloc reusing freed memory on the heap) might fail, so I don't think
> > you can say that thrd_nomem is inappropriate if malloc would succeed.
>
> The convincing arguments come from the documentation, I'd say. C11
> says
>
> The thrd_create function returns
>
> thrd_success on success, or
>
> thrd_nomem if no memory could be allocated for the thread requested, or
>
> thrd_error if the request could not be honored.
>
> (If this is a reasonable distinction of errors is a completely
> different question.)
>
> The man page for clone (whatever authority this has) says, among other
> things
>
> EAGAIN Too many processes are already running.
>
> ENOMEM Cannot allocate sufficient memory to allocate a task
> structure for the child, or to copy those parts of the
> caller's context that need to be copied.
>
> I have difficulties to read "lack of other resources" into the case
> for thrd_nomem. I see that EAGAIN is relatively well covered by
> thrd_error, but not very well by thrd_nomem. thrd_nomem is the
> catch-all-the-rest error, so I think that C11 permits to map EAGAIN
> together with eventually happening other exotic stuff on on it.
It should be noted that mmap/mprotect, as used by pthread_create, can
experience ENOMEM without memory exhaustion, as a result of a separate
limit: the kernel's limit on the number of VMAs a process can have. By
default I think this is something like 64k, and each thread with a
guard page uses two. I don't see how hitting the VMA limit is
significantly different than hidding the kernel task ("process")
limit. The reason I bring this up is because the VMA limit is
indistinguishable, at the syscall level, from memory exhaustion --
both cases give ENOMEM.
In both cases -- EAGAIN for pthread_create and thrd_nomem for
thrd_create, the error tells the application something useful: that
the failure is potentially transient. Whether it's actually transient
depends a lot on the application; if the app has a lot of memory tied
up that won't be freed asynchronously, then it's unlikely to be
transient, but if the app has threads responding to asynchronous
events (like requests from the network) it's very possible that the
needed resource will be available as soon as some of them complete.
Rich
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 8/8] Separate pthread_create and thrd_create
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
` (6 preceding siblings ...)
2014-08-30 18:47 ` [PATCH 7/8] add the thrd_xxxxxx functions Jens Gustedt
@ 2014-08-30 18:47 ` Jens Gustedt
7 siblings, 0 replies; 31+ messages in thread
From: Jens Gustedt @ 2014-08-30 18:47 UTC (permalink / raw)
To: musl
This moves the core of pthread_exit from pthread_create.c to its own TU,
pthread_exit.c. This is what the two thread implementations really have
to share.
The "create" functions that each have their own TU.
This is less intrusive for both thread models. With that, pthread goes
back to completely equivalent code than before the merge, only that
pthread_exit is in a different TU.
For C threads this also cuts off some dead branches in thrd_create.
---
src/thread/pthread_create.c | 166 +++----------------------------------------
src/thread/pthread_detach.c | 6 +-
src/thread/pthread_exit.c | 130 +++++++++++++++++++++++++++++++++
src/thread/pthread_join.c | 4 +-
src/thread/thrd_create.c | 98 +++++++++++++++++++++++++
src/thread/thrd_detach.c | 14 ++--
6 files changed, 246 insertions(+), 172 deletions(-)
create mode 100644 src/thread/pthread_exit.c
create mode 100644 src/thread/thrd_create.c
diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index 3212502..193b295 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -9,104 +9,16 @@
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)
+void __thread_enable(void);
+_Noreturn void __pthread_exit(void *);
+void *__copy_tls(unsigned char *);
+extern volatile size_t __pthread_tsd_size;
static void dummy_0()
{
}
weak_alias(dummy_0, __acquire_ptc);
weak_alias(dummy_0, __release_ptc);
-weak_alias(dummy_0, __pthread_tsd_run_dtors);
-weak_alias(dummy_0, __do_private_robust_list);
-weak_alias(dummy_0, __do_orphaned_stdio_locks);
-
-_Noreturn void __pthread_exit(void *result)
-{
- pthread_t self = __pthread_self();
- sigset_t set;
-
- self->result = result;
-
- while (self->cancelbuf) {
- void (*f)(void *) = self->cancelbuf->__f;
- void *x = self->cancelbuf->__x;
- self->cancelbuf = self->cancelbuf->__next;
- f(x);
- }
-
- __pthread_tsd_run_dtors();
-
- __lock(self->exitlock);
-
- /* Mark this thread dead before decrementing count */
- __lock(self->killlock);
- self->dead = 1;
-
- /* Block all signals before decrementing the live thread count.
- * This is important to ensure that dynamically allocated TLS
- * is not under-allocated/over-committed, and possibly for other
- * reasons as well. */
- __block_all_sigs(&set);
-
- /* Wait to unlock the kill lock, which governs functions like
- * pthread_kill which target a thread id, until signals have
- * been blocked. This precludes observation of the thread id
- * as a live thread (with application code running in it) after
- * the thread was reported dead by ESRCH being returned. */
- __unlock(self->killlock);
-
- /* It's impossible to determine whether this is "the last thread"
- * until performing the atomic decrement, since multiple threads
- * could exit at the same time. For the last thread, revert the
- * decrement and unblock signals to give the atexit handlers and
- * stdio cleanup code a consistent state. */
- if (a_fetch_add(&libc.threads_minus_1, -1)==0) {
- libc.threads_minus_1 = 0;
- __restore_sigs(&set);
- exit(0);
- }
-
- if (self->locale != &libc.global_locale) {
- a_dec(&libc.uselocale_cnt);
- if (self->locale->ctype_utf8)
- a_dec(&libc.bytelocale_cnt_minus_1);
- }
-
- __do_private_robust_list();
- __do_orphaned_stdio_locks();
-
- if (self->detached && self->map_base) {
- /* Detached threads must avoid the kernel clear_child_tid
- * feature, since the virtual address will have been
- * unmapped and possibly already reused by a new mapping
- * at the time the kernel would perform the write. In
- * the case of threads that started out detached, the
- * initial clone flags are correct, but if the thread was
- * detached later (== 2), we need to clear it here. */
- if (self->detached == 2) __syscall(SYS_set_tid_address, 0);
-
- /* The following call unmaps the thread's stack mapping
- * and then exits without touching the stack. */
- __unmapself(self->map_base, self->map_size);
- }
-
- for (;;) __syscall(SYS_exit, 0);
-}
-
-void __do_cleanup_push(struct __ptcb *cb)
-{
- if (!libc.has_thread_pointer) return;
- struct pthread *self = __pthread_self();
- cb->__next = self->cancelbuf;
- self->cancelbuf = cb;
-}
-
-void __do_cleanup_pop(struct __ptcb *cb)
-{
- if (!libc.has_thread_pointer) return;
- __pthread_self()->cancelbuf = cb->__next;
-}
static int start(void *p)
{
@@ -126,42 +38,11 @@ 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 */
-static volatile size_t dummy = 0;
-weak_alias(dummy, __pthread_tsd_size);
-static void *dummy_tsd[1] = { 0 };
-weak_alias(dummy_tsd, __pthread_tsd_main);
-
-static FILE *volatile dummy_file = 0;
-weak_alias(dummy_file, __stdin_used);
-weak_alias(dummy_file, __stdout_used);
-weak_alias(dummy_file, __stderr_used);
-
-static void init_file_lock(FILE *f)
+int pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp, void *(*entry)(void *), void *restrict arg)
{
- if (f && f->lock<0) f->lock = 0;
-}
-
-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, c11 = (attrp == __THRD_C11);
+ int ret;
size_t size, guard = 0;
struct pthread *self, *new;
unsigned char *map = 0, *stack = 0, *tsd = 0, *stack_limit;
@@ -173,19 +54,7 @@ static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restr
if (!libc.can_do_threads) return ENOSYS;
self = __pthread_self();
- if (!libc.threaded) {
- for (FILE *f=libc.ofl_head; f; f=f->next)
- init_file_lock(f);
- init_file_lock(__stdin_used);
- init_file_lock(__stdout_used);
- init_file_lock(__stderr_used);
- __syscall(SYS_rt_sigprocmask, SIG_UNBLOCK, SIGPT_SET, 0, _NSIG/8);
- self->tsd = (void **)__pthread_tsd_main;
- libc.threaded = 1;
- }
- if (c11) {
- attrp = 0;
- }
+ if (!libc.threaded) __thread_enable();
if (attrp) attr = *attrp;
__acquire_ptc();
@@ -253,10 +122,7 @@ static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restr
new->canary = self->canary;
a_inc(&libc.threads_minus_1);
- 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);
+ ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
__release_ptc();
@@ -284,19 +150,3 @@ 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 5ca7855..e8ec758 100644
--- a/src/thread/pthread_detach.c
+++ b/src/thread/pthread_detach.c
@@ -1,15 +1,11 @@
#include "pthread_impl.h"
-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_exit.c b/src/thread/pthread_exit.c
new file mode 100644
index 0000000..4f51269
--- /dev/null
+++ b/src/thread/pthread_exit.c
@@ -0,0 +1,130 @@
+#define _GNU_SOURCE
+#include "pthread_impl.h"
+#include "stdio_impl.h"
+#include "libc.h"
+#include <sys/mman.h>
+#include <threads.h>
+
+static void dummy_0()
+{
+}
+weak_alias(dummy_0, __pthread_tsd_run_dtors);
+weak_alias(dummy_0, __do_private_robust_list);
+weak_alias(dummy_0, __do_orphaned_stdio_locks);
+
+_Noreturn void __pthread_exit(void *result)
+{
+ pthread_t self = __pthread_self();
+ sigset_t set;
+
+ self->result = result;
+
+ while (self->cancelbuf) {
+ void (*f)(void *) = self->cancelbuf->__f;
+ void *x = self->cancelbuf->__x;
+ self->cancelbuf = self->cancelbuf->__next;
+ f(x);
+ }
+
+ __pthread_tsd_run_dtors();
+
+ __lock(self->exitlock);
+
+ /* Mark this thread dead before decrementing count */
+ __lock(self->killlock);
+ self->dead = 1;
+
+ /* Block all signals before decrementing the live thread count.
+ * This is important to ensure that dynamically allocated TLS
+ * is not under-allocated/over-committed, and possibly for other
+ * reasons as well. */
+ __block_all_sigs(&set);
+
+ /* Wait to unlock the kill lock, which governs functions like
+ * pthread_kill which target a thread id, until signals have
+ * been blocked. This precludes observation of the thread id
+ * as a live thread (with application code running in it) after
+ * the thread was reported dead by ESRCH being returned. */
+ __unlock(self->killlock);
+
+ /* It's impossible to determine whether this is "the last thread"
+ * until performing the atomic decrement, since multiple threads
+ * could exit at the same time. For the last thread, revert the
+ * decrement and unblock signals to give the atexit handlers and
+ * stdio cleanup code a consistent state. */
+ if (a_fetch_add(&libc.threads_minus_1, -1)==0) {
+ libc.threads_minus_1 = 0;
+ __restore_sigs(&set);
+ exit(0);
+ }
+
+ if (self->locale != &libc.global_locale) {
+ a_dec(&libc.uselocale_cnt);
+ if (self->locale->ctype_utf8)
+ a_dec(&libc.bytelocale_cnt_minus_1);
+ }
+
+ __do_private_robust_list();
+ __do_orphaned_stdio_locks();
+
+ if (self->detached && self->map_base) {
+ /* Detached threads must avoid the kernel clear_child_tid
+ * feature, since the virtual address will have been
+ * unmapped and possibly already reused by a new mapping
+ * at the time the kernel would perform the write. In
+ * the case of threads that started out detached, the
+ * initial clone flags are correct, but if the thread was
+ * detached later (== 2), we need to clear it here. */
+ if (self->detached == 2) __syscall(SYS_set_tid_address, 0);
+
+ /* The following call unmaps the thread's stack mapping
+ * and then exits without touching the stack. */
+ __unmapself(self->map_base, self->map_size);
+ }
+
+ for (;;) __syscall(SYS_exit, 0);
+}
+
+void __do_cleanup_push(struct __ptcb *cb)
+{
+ if (!libc.has_thread_pointer) return;
+ struct pthread *self = __pthread_self();
+ cb->__next = self->cancelbuf;
+ self->cancelbuf = cb;
+}
+
+void __do_cleanup_pop(struct __ptcb *cb)
+{
+ if (!libc.has_thread_pointer) return;
+ __pthread_self()->cancelbuf = cb->__next;
+}
+
+/* pthread_key_create.c overrides this */
+static volatile size_t dummy = 0;
+weak_alias(dummy, __pthread_tsd_size);
+static void *dummy_tsd[1] = { 0 };
+weak_alias(dummy_tsd, __pthread_tsd_main);
+
+static FILE *volatile dummy_file = 0;
+weak_alias(dummy_file, __stdin_used);
+weak_alias(dummy_file, __stdout_used);
+weak_alias(dummy_file, __stderr_used);
+
+static void init_file_lock(FILE *f)
+{
+ if (f && f->lock<0) f->lock = 0;
+}
+
+void __thread_enable(void)
+{
+ for (FILE *f=libc.ofl_head; f; f=f->next)
+ init_file_lock(f);
+ init_file_lock(__stdin_used);
+ init_file_lock(__stdout_used);
+ init_file_lock(__stderr_used);
+ __syscall(SYS_rt_sigprocmask, SIG_UNBLOCK, SIGPT_SET, 0, _NSIG/8);
+ __pthread_self()->tsd = (void **)__pthread_tsd_main;
+ libc.threaded = 1;
+}
+
+weak_alias(__pthread_exit, pthread_exit);
diff --git a/src/thread/pthread_join.c b/src/thread/pthread_join.c
index bca89aa..23efbf0 100644
--- a/src/thread/pthread_join.c
+++ b/src/thread/pthread_join.c
@@ -7,7 +7,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);
@@ -15,5 +15,3 @@ 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/thrd_create.c b/src/thread/thrd_create.c
new file mode 100644
index 0000000..f72f992
--- /dev/null
+++ b/src/thread/thrd_create.c
@@ -0,0 +1,98 @@
+#define _GNU_SOURCE
+#include "pthread_impl.h"
+#include "stdio_impl.h"
+#include "libc.h"
+#include <sys/mman.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);
+void __thread_enable(void);
+_Noreturn void __pthread_exit(void *);
+void *__copy_tls(unsigned char *);
+extern volatile size_t __pthread_tsd_size;
+
+_Noreturn void thrd_exit(int result) {
+ __pthread_exit((void*)(intptr_t)result);
+}
+
+static void dummy_0()
+{
+}
+weak_alias(dummy_0, __acquire_ptc);
+weak_alias(dummy_0, __release_ptc);
+
+static int start(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)
+
+int thrd_create(thrd_t *res, thrd_start_t entry, void *arg)
+{
+ int ret = -ENOMEM;
+ size_t guard = ROUND(DEFAULT_GUARD_SIZE);
+ size_t size = guard + ROUND(DEFAULT_STACK_SIZE + libc.tls_size + __pthread_tsd_size);
+ struct pthread *self, *new;
+ unsigned char *map, *stack, *tsd, *stack_limit;
+ unsigned flags = CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND
+ | CLONE_THREAD | CLONE_SYSVSEM | CLONE_SETTLS
+ | CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID | CLONE_DETACHED;
+
+ if (!libc.can_do_threads) return thrd_error;
+ self = __pthread_self();
+ if (!libc.threaded) __thread_enable();
+
+ __acquire_ptc();
+
+ if (guard) {
+ map = __mmap(0, size, PROT_NONE, MAP_PRIVATE|MAP_ANON, -1, 0);
+ if (map == MAP_FAILED) goto CLEANUP;
+ if (__mprotect(map+guard, size-guard, PROT_READ|PROT_WRITE)) {
+ __munmap(map, size);
+ goto CLEANUP;
+ }
+ } else {
+ map = __mmap(0, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
+ if (map == MAP_FAILED) goto CLEANUP;
+ }
+ tsd = map + size - __pthread_tsd_size;
+ stack = tsd - libc.tls_size;
+ stack_limit = map + guard;
+
+ new = __copy_tls(tsd - libc.tls_size);
+ new->map_base = map;
+ new->map_size = size;
+ new->stack = stack;
+ new->stack_size = stack - stack_limit;
+ new->start = (void *(*)(void*))entry;
+ new->start_arg = arg;
+ new->self = new;
+ new->tsd = (void *)tsd;
+ new->locale = &libc.global_locale;
+ new->unblock_cancel = self->cancel;
+ new->canary = self->canary;
+
+ a_inc(&libc.threads_minus_1);
+ ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
+
+ CLEANUP:
+ __release_ptc();
+
+ if (ret > 0) {
+ *res = new;
+ ret = thrd_success;
+ } else if (ret == -ENOMEM) {
+ ret = thrd_nomem;
+ } else {
+ a_dec(&libc.threads_minus_1);
+ if (map) __munmap(map, size);
+ ret = thrd_error;
+ }
+ return ret;
+}
diff --git a/src/thread/thrd_detach.c b/src/thread/thrd_detach.c
index 72cdfba..2597ae5 100644
--- a/src/thread/thrd_detach.c
+++ b/src/thread/thrd_detach.c
@@ -1,12 +1,14 @@
+#include "pthread_impl.h"
#include <threads.h>
-int __pthread_detach(thrd_t t);
+int __munmap(void *, size_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;
+ /* Cannot detach a thread that's already exiting */
+ if (a_swap(t->exitlock, 1))
+ return thrd_join(t, 0);
+ t->detached = 2;
+ __unlock(t->exitlock);
+ return 0;
}
--
1.7.10.4
^ permalink raw reply [flat|nested] 31+ messages in thread