Bug 39893 - Explicitly use PTHREAD_MUTEX_NORMAL to create pthread mutex
Summary: Explicitly use PTHREAD_MUTEX_NORMAL to create pthread mutex
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-28 11:59 PDT by Yong Li
Modified: 2010-06-01 12:53 PDT (History)
7 users (show)

See Also:


Attachments
the patch (1.37 KB, patch)
2010-05-28 12:05 PDT, Yong Li
sam: review-
Details | Formatted Diff | Diff
Updated (1.37 KB, patch)
2010-05-29 17:34 PDT, Yong Li
darin: review-
Details | Formatted Diff | Diff
use memset (1.48 KB, patch)
2010-05-31 07:36 PDT, Yong Li
darin: review-
Details | Formatted Diff | Diff
updated (1.19 KB, patch)
2010-05-31 13:03 PDT, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2010-05-28 11:59:24 PDT
There're some places in webkit that assumes mutex cannot be recursive like this:

ASSERT(!mutex.tryLock());

However, PTHREAD_MUTEX_DEFAULT can be PTHREAD_MUTEX_RECURSIVE depending the implementation.

Let's explicitly make sure we're creating non-recursive mutex.
Comment 1 Yong Li 2010-05-28 12:05:21 PDT
Created attachment 57359 [details]
the patch
Comment 2 Eric Seidel (no email) 2010-05-28 12:12:30 PDT
Attachment 57359 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2597047
Comment 3 Yong Li 2010-05-28 12:18:19 PDT
(In reply to comment #2)
> Attachment 57359 [details] did not build on mac:
> Build output: http://webkit-commit-queue.appspot.com/results/2597047

weird. struct T t = { 0 }; is a typical way to zero the struct
Comment 4 Alexey Proskuryakov 2010-05-28 14:32:55 PDT
The assertion checks that the mutex is not used recursively, it doesn't check how it's implemented.
Comment 5 Yong Li 2010-05-28 14:50:17 PDT
(In reply to comment #4)
> The assertion checks that the mutex is not used recursively, it doesn't check how it's implemented.

If the mutex is recursive (PTHREAD_MUTEX_RECURSIVE), the assertion will fail.
Comment 6 Alexey Proskuryakov 2010-05-28 14:53:55 PDT
Oh sorry, you're of course right.
Comment 7 Yong Li 2010-05-28 15:33:41 PDT
The other solution is:

1) add a flag in Mutex:

#ifndef NDEBUG
bool isLocked() const { return m_isLocked; }
bool m_isLocked;
#endif

2) set it in lock() and unlock()

3) change assertions to ASSERT(mutex.isLocked())
Comment 8 Sam Weinig 2010-05-29 10:06:55 PDT
Comment on attachment 57359 [details]
the patch

r- as this breaks the mac build.
Comment 9 Sam Weinig 2010-05-29 10:15:49 PDT
Can we do this as a compile time check.

#if PTHREAD_MUTEX_DEFAULT == PTHREAD_MUTEX_NORMAL
...old code
#else
...your code
#endif

I believe this is breaking the mac since the darwin implementation of pthread_t has a nested struct or array so you would have to initialize it with something like pt = {0, {0}}; I don't think depending on the implementations representation of pthread_t is a good idea though.
Comment 10 Yong Li 2010-05-29 17:23:29 PDT
(In reply to comment #9)
> Can we do this as a compile time check.
> 
> #if PTHREAD_MUTEX_DEFAULT == PTHREAD_MUTEX_NORMAL
> ...old code
> #else
> ...your code
> #endif

I thought about this, but it also assumes PTHREAD_MUTEX_NORMAL must be a constant. If PTHREAD_MUTEX_NORMAL is implemented by a function, this cannot build. Probably we can ignore this case?

> 
> I believe this is breaking the mac since the darwin implementation of pthread_t has a nested struct or array so you would have to initialize it with something like pt = {0, {0}}; I don't think depending on the implementations representation of pthread_t is a good idea though.

I can remove the initializer, because static variables are always zeroed.
Comment 11 Yong Li 2010-05-29 17:34:04 PDT
Created attachment 57417 [details]
Updated
Comment 12 Darin Adler 2010-05-29 21:01:14 PDT
Comment on attachment 57417 [details]
Updated

Patch looks generally good, but I have some concerns.

> +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.

This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?

> +    // Static variables are always zeroed.
> +    static pthread_mutex_t s_zeroedMutex;
> +    m_mutex = s_zeroedMutex;

It seems really strange to do it that way. If you just want to zero something out, the best way I can think of is:

    memset(&m_mutex, 0, sizeof(m_mutex));

No need for a global variable.

But why does m_mutex need to be initialized twice? Doesn't pthread_mutex_init do a complete job? Why the need to initialize it before calling that?

review- because I don't think we should land this with the extra unneeded code and the mysterious comment
Comment 13 Yong Li 2010-05-31 07:14:00 PDT
(In reply to comment #12)
> (From update of attachment 57417 [details])
> Patch looks generally good, but I have some concerns.
> > +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?

pthread_mutex_init and pthread_mutex_destroy are not supposed to be called on PTHREAD_MUTEX_INITIALIZER. I cannot find any scription that defines the behavior. So I assume the behavior is undefined, and up to implementation.

> > +    // Static variables are always zeroed.
> > +    static pthread_mutex_t s_zeroedMutex;
> > +    m_mutex = s_zeroedMutex;
> It seems really strange to do it that way. If you just want to zero something out, the best way I can think of is:
>     memset(&m_mutex, 0, sizeof(m_mutex));
> No need for a global variable.

I can change it to memset
Comment 14 Yong Li 2010-05-31 07:36:15 PDT
Created attachment 57462 [details]
use memset
Comment 15 Darin Adler 2010-05-31 09:56:49 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > > +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> > This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?
> 
> pthread_mutex_init and pthread_mutex_destroy are not supposed to be called on PTHREAD_MUTEX_INITIALIZER. I cannot find any scription that defines the behavior. So I assume the behavior is undefined, and up to implementation.

I think you're misunderstanding this. I read over the various documentation, and yes, it's true that you should not use both PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init on the same mutex, but that does not mean that you have to pre-initialize the mutex before calling pthread_mutex_init. It just means that it's a programming mistake to both use PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init.
Comment 16 Darin Adler 2010-05-31 10:01:25 PDT
Comment on attachment 57462 [details]
use memset

> +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> +    memset(&m_mutex, 0, sizeof(pthread_mutex_t));

If we are doing this memset, I think it's better style to call sizeof(m_mutex) here instead of sizeof(pthread_mutex_t), because that way you can see looking at this line of code that it's correct without having to look up the type of m_mutex to double check that it's a pthread_mutex_t.

As I said in my previous comment in this bug, this comment and code are not needed, and I am almost certain the comment reflects a misunderstanding on your part. I don't want to confuse future programmers working on this code.

Initializing to zero does have a small good effect. It gives us more predictable behavior if pthread_mutex_init fails. Both the old code path and the new code path have the same failing in that they don't check the error return of pthread_mutex_init. But this issue is not specific to this new code path and applies just as much to the PTHREAD_MUTEX_NORMAL == PTHREAD_MUTEX_DEFAULT case.

Despite the fact that this patch is otherwise just fine, I'm going to say review- because I don't want to check in a mysterious comment or unneeded code.

The only difference between the two branches of the #if should be the attributes passed in to pthread_mutex_init.
Comment 17 Yong Li 2010-05-31 12:59:33 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > > +    // Make sure it's not the same value as PTHREAD_MUTEX_INITIALIZER.
> > > This comment makes no sense. Why is it important to make sure that something is not the same value as PTHREAD_MUTEX_INITIALIZER? How is that relevant at all?
> > 
> > pthread_mutex_init and pthread_mutex_destroy are not supposed to be called on PTHREAD_MUTEX_INITIALIZER. I cannot find any scription that defines the behavior. So I assume the behavior is undefined, and up to implementation.
> 
> I think you're misunderstanding this. I read over the various documentation, and yes, it's true that you should not use both PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init on the same mutex, but that does not mean that you have to pre-initialize the mutex before calling pthread_mutex_init. It just means that it's a programming mistake to both use PTHREAD_MUTEXT_INITIALIZER and pthread_mutex_init.

If we don't initialize the mutex, what if the mutex just happens to be the same value as PTHREAD_MUTEXT_INITIALIZER? Although the chance if very small.

OK, I'll remove that code anyway.
Comment 18 Yong Li 2010-05-31 13:03:17 PDT
Created attachment 57492 [details]
updated
Comment 19 Darin Adler 2010-05-31 18:18:30 PDT
(In reply to comment #17)
> If we don't initialize the mutex, what if the mutex just happens to be the same value as PTHREAD_MUTEXT_INITIALIZER? Although the chance if very small.

Not important to this patch any more, but that question shows a misunderstanding. There's no way you could set the mutex to a value and guarantee it's not the same as PTHREAD_MUTEX_INITIALIZER, nor would you want to. I think you're just reading too much into some documentation you read. pthread_mutex_init does not require that before you call it you set the mutex up with a bit pattern that happens to not be equal to PTHREAD_MUTEX_INITIALIZER. And if it did require that I'm not sure how you would accomplish that portably anyway. There are two ways to initialize a pthreads mutex. One is to call pthread_mutex_init and the other is to use PTHREAD_MUTEX_INITIALIZER. In our case we are using pthread_mutex_init.
Comment 20 Yong Li 2010-06-01 08:56:59 PDT
(In reply to comment #19)
> (In reply to comment #17)
> > If we don't initialize the mutex, what if the mutex just happens to be the same value as PTHREAD_MUTEXT_INITIALIZER? Although the chance if very small.
> 
> Not important to this patch any more, but that question shows a misunderstanding. There's no way you could set the mutex to a value and guarantee it's not the same as PTHREAD_MUTEX_INITIALIZER, nor would you want to. I think you're just reading too much into some documentation you read. pthread_mutex_init does not require that before you call it you set the mutex up with a bit pattern that happens to not be equal to PTHREAD_MUTEX_INITIALIZER. And if it did require that I'm not sure how you would accomplish that portably anyway. There are two ways to initialize a pthreads mutex. One is to call pthread_mutex_init and the other is to use PTHREAD_MUTEX_INITIALIZER. In our case we are using pthread_mutex_init.

yeah. probably I have thought too much
Comment 21 WebKit Commit Bot 2010-06-01 10:46:53 PDT
Comment on attachment 57492 [details]
updated

Clearing flags on attachment: 57492

Committed r60487: <http://trac.webkit.org/changeset/60487>
Comment 22 WebKit Commit Bot 2010-06-01 10:47:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Xan Lopez 2010-06-01 11:36:13 PDT
This is not quite working on GTK+:


../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:5: warning: "PTHREAD_MUTEX_NORMAL" is not defined
../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:29: warning: "PTHREAD_MUTEX_DEFAULT" is not defined
Comment 24 Darin Adler 2010-06-01 11:38:59 PDT
(In reply to comment #23)
> This is not quite working on GTK+:
>
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:5: warning: "PTHREAD_MUTEX_NORMAL" is not defined
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:29: warning: "PTHREAD_MUTEX_DEFAULT" is not defined

I suggest doing this for now to fix the build:

    #if !defined PTHREAD_MUTEX_NORMAL || PTHREAD_MUTEX_NORMAL == PTHREAD_MUTEX_DEFAULT

Later we can figure it out.
Comment 25 Yong Li 2010-06-01 12:53:44 PDT
(In reply to comment #23)
> This is not quite working on GTK+:
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:5: warning: "PTHREAD_MUTEX_NORMAL" is not defined
> ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:238:29: warning: "PTHREAD_MUTEX_DEFAULT" is not defined

Oops. sorry for this. but PTHREAD_MUTEX_NORMAL is supposed to be defined in pthread.h.

Are you fixing it as Darin sugguests now?