[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1409254089.4476.137.camel@eris.loria.fr>
Date: Thu, 28 Aug 2014 21:28:09 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: C threads, v. 6.2
Hello,
Am Donnerstag, den 28.08.2014, 12:15 -0400 schrieb Rich Felker:
> > 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.
>
> That's not how it works. In C++, there is no separate namespace for
> struct tags versus types. If you define a struct without a tag but
> using typedef, the typedef name becomes its effective struct tag,
> which is what gets used for name mangling. If you later use typedef
> again to make an alias for this type, it does not make a new struct
> type with a new tag, just an alias for the existing one. Thus, your
> patch as written changes the struct tags for C++ ABI from
> "pthread_mutex_t" and "pthread_cond_t" to "__pthread_mutex_t" and
> "__pthread_cond_t".
argh
at least it doesn't matter for the standard functions (they are `extern
"C"`) but only for user functions with C++ interfaces.
Well, ok, so if you could come up with some better idea in the future,
let me know.
> > > > +#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.
>
> Actually I like it much better, but I'm not sufficiently familiar with
> it to know whether it's a conforming declaration. Is it formally
> compatible with the standard prototype?
yes, C allows even for crufty things like
void f(int a[static volatile const 42]);
to be equivalent to
void f(int *volatile const a);
as a prototype. Interfaces can contain more information than just the
prototype.
> If so, how does it allow the compiler to enforce anything?
It doesn't enforce anything, it is more a hint at the same level as
`restrict` or `register`. It is a prerequisite that the user of the
function has to ensure.
As nsz remarked in his reply, most older compilers don't do anything
with it, they just ignore it, though the existence of the nonnull
attribute shows that they would be easily capable of doing so.
The other inconvenience for `static 1` is C++. They haven't adopted
it, so as such this would make the headers incompatible with C++. So
also for this one we would need some preprocessor magic.
And then, also, it is ugly :(
> > > > 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.
>
> Or: thrd_success ? thrd_success : ret;
ok, I'll go for that
> > > > 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)
>
> In this case it's still possible to implement tss_set as a wrapper,
I am really allergic against casts, even more if they are somewhat
hidden. So I'd do much to avoid that.
> just not an alias, but the code is sufficiently small that maybe it's
> better just to duplicate it.
ok
> > > 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.
>
> From a maintainability standpoint, having two subtly different
> versions of the same complex code is a big cost. I wouldn't be
> fundamentally opposed to a factoring that avoids this duplication
> while allowing the POSIX-specific parts to be optimized out for pure
> C11 use, but I think that would be a lot more invasive (mainly in
> terms of potential for errors, possibly also for performance, but
> unlikely since basically all the time is spent in mmap, mprotect, and
> clone anyway) and would be something to look into at a later time, not
> while trying to get the initial work on C11 threads in.
>
> > 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.
>
> I don't think saving 200 bytes in a pure-C11 static-linked program
> while adding 300 bytes to libc.so in code duplication (these numbers
> are just guesses but sound about right) really makes it come across as
> more lightweight.
I know. I'll have a look and try to factor these things out, such that
we really can weigh the alternatives.
> > > > 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
>
> It's not so odd if you think about it -- when the thread to be
> detached has already exited, you can't (implementation-wise) detach it
> and leave it responsible for cleaning up itself, because it doesn't
> exist anymore. Since you know it's exited though, joining and throwing
> away the result is possible.
But thinking of it and looking into the patches I discovered my real
motivation for doing so. (I should have better commented to remember
things myself.) The thing is that using pthread_join under the hood is
semantically wrong because of the different interfaces. Well we are
only passing in a null pointer, here, but still the type is wrong, C
threads "join" on a int* and not on a void**.
> > > 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 :)
>
> I agree.
>
> > 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.
>
> That seems likely, or they might make it undefined. C <3 UB...
>
> > For the EFAULT case, I don't have a strong opinion, could be either
> > way.
>
> EFAULT really isn't an error the caller can expect to get. If there's
> any userspace work aside from the syscall, it would be SIGSEGV
> instead.
>
> Is there a reason non-EINTR errors need to be mapped to distinct
> values, though? I was thinking just 3 cases, 0, EINTR, and default,
> would suffice.
ok, let's do it like that
> > 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.
>
> With any luck, UTC might eventually be fixed to drop leap seconds, and
> then it won't matter for future times (and having your clock set to a
> past time is nonsense anyway). But I don't think C is strict on how
> the clock corresponds to real-world time; at least it wasn't before
> C11. And POSIX time explicitly excludes leap seconds.
>
> > 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.
>
> I think the "implementation defined" "epoch" should actually be
> defined as something fixed, not something that varies. And for the
> main intended use of the function (passing the timespec to
> timedlock/timedwait functions), we need to match the clock the pthread
> functions use if we're wrapping the pthread implementations, which
> means using CLOCK_REALTIME.
yes, right
I found another glitch with timespec_get, though. It shouldn't touch
errno. So either we do some stiches around it to save errno and
restore it (for a case that probably never happens) or implement it
directly from the vdso and the syscall. I already have done the later,
and it looks sufficiently simple to be acceptable and not too much
code duplication.
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.
