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.
Created attachment 57359 [details] the patch
Attachment 57359 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/2597047
(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
The assertion checks that the mutex is not used recursively, it doesn't check how it's implemented.
(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.
Oh sorry, you're of course right.
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 on attachment 57359 [details] the patch r- as this breaks the mac build.
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.
(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.
Created attachment 57417 [details] Updated
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
(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
Created attachment 57462 [details] use memset
(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 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.
(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.
Created attachment 57492 [details] updated
(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.
(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 on attachment 57492 [details] updated Clearing flags on attachment: 57492 Committed r60487: <http://trac.webkit.org/changeset/60487>
All reviewed patches have been landed. Closing bug.
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
(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.
(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?