[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1409491161.4476.283.camel@eris.loria.fr>
Date: Sun, 31 Aug 2014 15:19:21 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 7/8] add the thrd_xxxxxx functions
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?
> > > 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.
> > > 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 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'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.
> > > > 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 <sys/mman.h>
> > > > +#include <threads.h>
> > > > +
> > > > +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;
> }
dunno, doesn't look much simpler to me and drags in one more TU into C
thread applications
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.
