From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/6110 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 5/9] add the functions for mtx_t Date: Sat, 6 Sep 2014 21:51:49 -0400 Message-ID: <20140907015148.GZ23797@brightrain.aerifal.cx> References: <0f3acdbceb88a357600611d64f7b5ed13ab3eced.1409524413.git.Jens.Gustedt@inria.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 1410054729 31804 80.91.229.3 (7 Sep 2014 01:52:09 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 7 Sep 2014 01:52:09 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-6123-gllmg-musl=m.gmane.org@lists.openwall.com Sun Sep 07 03:52:02 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 1XQRe6-00039X-3w for gllmg-musl@plane.gmane.org; Sun, 07 Sep 2014 03:52:02 +0200 Original-Received: (qmail 3706 invoked by uid 550); 7 Sep 2014 01:52:01 -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 3684 invoked from network); 7 Sep 2014 01:52:01 -0000 Content-Disposition: inline In-Reply-To: <0f3acdbceb88a357600611d64f7b5ed13ab3eced.1409524413.git.Jens.Gustedt@inria.fr> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:6110 Archived-At: 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 > + > +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 > +#include > + > +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 > + > +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 > + > +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