253080 - _PT_PTHREAD_MUTEX_IS_LOCKED macro broken, causes debug build to fail

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[green]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7) Gecko/20040720 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7) Gecko/20040720 Firefox/0.9.1

When WITH_DEBUG=1 is used, assertions that use the macro
_PT_PTHREAD_MUTEX_IS_LOCKED() fail consistently.  This is due to FreeBSD
returning the right POSIX error (EDEADLK) instead of EBUSY in the case where the
owner is trying to lock a locked, non-recursive mutex.

Reproducible: Always
Steps to Reproduce:

Comment 1

[green]

Attachment: change check for error return of EBUSY to any error return

Comment 2

[Wan-Teh Chang]

Thanks for the bug report.  I remember dealing with this
problem for some other platform (AIX?) before.  I seem to
recall that in that case the vendor decided to change their
error code.

I remember the POSIX spec can be interpreted to say
either EBUSY or EDEADLK is correct.  It would be better if
we can define the macro to allow only EBUSY and EDEADLK,
rather than any error.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #2)
> I remember the POSIX spec can be interpreted to say
> either EBUSY or EDEADLK is correct.  It would be better if
> we can define the macro to allow only EBUSY and EDEADLK,
> rather than any error.

Does it matter, though?  Right now, if we get the "wrong" return value, code
assumes that the mutex is not locked, which seems far more incorrect than
assuming that the mutex is locked on EINVAL or EFAULT (which are far less
frequent cases that are bound to be cought elsewhere).  I'm not sure if calling
pthread_mutex_trylock twice to test for both EBUSY/EDEADLK in the macro is a
better alternative. 

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Actually, I take that back -- the code should probably just use the
PT_TRYLOCK_BUSY define (that's already different between _PR_DCETHREADS and the
rest of the world), and platforms which return EDEADLK can define it to that. 
_PT_PTHREAD_MUTEX_IS_LOCKED can then check against PT_TRYLOCK_BUSY.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 154350 [details] [diff] [review]
change check for error return of EBUSY to any error return

>-#define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (EBUSY == pthread_mutex_trylock(&(m)))
>+#define _PT_PTHREAD_MUTEX_IS_LOCKED(m)    (0 != pthread_mutex_trylock(&(m)))

Later on in _pth.h, there is a comment about pthread_mutex_trylock returning
different values on DCETHREADS vs. pthreads.  There's a define,
PT_TRYLOCK_BUSY, that's set to 0 on DCETHREADS and currently EBUSY on
everything else.  The _PT_PTHREAD_MUTEX_TRYLOCK macro should probably use
PT_TRYLOCK_BUSY, and appropriate sections added for various OS's that return
EDEADLK instead of EBUSY.

Then again, given that there's no way to do real error checking anyway, 0 might
just be easier.

Comment 6

[Christophe Ravel]

The target milestone is already released. Resetting target milestone.

Comment 7

[Phoenix]

Is this still actual?