Summary: | Explicitly use PTHREAD_MUTEX_NORMAL to create pthread mutex | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yong Li <yong.li.webkit> | ||||||||||
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, commit-queue, darin, eric, sam, staikos, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | Other | ||||||||||||
Attachments: |
|
Description
Yong Li
2010-05-28 11:59:24 PDT
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? |