Re: [PATCH 7/8] add the thrd_xxxxxx functions

[Rich Felker <dalias@libc.org>]

On Sun, Aug 31, 2014 at 03:19:21PM +0200, Jens Gustedt wrote:
> Am Sonntag, den 31.08.2014, 08:57 -0400 schrieb Rich Felker:
> > On Sun, Aug 31, 2014 at 09:57:34AM +0200, Jens Gustedt wrote:
> > > > >  	if (attrp) attr = *attrp;
> > > > >  
> > > > >  	__acquire_ptc();
> > > > > @@ -234,7 +253,10 @@ static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restr
> > > > >  	new->canary = self->canary;
> > > > >  
> > > > >  	a_inc(&libc.threads_minus_1);
> > > > > -	ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
> > > > > +	if (c11)
> > > > > +		ret = __clone(start_c11, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
> > > > > +	else
> > > > > +		ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
> > > > 
> > > > Couldn't this be "c11 ? start_c11 : start" to avoid duplicating the
> > > > rest of the call?
> > > 
> > > I think that the ternary expression together with the other
> > > parenthesized paramenter and the length of the line would make the
> > > line barely readable.
> > > 
> > > This are some of the pivots lines of the implementation, I'd rather
> > > have them stick out.
> > > 
> > > Also the assembler that is produced should be identical.
> > 
> > Whether or not the output is identical, your code is much less
> > readable and maintainable: an active effort is required by the reader
> > to determine that the only difference between the two code paths is
> > the function pointer being passed. This is why I prefer the use of ?:.
> > 
> > The following looks perfectly readable to me:
> > 
> > 		ret = __clone(c11 ? start_c11 : start, stack, flags,
> > 			new, &new->tid, TP_ADJ(new), &new->tid);
> 
> No to me (seriously!) because my builtin parser has difficulties to
> capture the end of the ternary expression.
> 
> Would it be acceptable to have parenthesis around?

I wouldn't even mind if you'd prefer to rename start to start_pthread
and then make start a local variable:

	start = c11 ? start_c11 : start_pthread;
	ret = __clone(start, stack, flags, new,
		&new->tid, TP_ADJ(new), &new->tid);

Would that be nicer?

> > > > Also, since it doesn't depend on anything in pthread_create.c,
> > > 
> > > It does, __THRD_C11 :)
> > 
> > How about naming it to __ATTRP_C11_THREAD and putting it in
> > pthread_impl.h then?
> 
> ok, I'll do that
> 
> But hopefully we didn't overlook any possible use pattern that would
> drag in different versions of the tsd symbols. I am not too
> comfortable with that.

What do you mean here? I don't follow.

> > Yes I'm aware, but I don't want gratuitous incentives for patch 8/8
> > just because patch 7/8 is done intentionally suboptimally. :-) If the
> > approaches are being compared, we should be comparing the preferred
> > efficient versions of both. And I'd like to wait to seriously think
> > about 8/8 until everything else is fully ready to commit, or
> > preferably already committed.
> 
> I know.
> 
> But I just discovered another such incentive :) You were right, that
> the error handling for thrd_create was not correct for C11, but it
> wasn't my fault :) POSIX (and thus __pthread_create) basically maps
> all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
> other failures.
> 
> Thus I had to integrate this difference into __pthread_create, which
> was not difficult, but which intrudes even a little bit more into the
> existing code.

I think POSIX's EAGAIN is fully equivalent to C11's thrd_nomem: both
should reflect any condition where the thread could not be created due
to a resource exhaustion type failure. While you could argue that
thrd_nomem should only indicate failure of the mmap, not the clone, I
think this would be a useless distinction to callers (both of them are
fundamentally allocation operations) and then you'd be forced to use
thrd_error for clone failures, whereas I would think thrd_error should
be reserved for either erroneous usage (but that's UB anyway) or more
permanent failures (like lack of thread support on the OS).

> > > > I'd really just prefer that all of these can't-fail cases be a
> > > > straight tail call with no support for nonzero thrd_success values.
> > > > But as long as the code is correct and the inefficiency is trivially
> > > > optimized out, I'm not going to spend a lot of time arguing about it.
> > > > I do think it's telling, though, that the (albeit minimal) complexity
> > > > of trying to handle the case where thrd_success is nonzero seems to
> > > > have caused a couple bugs -- this is part of why I don't like having
> > > > multiple code paths where one path is untestable/untested. To me, a
> > > > code path that's never going to get tested is a much bigger offense
> > > > than an assumption that a constant has the value we decided it has.
> > 
> > Do you have any thoughts on this part?
> 
> ah, I should have deleted that in my reply, since I basically
> agree. In the new version that I am preparing there are tail calls
> with corresponding comments.

OK.

> > I'm aware that you can't cast the int* to void**, but you can still
> > implement the function as a trivial wrapper that doesn't introduce any
> > duplication of internal logic for cleaning up an exited thread:
> > 
> > int thrd_join(thrd_t t, int *res)
> > {
> > 	void *pthread_res;
> > 	__pthread_join(t, &pthread_res);
> > 	if (res) *res = (int)(intptr_t)pthread_res;
> > 	return thrd_success;
> > }
> 
> dunno, doesn't look much simpler to me and drags in one more TU into C
> thread applications

What's simpler is that this version does not:

- Need pthread_impl.h
- Have knowledge of the mechanism for waiting for thread exit.
- Have knowledge of how to obtain the exit code out of thread struct.
- Have knowledge of thread deallocation mechanism.

It does encode one piece of semi-implementation-specific knowledge,
that the C11 result code is being returned via casting the int to
void* rather than storing it in some other way. But to me that's
knowledge of the way the wrapping is being performed, which is
appropriate knowledge for a C11 threads implementation file, as
opposed to knowledge of how the underlying pthreads implementation
works, which is "inappropriate knowledge". Please note that I'm not
fully against the latter when there's a good reason for it (like
making something more efficient or less ugly), but I don't like it to
be there gratuitously. Consider what would happen if we were switching
to a mechanism like glibc does of caching and reusing thread stacks
after exit (not something I'd want to do, but a good example); then
we'd have two places to update the code with much more complex logic,
rather than just one. Or as another example that may be more
reasonable, consider that we might want to make exiting threads do
their own unmapping of all but the 1 (or 2 when it straddles) page
containing the struct __pthread, so that memory isn't tied up when
there's a long gap between pthread_exit and pthread_join, and only
leave unmapping of that last page (or 2) to the joining thread. This
would also potentially require big changes in two places with your
approach.

Rich