Re: C threads, v. 6.2

[Rich Felker <dalias@libc.org>]

On Thu, Aug 28, 2014 at 11:34:13PM +0200, Jens Gustedt wrote:
> 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.

Do you have a conclusion from this as to whether what we're doing is
okay? FWIW the mutex and the code manipulating its internals are
always in different TUs.

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

Yes.

> > > > > > > +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.

Then do you also refrain from using strstr, strchr, etc.? :)

I certainly don't see any harm in passing to a pointer which
originally has type (void *) through a function that's going to
convert it to (const void *) and back to (void *). I could see your
objection making sense if the _original_ type were const qualified,
but here it's not.

In any case I still don't care whether the code gets duplicated or not
since it's trivial. So do whichever you like.

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

Without this change, it's a tiny patch to pthread_create.c (basically
just adding one tiny C11 start function and a few namespace fixes).
With it, there's a lot more to do, but my concern isn't whether you
have time to do a proposed refactoring of pthread_create; rather, it's
the amount of review that will need to go into evaluating whether it's
a worthwhile change.

Rich