This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 2/4] manual: Add documentation for pthread_tryjoin_np and pthread_timedjoin_np
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Mike Crowe <mac at mcrowe dot com>
- Cc: libc-alpha at sourceware dot org, Yann Droneaud <ydroneaud at opteya dot com>
- Date: Mon, 14 Oct 2019 16:15:11 -0300
- Subject: Re: [PATCH v2 2/4] manual: Add documentation for pthread_tryjoin_np and pthread_timedjoin_np
- References: <cover.3b6b26e85a044f5ad7494dfb035620d75eb57c63.1568809830.git-series.mac@mcrowe.com> <f6aea6d8eb36a982f7e42e7b571e6d7dd6919a8a.1568809830.git-series.mac@mcrowe.com> <11fa584c-ca33-dc49-7d2b-f590b53cd032@linaro.org> <20190928085347.GA11327@mcrowe.com>
On 28/09/2019 05:53, Mike Crowe wrote:
> On Thursday 26 September 2019 at 14:26:00 -0700, Adhemerval Zanella wrote:
>> On 18/09/2019 05:30, Mike Crowe wrote:
>>> This documentation isn't perfect, but it's better than nothing and can
>>> hopefully act as a basis for future improvement. In particular, although
>>> I'm certain that the functions are MT-Safe, I'm not sure about the rest of
>>> the @safety line.
>>>
>>> Yann Droneaud pointed out that pthread_timedjoin_np would wait forever if
>>> the timeout parameter is passed as NULL. Since it's now too late to change
>>> that, I've documented it.
>>
>> To which semantic do you think it would be better to change? Since this
>> symbol are not yet in POSIX there still the possibility to change it
>> by using a new plus a compat symbol.
>
> The alternative would be to mark the timeout parameter as __nonnull and not
> permit it to be NULL, as is the case for the other pthread timed*/clock*
> functions. I imagine that if the functions were ever standardised then this
> would be the case - but that doesn't look like it's on the horizon.
>
> Although we wouldn't break existing binaries if we change the behaviour and
> add a compat symbol, we could break people compiling existing source. I
> don't think it's a serious enough problem for it to be worth even doing
> that.
>
> However, if others feel strongly about it then I'm prepared to make that
> change.
At least other pthread interfaces that uses a timespec as input argument
add a nonnull on their interfaces. However they handle a null not equal:
- pthread_mutex_{clock,timed}lock will trigger a SEGFAULT due
lll_timedlock_wait.c:31 for the contended case, it works for
uncontended case (where an userspace atomic operation is suffice).
- pthread_cond_timedwait will also trigger a SEGFAULT due
nptl/pthread_cond_wait.c:648.
- pthread_{rw,rd}lock_{clock,timed}lock will handle it as unbounded
time (since both pthread_rwlock_common.c and futexes call check if
abstime is NULL).
> - wait forever (aka. pthread_join()) : pass NULL
> - wait: (aka. pthread_timedjoin_np()): pass deadline
> - don't wait (aka. pthread_tryjoin_np()) : pass current time
I don't see that making pthread_timedjoin_np behaves differently than
other pthread interfaces a good move forward. So I would avoid support
both 'wait forever' and 'don't wait' semantic in this case.
And POSIX also is not specific regarding timeout handling in aforementioned
pthread interfaces. And systems that implement them do differ, for instance:
- FreeBSD 11.1 does not trigger any build warning or runtime error for
NULL argument for neither pthread_mutex_timedlock or
pthread_{rw,rd}lock_lock. The pthread_cond_timedwait does return
EINVAL though.
- Solaris 11.4 seems to behave as glibc, it triggers a SEGFAULT for
pthread_mutex_timedlock (for *both* contended and non-contended case),
pthread_{rw,rd}lock_timedlock, *and* for pthread_cond_timedwait.
- AIX 7.2 returns EINVAL for contended pthread_mutex_timedlock case and
works for non-contended case. It does not trigger any runtime issue
for {rw,rd}lock and also returns EINVAL for pthread_cond_timedwait.
So trying to come up with a portable error handling does not seem to
that valuable, it leads to believe common usage is to *not* use NULL
value for timeout arguments.
So I would suggest to sync with current practice and fail early for
invalid inputs either by EINVAL or by assuming valid pointer (which
trigger an invalid memory access and warns user that this is an invalid
usage).