Re: working C11 thread implementation

[Rich Felker <dalias@libc.org>]

On Sat, Aug 02, 2014 at 12:21:12AM +0200, Jens Gustedt wrote:
> Hello,
> thanks for the quick review!
> 
> Am Freitag, den 01.08.2014, 23:08 +0200 schrieb Szabolcs Nagy:
> > > For the choices of the constants, for this version they are such that
> > > most wrapper calls result in being tail calls. I verified this by
> > > looking into the assembler that is produced. As they are now, most of
> > > these tail functions could also be just provided as weak aliases. We
> > > chose to implement them as functions such that in case of change of
> > > the constants we only have to recompile and no other maintenance is
> > > required.
> > > 
> > 
> > i'd assume the constants to be right and fix up the
> > code only in case of later breakage
> 
> Well, the problem is that these constants are not "constants" but come
> from errno.h. This has two disadvantages:
> 
>  - currently this pollutes the name space of the C thread
>    implementation with the E-names
> 
>  - these constants are arch dependent, we would need another mechanism
>    to get them visible
> 
> My preferred solution to this is to make a TR to the thread
> specification, by forcing the obvious choices

I'm not a fan of this solution, and I'd prefer the constants be
completely independent, especially since they're not semantically
identical to the POSIX ones. I don't see how C could mandate that they
be equal to the POSIX ones when it doesn't even define the POSIX ones
in errno.h. And from a practical standpoint, the fact that the errno
values vary by arch makes them awkward to use.

Of course the most important thing in choosing these values is to
avoid an ABI divergence with glibc. And unfortunately I haven't gotten
any response from their side on how they want to define them.

> and by allowing
> threads.h to include errno.h. (The optional Annex K sets a precedent
> by having errno codes as return value for functions.)

There's no precedent for any other header including errno.h
implicitly, and I'm against adding any such requirement or even
allowance. There's a lot more value in keeping the headers independent
and requiring explicit inclusion of headers you want to use.

> > > +int cnd_signal(cnd_t * cnd) {
> > > +	/* In the best of all worlds this is a tail call. */
> > > +	int ret = __pthread_cond_signal(cnd);
> > > +	if (thrd_success)
> > > +		return ret ? thrd_error : thrd_success;
> > > +	return ret;
> > > +}
> > 
> > this is a bit weird
> > 
> > i think it's better to just assume thrd_success==0
> > and static assert it somewhere as a reminder
> 
> _Static_assert would need a compiler that implements this. It would be
> easier just not to introduce a dependency for the tool chain. The code
> as it is, now, will get optimized out by any decent compiler, I hope.

I'm fine with just assuming thrd_success is zero as long as we
actually end up defining it as such.

> > > +/* This is needed because not all arch have __pthread_self as a static
> > > +   symbol. */
> > >  pthread_t pthread_self()
> > >  {
> > >  	return __pthread_self();
> > 
> > ...
> > 
> > > +/* This is needed because not all arch have __pthread_self as a static
> > > +   symbol. */
> > > +pthread_t thrd_current()
> > > +{
> > > +	return __pthread_self();
> > > +}
> > 
> > 
> > i dont understand these comments
> > you mean that they could be weak aliases otherwise?
> 
> yes something like that, I'll amend the comment

From a standpoint of minimizing the patch for adding C11 threads and
making it C11-threads-only (rather than unrelated changes) I think the
comment for pthread_self() should be omitted. But the comment is
confusing as written anyway. Is your point that you're not making it
an alias for __pthread_self because the latter is not necessarily a
function?

> > > +/* Roughly __syscall already returns the right thing: 0 if all went
> > > +   well or a negative error indication, otherwise. Unfortunately the C
> > > +   standard foresees the special case of an interrupted call and fixes
> > > +   that error return to -1 (instead of introducing EINTR). */
> > > +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 -EINTR: return -1;
> > > +    case -1: return -EINTR;
> > > +    }
> > > +  }
> > > +  // potentially a tail call
> > > +  return ret;
> > > +}
> > 
> > "The thrd_sleep function returns zero if the requested
> >  time has elapsed, -1 if it has been interrupted by a
> >  signal, or a negative value if it fails."
> > 
> > this is confusing
> 
> you mean the C standards text is confusing? yes, definitively
> 
> if you mean the code is confusing, then I should explain it better :)
> 
> The idea is that if SYS_nanosleep returns -EINTR, thrd_sleep in turn
> must return -1. If EINTR is in fact 1 on the arch we just can return
> the value (and 0 if everthing is ok and some undefined value in other
> UB cases.)
> 
> If EINTR isn't 1, we have to be careful not to return -1 for some
> other error code of SYS_nanosleep, whatever happens to be error number
> 1. So the second case captures such an accidental -1 and sends -EINTR
> (which we know not to be -1.).

This is kind of clever, but it might just be nicer to hard-code values
like -1 and -2 rather than swapping -1/-EINTR so that the result looks
like an errno value but really isn't one...

> > (either should not have the name thrd_ or follow
> > the error enum convention of other thrd_ functions)
> 
> I am not sure that I understand what you try to say.

Probably just that the spec is poorly designed.

Rich