Re: [PATCH 5/9] add the functions for mtx_t

[Rich Felker <dalias@libc.org>]

On Mon, Sep 01, 2014 at 12:46:57AM +0200, Jens Gustedt wrote:
> diff --git a/src/thread/mtx_lock.c b/src/thread/mtx_lock.c
> new file mode 100644
> index 0000000..4d5d75e
> --- /dev/null
> +++ b/src/thread/mtx_lock.c
> @@ -0,0 +1,13 @@
> +#include "pthread_impl.h"
> +#include <threads.h>
> +
> +int mtx_lock(mtx_t *m)
> +{
> +	if (m->_m_type == PTHREAD_MUTEX_NORMAL && !a_cas(&m->_m_lock, 0, EBUSY))
> +		return thrd_success;
> +	/* Calling mtx_timedlock with a null pointer is an
> +	   extension. Such a call is convenient, here, since it avoids
> +	   to repeat the case analysis that is already done for
> +	   mtx_timedlock. */
> +	return mtx_timedlock(m, 0);
> +}
> diff --git a/src/thread/mtx_timedlock.c b/src/thread/mtx_timedlock.c
> new file mode 100644
> index 0000000..08359d8
> --- /dev/null
> +++ b/src/thread/mtx_timedlock.c
> @@ -0,0 +1,18 @@
> +#include <threads.h>
> +#include <errno.h>
> +
> +int __pthread_mutex_timedlock(mtx_t *restrict m, const struct timespec *restrict ts);
> +
> +int mtx_timedlock(mtx_t *restrict mutex, const struct timespec *restrict ts) {
> +	int ret = __pthread_mutex_timedlock(mutex, ts);
> +	switch (ret) {
> +	/* May also return EINVAL or EAGAIN. EAGAIN is
> +	   specially tricky since C11 doesn't define how many recursive
> +	   calls can be done. (this *isn't* the maximum amount of nested
> +	   calls!) This implementation here deals this with a counter and
> +	   detects overflow, so this is definitively UB. */

I'm omitting this comment for now because it sems wrong. EINVAL is not
possible here, and I can't find any evidence that "too many recursive
locks" is UB, so since mtx_timedlock is required to report errors when
the operation can't be performed, I think thrd_error is just the
"right behavior". The spec should be more clear though.

> diff --git a/src/thread/mtx_trylock.c b/src/thread/mtx_trylock.c
> new file mode 100644
> index 0000000..4f55796
> --- /dev/null
> +++ b/src/thread/mtx_trylock.c
> @@ -0,0 +1,21 @@
> +#include "pthread_impl.h"
> +#include <threads.h>
> +
> +int __pthread_mutex_trylock(pthread_mutex_t *restrict m);
> +
> +int mtx_trylock(mtx_t *restrict m) {
> +	if (m->_m_type == PTHREAD_MUTEX_NORMAL)
> +		return (a_cas(&m->_m_lock, 0, EBUSY) & EBUSY) ? thrd_busy : thrd_success;
> +
> +	int ret = __pthread_mutex_trylock(m);
> +	switch (ret) {
> +	/* In case of UB may also return EINVAL or EAGAIN. EAGAIN is
> +	   specially tricky since C11 doesn't define how many recursive
> +	   calls can be done. (this *isn't* the maximum amount of nested
> +	   calls!) This implementation here deals this with a counter and
> +	   detects overflow, so this is definitively UB. */

Likewise here. One could even argue thrd_busy is more appropriate
here, but for consistency it should probably be thrd_error too.

> diff --git a/src/thread/mtx_unlock.c b/src/thread/mtx_unlock.c
> new file mode 100644
> index 0000000..e673cd5
> --- /dev/null
> +++ b/src/thread/mtx_unlock.c
> @@ -0,0 +1,9 @@
> +#include <threads.h>
> +
> +int __pthread_mutex_unlock(mtx_t *);
> +
> +int (mtx_unlock)(mtx_t *mtx) {
> +	int ret = __pthread_mutex_unlock(mtx);
> +	/* In case of UB may also return EPERM. */
> +	return ret ? thrd_error : thrd_success;
> +}

We had discussed trimming this down at some point (there is no error
that's not UB) to make it a tail call, so I'll try to do that now.

Rich