[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1409218850.4476.96.camel@eris.loria.fr>
Date: Thu, 28 Aug 2014 11:40:50 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: C threads, v. 6.2
Hi,
thanks for the fast review and reply, Rich.
I will not reply to all raised issues, otherwise this gets out of
bounds :) Consider the ones where I don't as agreed.
For a start, I didn't intend this to be integrated into musl as just
one patch, this was just meant for easier review my list of perhaps 30
patches that I have divided this currently. Once we agree on the
content, I would regroup and rebase these to reasonable semantical
units, and propose them as 4 or 5 patches, say.
Am Mittwoch, den 27.08.2014, 19:46 -0400 schrieb Rich Felker:
> On Thu, Aug 28, 2014 at 12:11:45AM +0200, Jens Gustedt wrote:
> > diff --git a/arch/arm/bits/alltypes.h.in b/arch/arm/bits/alltypes.h.in
> > index 183c4c4..333d43f 100644
> > --- a/arch/arm/bits/alltypes.h.in
> > +++ b/arch/arm/bits/alltypes.h.in
> > @@ -19,7 +19,7 @@ TYPEDEF long time_t;
> > TYPEDEF long suseconds_t;
> >
> > TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
> > -TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
> > -TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } pthread_cond_t;
> > +TYPEDEF struct { union { int __i[6]; volatile void *volatile __p[6]; } __u; } __pthread_mutex_t;
> > +TYPEDEF struct { union { int __i[12]; void *__p[12]; } __u; } __pthread_cond_t;
> > [...]
> > +TYPEDEF __pthread_cond_t pthread_cond_t;
> > +TYPEDEF __pthread_cond_t cnd_t;
> > +TYPEDEF __pthread_mutex_t pthread_mutex_t;
> > +TYPEDEF __pthread_mutex_t mtx_t;
>
> This changes the C++ ABI by changing the tag for the POSIX types, so
> it can't be done this way. We could return to some serious language
> lawyering about whether it's valid to access members via the 'wrong'
> struct type when we have two identical struct types, or just assume
> that's safe for now and do it.
I don't see your point, this doesn't change the tag name, no? (There
is no tag name, here.) This is just typedeffing to the same type as
before so this should be "as allowed" as it was before.
> > +#ifdef __GNUC__
> > +#define _Nonnull(...) __attribute__((__nonnull__(__VA_ARGS__)))
> > +#else
> > +#define _Nonnull(...)
> > +#endif
>
> If we want to use attributes like this, it's a separate change from
> adding threads. But I'm fairly much against doing it anyway since the
> compiler already knows how to make these warnings for standard
> functions, without help from the headers.
There will be a while, until compilers know these things for C11
thread functions, I think.
BTw, I was tempted to do it the "C" way by using things like
`[static 1]` instead of `*`, but then I thought you wouldn't like
that.
> > +#ifndef __THRD_EXPERIMENTAL
> > +# define __THRD_EXPERIMENTAL 1
> > +#endif
> > +
> > + /* The following list of 9 integer constants makes up for the binary
> > + compatibility of this C thread implementation. You must never
> > + link code against versions of the C library that do not agree
> > + upon these ABI parameters.
> > [...]
>
> Getting this committed to musl, or at least making the first release
> with it present, should be committing to the ABI, so I don't think we
> need any ABI-check type stuff here.
right, I'll pull that out
> But I am curious how it works.
> __THRD_ABI_CHECK doesn't seem to be used anywhere. Is the intent just
> that you would put it in a program manually?
yes, that was the use case I had when starting all this. Should be
obsolete by now.
> > +#define thread_local _Thread_local
>
> This needs to be omitted for C++11+, I think.
right
> > diff --git a/include/time.h b/include/time.h
> > index dc88070..13dccaa 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -129,6 +129,19 @@ int stime(const time_t *);
> > time_t timegm(struct tm *);
> > #endif
> >
> > +#if __STDC_VERSION__ >= 201112L
> > + /* 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);
> > +#endif
> > +
> > #ifdef __cplusplus
> > }
> > #endif
>
> So far, when it comes to C11 library functionality as opposed to
> compiler features that might not be available without compiler
> support, the approach we've taken in musl is to expose them
> unconditionally. This makes it possible to use the library features
> even on compilers without explicit C11 Support.
ok, I'll make that unconditional
> Arguments could be
> made either way on this but I think we should be consistent. It might
> also be more appropriate to factor out timespec_get as a separate
> patch since it's nominally separate from threads, although I don't
> care much either way. Presumably the main use of it is for arguments
> to timedwait/timedlock operations.
see my remark on regrouping into subsets of patches above
> > 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);
>
> Here it might be nicer to just use __syscall(SYS_mprotect...) directly
> in pthread_create. We don't need the extra page alignment overhead
> there, only for the public function, and __syscall is lighter than a
> function call anyway.
I'd rather leave such intrusive changes to the pthread code to you :)
> > diff --git a/src/thread/cnd_broadcast.c b/src/thread/cnd_broadcast.c
> > new file mode 100644
> > index 0000000..b1db667
> > --- /dev/null
> > +++ b/src/thread/cnd_broadcast.c
> > @@ -0,0 +1,9 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int __pthread_cond_broadcast(pthread_cond_t *);
> > +
> > +int cnd_broadcast(cnd_t * cnd) {
> > + int ret = __pthread_cond_broadcast(cnd);
> > + return ret ? thrd_error : thrd_success;
> > +}
>
> This could/should actually be a separate function using
> __private_signal from pthread_cond_timedwait.c, I think.
right, good idea (and for all the similar or implied changes you
mention after)
> Also I'm fine with omitting the error conversion logic since the
> called function is implemented so as never to return an error (and in
> fact POSIX defines no errors, so it would have to be an additional
> implementation-defined error, which musl strongly avoids). If you want
> this to be more clear, a short comment should probably suffice without
> dead code to convert error numbers that never occur.
ok
> > diff --git a/src/thread/cnd_destroy.c b/src/thread/cnd_destroy.c
> > new file mode 100644
> > index 0000000..11cfc19
> > --- /dev/null
> > +++ b/src/thread/cnd_destroy.c
> > @@ -0,0 +1,17 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int __pthread_cond_destroy(pthread_cond_t *);
> > +
> > +/* The behavior of cnd_destroy is undefined if cnd is still in
> > + use. The choice for pthread_cond_destroy in that situation is to
> > + wake up all users before destroying. I am not sure that we should
> > + do it like that here, too. Alternatives would be:
> > + - complain by using perror or equivalent
> > + - assert that there is no waiter
> > + - abort when there is a waiter
> > + - do nothing
> > + */
> > +void (cnd_destroy)(cnd_t *cnd) {
> > + (void)__pthread_cond_destroy(cnd);
> > +}
>
> This can just be a NOP now. The non-NOP pthread_cond_destroy is only
> for process-shared cond vars. Non-shared ones do not need any action
> for destruction.
right, this is definitively an advantage of the new cond code
> > diff --git a/src/thread/cnd_timedwait.c b/src/thread/cnd_timedwait.c
> > new file mode 100644
> > index 0000000..cce47a4
> > --- /dev/null
> > +++ b/src/thread/cnd_timedwait.c
> > @@ -0,0 +1,14 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m, const struct timespec *restrict ts);
> > +
> > +int cnd_timedwait(cnd_t *restrict cond, mtx_t *restrict mutex, const struct timespec *restrict ts) {
> > + 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;
> > + }
> > +}
>
> C11 isn't clear on what happens when the timespec is non-normalized.
yes, I noticed that, too, have that on my list of necessary
clarifications for C11, now
> Should we still just treat this as an error, or attempt to normalize
> it first?
I would leave it as is, I'd rather like to stay as close to the POSIX
behavior as possible
> > diff --git a/src/thread/mtx_init.c b/src/thread/mtx_init.c
> > new file mode 100644
> > index 0000000..827a56a
> > --- /dev/null
> > +++ b/src/thread/mtx_init.c
> > @@ -0,0 +1,10 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int mtx_init(mtx_t * m, int type)
> > +{
> > + *m = (pthread_mutex_t) {
> > + ._m_type = ((type&mtx_recursive) ? PTHREAD_MUTEX_RECURSIVE : 0),
> > + };
> > + return thrd_success;
> > +}
>
> I'm okay with a 0 here since we assume all over that {0} init for a
> mutex is a default type mutex, but you might prefer something like
> PTHREAD_MUTEX_NORMAL here instead of 0.
yes
> > diff --git a/src/thread/mtx_lock.c b/src/thread/mtx_lock.c
> > new file mode 100644
> > index 0000000..89730f1
> > --- /dev/null
> > +++ b/src/thread/mtx_lock.c
> > @@ -0,0 +1,14 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int mtx_lock(mtx_t *m)
> > +{
> > + __THRD_ABI_MARK;
> > + if (m->_m_type == PTHREAD_MUTEX_NORMAL && !a_cas(&m->_m_lock, 0, EBUSY))
> > + return thrd_success;
> > + /* Calling mtx_timedlock with a null pointer is an
> > + extension. Such a call is convenient, here, since it avoids
> > + to repeat the case analysis that is already done for
> > + mtx_timedlock. */
> > + return mtx_timedlock(m, 0);
> > +}
> > diff --git a/src/thread/mtx_timedlock.c b/src/thread/mtx_timedlock.c
> > new file mode 100644
> > index 0000000..c42a096
> > --- /dev/null
> > +++ b/src/thread/mtx_timedlock.c
> > @@ -0,0 +1,19 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec *restrict ts);
> > +
> > +int mtx_timedlock(mtx_t *restrict mutex, const struct timespec *restrict ts) {
>
> It might be worth duplicating the a_cas attempt here for normal-type;
> I'm not sure.
that would be premature optimization :)
> > diff --git a/src/thread/mtx_unlock.c b/src/thread/mtx_unlock.c
> > new file mode 100644
> > index 0000000..d1721a6
> > --- /dev/null
> > +++ b/src/thread/mtx_unlock.c
> > @@ -0,0 +1,11 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int __pthread_mutex_unlock(pthread_mutex_t *);
> > +
> > +int (mtx_unlock)(mtx_t *mtx) {
> > + /* In the best of all worlds this is a tail call. */
> > + int ret = __pthread_mutex_unlock(mtx);
> > + /* In case of UB may also return EPERM. */
> > + return ret ? thrd_error : thrd_success;
> > +}
>
> For unlock, C11 makes it UB to unlock a mutex you don't own (even for
> recursive type), so the tail call would be legal. I don't have a
> strong opinion either way here.
I'll remove the comment, this is a left over. Tail call is still not
possible, since moraly thrd_success is not 0 :)
But we could just return thrd_success unconditionnally.
> > diff --git a/src/thread/pthread_cond_wait.c b/src/thread/pthread_cond_wait.c
> > index 8735bf1..58656f7 100644
> > --- a/src/thread/pthread_cond_wait.c
> > +++ b/src/thread/pthread_cond_wait.c
> > @@ -1,6 +1,8 @@
> > #include "pthread_impl.h"
> >
> > +int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m, const struct timespec *restrict ts);
> > +
> > int pthread_cond_wait(pthread_cond_t *restrict c, pthread_mutex_t *restrict m)
> > {
> > - return pthread_cond_timedwait(c, m, 0);
> > + return __pthread_cond_timedwait(c, m, 0);
> > }
>
> This is unnecessary.
right
> > diff --git a/src/thread/pthread_equal.c b/src/thread/pthread_equal.c
> > index 3e3df4f..38fb45e 100644
> > --- a/src/thread/pthread_equal.c
> > +++ b/src/thread/pthread_equal.c
> > @@ -1,4 +1,4 @@
> > -#include <pthread.h>
> > +#include "pthread_impl.h"
> >
> > int (pthread_equal)(pthread_t a, pthread_t b)
> > {
>
> This does not seem like a useful change and just increases compile
> time.
a leftover, sorry
> > diff --git a/src/thread/pthread_setcanceltype.c b/src/thread/pthread_setcanceltype.c
> > index bf0a3f3..d493c1b 100644
> > --- a/src/thread/pthread_setcanceltype.c
> > +++ b/src/thread/pthread_setcanceltype.c
> > @@ -1,11 +1,13 @@
> > #include "pthread_impl.h"
> >
> > +void __pthread_testcancel(void);
> > +
> > int pthread_setcanceltype(int new, int *old)
> > {
> > struct pthread *self = __pthread_self();
> > if (new > 1U) return EINVAL;
> > if (old) *old = self->cancelasync;
> > self->cancelasync = new;
> > - if (new) pthread_testcancel();
> > + if (new) __pthread_testcancel();
> > return 0;
> > }
>
> This change looks gratuitous. pthread_setcanceltype is in the POSIX
> namespace anyway and isn't being changed for use in C11 (and it has no
> use to C11).
right
> > diff --git a/src/thread/pthread_setspecific.c b/src/thread/pthread_setspecific.c
> > index 55e46a8..dd18b96 100644
> > --- a/src/thread/pthread_setspecific.c
> > +++ b/src/thread/pthread_setspecific.c
> > @@ -1,6 +1,6 @@
> > #include "pthread_impl.h"
> >
> > -int pthread_setspecific(pthread_key_t k, const void *x)
> > +int __pthread_setspecific(pthread_key_t k, const void *x)
> > {
> > struct pthread *self = __pthread_self();
> > /* Avoid unnecessary COW */
> > @@ -10,3 +10,5 @@ int pthread_setspecific(pthread_key_t k, const void *x)
> > }
> > return 0;
> > }
> > +
> > +weak_alias(__pthread_setspecific, pthread_setspecific);
>
> It looks like you have a duplicate tss_set for this rather than using
> the above, so either the above is a grauitous change, or the new
> tss_set is duplicate code that should be removed. Or did I miss
> something?
you are right, this should be omitted
(the tss_get code differs in the const qualification of the second argument)
> > 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;
> > +}
>
> Minor indention/formatting issues. But my main concern is the amount
> of code duplication, especially delicate implementation details where
> it would be easy for them to get out of sync. To me that seems like a
> bigger concern than thrd_create pulling in a few hundred bytes more
> than it should (for the extra unused functionality in pthread_create).
>
> Didn't we discuss before just passing a special attr pointer to
> __pthread_create to effect C11-style creation?
I did that, and that version could still be revived. But I was not too
happy with it, because it meant intruding into the existing pthread
code. For this version, the pthread code is just reorganized in
different TU, but otherwise there should be no change.
Also I'd really like to advertise C threads as a lightweighted,
stripped down thread implementation, so the short cuts (that are
merely a fall off of the code separation, here) were politically
welcome.
> > 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();
> > +}
>
> OK, but I'm not sure the detailed comment is needed. The exact same
> code appears in pthread_self.c.
I certainly have the tendency to comment a bit more than you :)
All this could potentially divert from pthread one day, so I'd like to
have such information in place for future coders or maintainers
(myself, probably)
> > diff --git a/src/thread/thrd_detach.c b/src/thread/thrd_detach.c
> > new file mode 100644
> > index 0000000..211b8fb
> > --- /dev/null
> > +++ b/src/thread/thrd_detach.c
> > @@ -0,0 +1,18 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int __munmap(void *, size_t);
> > +
> > +int thrd_detach(thrd_t t)
> > +{
> > + /* Cannot detach a thread that's already exiting */
> > + if (a_swap(t->exitlock, 1)){
> > + int tmp;
> > + while ((tmp = t->tid)) __timedwait(&t->tid, tmp, 0, 0, 0, 0, 0);
> > + if (t->map_base) __munmap(t->map_base, t->map_size);
> > + } else {
> > + t->detached = 2;
> > + __unlock(t->exitlock);
> > + }
> > + return thrd_success;
> > +}
>
> You already moved pthread_detach to the protected namespace, so this
> seems to be duplicate code.
It is.
> And it's a place that has implementation
> details (magic numbers like 2, which you could argue shouldn't exist
> ;-)
I would !)
> so I think it's best to avoid spreading these details out over
> more places where they could go out of sync, no?
yes, probably, the fact that pthread_detach called pthread_join
somehow disturbed me. I'll reconsider
> > diff --git a/src/thread/tss_delete.c b/src/thread/tss_delete.c
> > new file mode 100644
> > index 0000000..d50731f
> > --- /dev/null
> > +++ b/src/thread/tss_delete.c
> > @@ -0,0 +1,8 @@
> > +#include "pthread_impl.h"
> > +#include <threads.h>
> > +
> > +int __pthread_key_delete(pthread_key_t k);
> > +
> > +void (tss_delete)(tss_t key) {
> > + (void)__pthread_key_delete(key);
> > +}
>
> This could probably be done with just an alias, but I'm fine with the
> tail-call.
I thought of this, but I was reluctant to make an alias to a function
with a different calling convention. Couldn't that pollute the return
register in an unpredictable way?
> It should be a very rarely-used function (ideally,
> never-used) since it's rare for there to be any safe way to destroy a
> key once you create it.
Yes, exactly. That's why I also didn't try to make this a #define or
static inline in the haeder file, it's not worth the effort.
> > 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;
> > +}
>
> As mentioned before, this is duplicate, and it could probably just be
> an alias.
no, I don't think so, it has a different interface, there is no const
> > diff --git a/src/time/thrd_sleep.c b/src/time/thrd_sleep.c
> > new file mode 100644
> > index 0000000..6570b0c
> > --- /dev/null
> > +++ b/src/time/thrd_sleep.c
> > @@ -0,0 +1,31 @@
> > +#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);
> > + // compile time comparison, constant propagated
> > + if (EINTR != 1) {
> > + 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;
> > + /* error described by POSIX */
> > + case -EINVAL: return -2;
> > + /* described for linux */
> > + case -EFAULT: return -3;
> > + /* No other error returns are specified. */
> > + default: return INT_MIN;
> > + }
> > + }
> > + // potentially a tail call
> > + return ret;
> > +}
>
> __syscall is almost never a "tail call" because it's rarely a call at
> all; usually, it's inline asm. :-)
I'll remove that comment and the check for EINTR being 1.
> But okay. My preference would be
> not to do fancy remapping of error codes (especially not when the
> codes will differ by arch, since this just encourages programs to
> assume they're fixed and then break on the arch where they're
> different, if any).
We have to be at least partially fancy, because C enforces -1 for the
interrupt case without really being precise what that should mean. I
just naively assumed that they mean the same as EINTR :)
For EINVAL, as discussed above, I'd expect that the C specification
will be modified to also have the validity check for the req
argument. POSIX has a "shall" here, so I'd like to keep the case well
identified.
For the EFAULT case, I don't have a strong opinion, could be either
way.
> Also indention is inconsistent with musl (tabs for indention, spaces
> for alignment).
ah, still, sorry
> > diff --git a/src/time/timespec_get.c b/src/time/timespec_get.c
> > new file mode 100644
> > index 0000000..bf78e5a
> > --- /dev/null
> > +++ b/src/time/timespec_get.c
> > @@ -0,0 +1,9 @@
> > +#include <time.h>
> > +
> > +int __clock_gettime(clockid_t clk, struct timespec *ts);
> > +
> > +/* the base argument is simply ignored, there is no other implemented
> > + value than TIME_UTC. */
> > +int timespec_get(struct timespec * ts, int base) {
> > + return __clock_gettime(CLOCK_REALTIME, ts) < 0 ? 0 : base;
> > +}
>
> As mentioned in another email (off-list, IIRC), I'd like to reject
> unknown base values so that programs built against a newer musl that
> has other clock bases, but which get an older musl at runtime, will
> see an error rather than silently using the wrong clock. Does this
> make sense?
I makes perfect sense, but I don't recall you having that said as
clearly.
BTW, the choice of CLOCK_REALTIME, here, could be subject of
debade. IIRC this clock doesn't exactly give the seconds since Unix
Epoch, but that value minus some leapseconds. So it doesn't seem to
fulfill the C specification.
One could think of using CLOCK_MONOTONIC_RAW, instead, or
CLOCK_BOOTTIME if it exists. But I am not sure if the bootime of the
machine can be considered as an "epoch" in the sense of the C standard
or even in plain English.
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 ::
Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
