From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/6012 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 7/8] add the thrd_xxxxxx functions Date: Sun, 31 Aug 2014 08:57:45 -0400 Message-ID: <20140831125745.GW12888@brightrain.aerifal.cx> References: <22215ff2f880382340930f78cc746565a625a806.1409423162.git.Jens.Gustedt@inria.fr> <20140831004643.GP12888@brightrain.aerifal.cx> <1409471854.4476.272.camel@eris.loria.fr> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1409489889 28609 80.91.229.3 (31 Aug 2014 12:58:09 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 31 Aug 2014 12:58:09 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-6019-gllmg-musl=m.gmane.org@lists.openwall.com Sun Aug 31 14:57:58 2014 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1XO4hi-0006Ic-Kt for gllmg-musl@plane.gmane.org; Sun, 31 Aug 2014 14:57:58 +0200 Original-Received: (qmail 9772 invoked by uid 550); 31 Aug 2014 12:57:57 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 9764 invoked from network); 31 Aug 2014 12:57:57 -0000 Content-Disposition: inline In-Reply-To: <1409471854.4476.272.camel@eris.loria.fr> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:6012 Archived-At: 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); > > 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? > > it would be nice to put it in a separate thrd_create.c. It's not a big > > deal but it shaves off a small function in POSIX programs that don't > > use thrd_create. > > I'd really prefer to keep all the logic of thrd_create together in one > file. All the other additions at this point are tightly bound to be in > the same TU as pthread_create, for all this weak symbol stuff that is > going on with tsd and friends. > > Also see that as an incentive to accept patch 8/8 to separate both > implementations more cleanly :) 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'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? > > > diff --git a/src/thread/thrd_join.c b/src/thread/thrd_join.c > > > new file mode 100644 > > > index 0000000..a8c7aed > > > --- /dev/null > > > +++ b/src/thread/thrd_join.c > > > @@ -0,0 +1,16 @@ > > > +#include "pthread_impl.h" > > > +#include > > > +#include > > > + > > > +int __munmap(void *, size_t); > > > + > > > +/* C11 threads cannot be canceled, so there is no need for a > > > + cancelation function pointer, here. */ > > > +int thrd_join(thrd_t t, int *res) > > > +{ > > > + int tmp; > > > + while ((tmp = t->tid)) __timedwait(&t->tid, tmp, 0, 0, 0, 0, 0); > > > + if (res) *res = (int)(intptr_t)t->result; > > > + if (t->map_base) __munmap(t->map_base, t->map_size); > > > + return thrd_success; > > > +} > > > > I'd rather avoid duplicating this function too. > > No this ain't a duplicate. The cast here is necessary and plain use of > pthread_join would have us interpret an int* as void**, so the > assignment could potentially overwrite the second half of the word res > is pointing to. > > I'll have that visually stick out with a comment 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; } Rich