[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 28 Aug 2014 23:34:13 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: C threads, v. 6.2
Am Donnerstag, den 28.08.2014, 16:00 -0400 schrieb Rich Felker:
> On Thu, Aug 28, 2014 at 09:28:09PM +0200, Jens Gustedt wrote:
> > at least it doesn't matter for the standard functions (they are `extern
> > "C"`) but only for user functions with C++ interfaces.
>
> Right, but it matters for all C++ code containing C++ functions that
> use pthread_mutex_t* as an argument. And apparently there's a lot of
> such code.
>
> > Well, ok, so if you could come up with some better idea in the future,
> > let me know.
>
> I'm not even sure it's an issue. I've seen it argued that aliasing
> rules don't even apply here because, when you access something like
> m->_m_lock, that's not an "access" to the structure object/type but to
> the individual member. If that's true, then as long as the structs
> have identical layout, it should be valid to access the members via
> either.
Yes, there is a special rule for struct types in different TU, that
they are compatible when their internal structure is the same
(including alignment) and if their *tag* name is the same.
> Also, what is the relationship between two identical struct or union
> types without tags (i.e. the first member of pthread_mutex_t and the
> first member of mtx_t, both of which are unions with no tag)?
For structs with no tags the situation is more subtle. If you are in
the same TU and declare them in different places they are *not*
compatible, basically they are two different struct. On the other hand
two such struct in different TU are compatible, if they comply to the
above rule of structural equivalence.
And for C++ it really seems that typedef identifiers have yet another
exception for the case that they typedef a struct or union type.
struct stat { int a; };
void stat(struct stat A) { }
struct stut { int a; };
typedef struct { int a; } stut;
is only an error on the typedef.
> > > > > > +#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.
>
> I was asking whether the use of static to mean "pointer to at least
> this many elements" used for an argument in a function type resulted
> in a distinct function type from the same without static.
the answer is no to that, too, the function type is the same
for the first "dimension" only type qualifiers are accounted for the
function type
> > > 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.
>
> So the following is NOT UB?
>
> void foo(int [static 2]);
>
> void bar()
> {
> int x;
> foo(&x);
> }
>
> I'm guessing not.
you are guessing correctly, UB only occurs if x[1] is accessed through
foo.
> The same seems to apply to restrict too -- it has no
> meaning on the caller side, so the compiler cannot automatically
> assume UB when the restrict constraint is violated by the caller. It
> can only be used when optimizing on the callee side. I suppose static
> is the same -- but in the case of static, the only optimization that
> seems to be possible on the callee side is assuming the pointer is
> non-null; the length seems irrelevant.
The callee can also be a caller to another (or the same) function and
thus transmit the invariant, there.
Basically this could do some bounds checking for arrays with
statically known sizes.
> > 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 :(
>
> Yes. Then let's just omit it for now.
ok
(you probably mean that also for the nonnull version, I suppose)
> > > > > > 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.
>
> There's no cast here. void * converts implicitly to const void *.
There is a cast inside the pthread_setspecific function which I really
don't like, we discussed that before, I think. I'd rather not use a
function that does const conversion magic under the hood. These are
really badly designed interfaces.
> > I know. I'll have a look and try to factor these things out, such that
> > we really can weigh the alternatives.
>
> Can we look at this as a potential post-merge task? I'm skeptical that
> it improves anything; saving maybe 100-200 bytes in the static-linked
> C11-only case is probably not worth spreading code out over multiple
> functions or files and making the flow of pthread_create less obvious.
> I'm willing to look at it if you want to try anyway, but I don't think
> it should be holding up getting C11 threads support added.
wouldn't be holding up, I promisse. I'd have to factor this into
digestable patches anyhow, so this should not be much more effort.
> > 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**.
>
> I see. But I don't think it matters. You're already marshalling the
> int return value via a cast to void* in thrd_exit and thrd_join, and
> in the case of detach, the result is being thrown away anyway. There's
> nothing sketchy/UB going on here, while other places already encode
> the assumption that void* can faithfully round-trip int values.
I know, there is no UB, but I still don't like this kind of
stuff. Basically, I have to read deeply into the code at any time I
touch it to understand that fact that it is not UB. These oddities
should be constrained to two or three well identifiable places where
they are unavoidable, namely the create, start and join functions.
> > 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
>
> Why? I see nowhere in C11 that it's specified not to touch errno. In
> general, any standard library function can clobber errno as long as it
> does not set it to 0, unless it's specifically documented not to
> modify errno on success.
Somehow I always think that the standard is more constrained than that.
At least footnote 202 suggests somehow that a library function
[w|c]ould save errno and restore it later. I guess this is considered
"being nice".
> > 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.
>
> The latter is some fairly major code duplication, especially since we
> have fallbacks for pre-SYS_clock_gettime kernels. Eventually I plan to
> add emulation/fallback for at least CLOCK_MONOTONIC and
> CLOCK_CPUTIME_CLOCKID too, and if we want to later add support for
> more clock bases to timespec_get, using the same __clock_gettime
> backend allows the fallback code to work there too. So I think it's
> best not to try to make the vdsocall/syscall directly.
I didn't find that code to complicated and much of a duplicate. Let's
see.
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.
