This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2 2/4] manual: Add documentation for pthread_tryjoin_np and pthread_timedjoin_np



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).


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]