Alexandre Julliard wrote:
> Ulrich Weigand <[EMAIL PROTECTED]> writes:
>
> > But in this case, I think Ove is basically right: you have only one
> > race between QUEUE_SetWakeBit and QUEUE_WaitBits.
>
> Well, yes of course, the race is between testing and setting the bits,
> but the race not only happens if the compiler optimizes out the second
> test, it can happen for a number of reasons (like reordering of the
> assignments, two threads setting bits at the same time, etc.)
Well, volatile prevents reordering of assignments. But nevertheless
I agree, volatile helps only if you have atomic operations in the
first place ... (On Intel, load/store to an aligned dword is atomic,
so the only problem is the |=. Of course, on other architectures,
not even that is necessarily guaranteed.)
> > What do you think about this patch, for example? (Untested ...)
>
> That seems reasonable yes.
Well, that quick patch contained a potential deadlock due to lock
order inversion :-( Furthermore, it really only makes sense if
*all* accesses to wakeBits/changeBits/wakeMask are protected ...
The following patch is a more complete implementation of this idea;
it is also somewhat tested (at least the Borland Thread Sorting Demo,
my old stress test case for inter-thread messages, still runs ;-).
Ove, could you try whether this solves your deadlock?
Bye,
Ulrich
ChangeLog:
* include/queue.h windows/queue.c windows/message.c
Protect queue->wakeBits/changeBits/wakeBits by critical section.
diff -ur wine-cvs/include/queue.h wine-uw/include/queue.h
--- wine-cvs/include/queue.h Wed Jan 3 22:51:30 2001
+++ wine-uw/include/queue.h Wed Jan 3 23:10:28 2001
@@ -159,7 +159,8 @@
extern MESSAGEQUEUE *QUEUE_GetSysQueue(void);
extern void QUEUE_SetWakeBit( MESSAGEQUEUE *queue, WORD bit );
extern void QUEUE_ClearWakeBit( MESSAGEQUEUE *queue, WORD bit );
-extern void QUEUE_ReceiveMessage( MESSAGEQUEUE *queue );
+extern WORD QUEUE_TestWakeBit( MESSAGEQUEUE *queue, WORD bit );
+extern BOOL QUEUE_ReceiveMessage( MESSAGEQUEUE *queue );
extern int QUEUE_WaitBits( WORD bits, DWORD timeout );
extern void QUEUE_IncPaintCount( HQUEUE16 hQueue );
extern void QUEUE_DecPaintCount( HQUEUE16 hQueue );
diff -ur wine-cvs/windows/message.c wine-uw/windows/message.c
--- wine-cvs/windows/message.c Wed Jan 3 22:51:35 2001
+++ wine-uw/windows/message.c Wed Jan 3 23:12:58 2001
@@ -1123,7 +1123,7 @@
static BOOL MSG_PeekMessage( int type, LPMSG msg, HWND hwnd,
DWORD first, DWORD last, WORD flags, BOOL peek )
{
- int mask;
+ int changeBits, mask;
MESSAGEQUEUE *msgQueue;
HQUEUE16 hQueue;
POINT16 pt16;
@@ -1159,12 +1159,15 @@
WIN_RestoreWndsLock(iWndsLocks);
return FALSE;
}
+
+ EnterCriticalSection( &msgQueue->cSection );
msgQueue->changeBits = 0;
+ LeaveCriticalSection( &msgQueue->cSection );
/* First handle a message put by SendMessage() */
- while (msgQueue->wakeBits & QS_SENDMESSAGE)
- QUEUE_ReceiveMessage( msgQueue );
+ while ( QUEUE_ReceiveMessage( msgQueue ) )
+ ;
/* Now handle a WM_QUIT message */
@@ -1183,7 +1186,7 @@
/* Now find a normal message */
retry:
- if (((msgQueue->wakeBits & mask) & QS_POSTMESSAGE) &&
+ if ((QUEUE_TestWakeBit(msgQueue, mask & QS_POSTMESSAGE)) &&
((qmsg = QUEUE_FindMsg( msgQueue, hwnd, first, last )) != 0))
{
/* Try to convert message to requested type */
@@ -1206,7 +1209,10 @@
break;
}
- msgQueue->changeBits |= MSG_JournalPlayBackMsg();
+ changeBits = MSG_JournalPlayBackMsg();
+ EnterCriticalSection( &msgQueue->cSection );
+ msgQueue->changeBits |= changeBits;
+ LeaveCriticalSection( &msgQueue->cSection );
/* Now find a hardware event */
@@ -1222,12 +1228,12 @@
/* Check again for SendMessage */
- while (msgQueue->wakeBits & QS_SENDMESSAGE)
- QUEUE_ReceiveMessage( msgQueue );
+ while ( QUEUE_ReceiveMessage( msgQueue ) )
+ ;
/* Now find a WM_PAINT message */
- if ((msgQueue->wakeBits & mask) & QS_PAINT)
+ if (QUEUE_TestWakeBit(msgQueue, mask & QS_PAINT))
{
WND* wndPtr;
msg->hwnd = WIN_FindWinToRepaint( hwnd , hQueue );
@@ -1263,10 +1269,11 @@
if (!(flags & PM_NOYIELD))
{
UserYield16();
- while (msgQueue->wakeBits & QS_SENDMESSAGE)
- QUEUE_ReceiveMessage( msgQueue );
+ while ( QUEUE_ReceiveMessage( msgQueue ) )
+ ;
}
- if ((msgQueue->wakeBits & mask) & QS_TIMER)
+
+ if (QUEUE_TestWakeBit(msgQueue, mask & QS_TIMER))
{
if (TIMER_GetTimerMsg(msg, hwnd, hQueue, flags & PM_REMOVE)) break;
}
@@ -1279,7 +1286,7 @@
WIN_RestoreWndsLock(iWndsLocks);
return FALSE;
}
- msgQueue->wakeMask = mask;
+
QUEUE_WaitBits( mask, INFINITE );
QUEUE_Unlock( msgQueue );
}
@@ -1986,8 +1993,10 @@
return WAIT_FAILED;
}
+ EnterCriticalSection( &msgQueue->cSection );
msgQueue->changeBits = 0;
msgQueue->wakeMask = dwWakeMask;
+ LeaveCriticalSection( &msgQueue->cSection );
if (THREAD_IsWin16(NtCurrentTeb()))
{
@@ -2025,11 +2034,14 @@
/*
* If a message matching the wait mask has arrived, return.
*/
+ EnterCriticalSection( &msgQueue->cSection );
if (msgQueue->changeBits & dwWakeMask)
{
+ LeaveCriticalSection( &msgQueue->cSection );
ret = nCount;
break;
}
+ LeaveCriticalSection( &msgQueue->cSection );
/*
* And continue doing this until we hit the timeout.
diff -ur wine-cvs/windows/queue.c wine-uw/windows/queue.c
--- wine-cvs/windows/queue.c Wed Jan 3 22:51:35 2001
+++ wine-uw/windows/queue.c Wed Jan 3 23:10:19 2001
@@ -620,19 +620,32 @@
*
* See "Windows Internals", p.449
*/
-void QUEUE_SetWakeBit( MESSAGEQUEUE *queue, WORD bit )
+static BOOL QUEUE_TrySetWakeBit( MESSAGEQUEUE *queue, WORD bit, BOOL always )
{
- TRACE_(msg)("queue = %04x (wm=%04x), bit = %04x\n",
- queue->self, queue->wakeMask, bit );
+ BOOL wake = FALSE;
+
+ EnterCriticalSection( &queue->cSection );
+
+ TRACE_(msg)("queue = %04x (wm=%04x), bit = %04x, always = %d\n",
+ queue->self, queue->wakeMask, bit, always );
- if (bit & QS_MOUSE) pMouseQueue = queue;
- if (bit & QS_KEY) pKbdQueue = queue;
- queue->changeBits |= bit;
- queue->wakeBits |= bit;
+ if ((queue->wakeMask & bit) || always)
+ {
+ if (bit & QS_MOUSE) pMouseQueue = queue;
+ if (bit & QS_KEY) pKbdQueue = queue;
+ queue->changeBits |= bit;
+ queue->wakeBits |= bit;
+ }
if (queue->wakeMask & bit)
{
queue->wakeMask = 0;
+ wake = TRUE;
+ }
+ LeaveCriticalSection( &queue->cSection );
+
+ if ( wake )
+ {
/* Wake up thread waiting for message */
if ( THREAD_IsWin16( queue->teb ) )
{
@@ -652,6 +665,12 @@
SERVER_END_REQ;
}
}
+
+ return wake;
+}
+void QUEUE_SetWakeBit( MESSAGEQUEUE *queue, WORD bit )
+{
+ QUEUE_TrySetWakeBit( queue, bit, TRUE );
}
@@ -660,8 +679,22 @@
*/
void QUEUE_ClearWakeBit( MESSAGEQUEUE *queue, WORD bit )
{
+ EnterCriticalSection( &queue->cSection );
queue->changeBits &= ~bit;
queue->wakeBits &= ~bit;
+ LeaveCriticalSection( &queue->cSection );
+}
+
+/***********************************************************************
+ * QUEUE_TestWakeBit
+ */
+WORD QUEUE_TestWakeBit( MESSAGEQUEUE *queue, WORD bit )
+{
+ WORD ret;
+ EnterCriticalSection( &queue->cSection );
+ ret = queue->wakeBits & bit;
+ LeaveCriticalSection( &queue->cSection );
+ return ret;
}
@@ -690,29 +723,30 @@
for (;;)
{
+ EnterCriticalSection( &queue->cSection );
+
if (queue->changeBits & bits)
{
/* One of the bits is set; we can return */
queue->wakeMask = 0;
+
+ LeaveCriticalSection( &queue->cSection );
QUEUE_Unlock( queue );
return 1;
}
if (queue->wakeBits & QS_SENDMESSAGE)
{
/* Process the sent message immediately */
-
queue->wakeMask = 0;
+
+ LeaveCriticalSection( &queue->cSection );
QUEUE_ReceiveMessage( queue );
continue; /* nested sm crux */
}
queue->wakeMask = bits | QS_SENDMESSAGE;
- if(queue->changeBits & bits)
- {
- continue;
- }
-
TRACE_(msg)("%04x) wakeMask is %04x, waiting\n", queue->self, queue->wakeMask);
+ LeaveCriticalSection( &queue->cSection );
if ( !THREAD_IsWin16( NtCurrentTeb() ) )
{
@@ -905,21 +939,24 @@
/***********************************************************************
* QUEUE_ReceiveMessage
*
- * This routine is called when a sent message is waiting for the queue.
+ * This routine is called to check whether a sent message is waiting
+ * for the queue. If so, it is received and processed.
*/
-void QUEUE_ReceiveMessage( MESSAGEQUEUE *queue )
+BOOL QUEUE_ReceiveMessage( MESSAGEQUEUE *queue )
{
LRESULT result = 0;
SMSG *smsg;
MESSAGEQUEUE *senderQ;
- TRACE_(sendmsg)("queue %04x\n", queue->self );
-
+ EnterCriticalSection( &queue->cSection );
if ( !((queue->wakeBits & QS_SENDMESSAGE) && queue->smPending) )
{
- TRACE_(sendmsg)("\trcm: nothing to do\n");
- return;
+ LeaveCriticalSection( &queue->cSection );
+ return FALSE;
}
+ LeaveCriticalSection( &queue->cSection );
+
+ TRACE_(sendmsg)("queue %04x\n", queue->self );
/* remove smsg on the top of the pending list and put it in the processing list */
smsg = QUEUE_RemoveSMSG(queue, SM_PENDING_LIST, 0);
@@ -974,6 +1011,7 @@
ReplyMessage( result );
TRACE_(sendmsg)("done!\n" );
+ return TRUE;
}
@@ -1144,28 +1182,28 @@
}
if (hQueue)
- queue = QUEUE_Lock( hQueue );
-
- if( !queue )
{
- queue = QUEUE_Lock( hFirstQueue );
- while( queue )
- {
- if (queue->wakeMask & wakeBit) break;
-
- QUEUE_Unlock(queue);
- queue = QUEUE_Lock( queue->next );
- }
- if( !queue )
- {
- WARN_(msg)("couldn't find queue\n");
- return;
- }
+ queue = QUEUE_Lock( hQueue );
+ QUEUE_SetWakeBit( queue, wakeBit );
+ QUEUE_Unlock( queue );
+ return;
}
- QUEUE_SetWakeBit( queue, wakeBit );
+ /* Search for someone to wake */
+ queue = QUEUE_Lock( hFirstQueue );
+ while ( queue )
+ {
+ if (QUEUE_TrySetWakeBit( queue, wakeBit, FALSE ))
+ {
+ QUEUE_Unlock( queue );
+ return;
+ }
+
+ QUEUE_Unlock(queue);
+ queue = QUEUE_Lock( queue->next );
+ }
- QUEUE_Unlock( queue );
+ WARN_(msg)("couldn't find queue\n");
}
@@ -1275,7 +1313,9 @@
MESSAGEQUEUE *queue;
if (!(queue = (MESSAGEQUEUE *)QUEUE_Lock( hQueue ))) return;
+ EnterCriticalSection( &queue->cSection );
queue->wPaintCount++;
+ LeaveCriticalSection( &queue->cSection );
QUEUE_SetWakeBit( queue, QS_PAINT );
QUEUE_Unlock( queue );
}
@@ -1289,8 +1329,10 @@
MESSAGEQUEUE *queue;
if (!(queue = (MESSAGEQUEUE *)QUEUE_Lock( hQueue ))) return;
+ EnterCriticalSection( &queue->cSection );
queue->wPaintCount--;
if (!queue->wPaintCount) queue->wakeBits &= ~QS_PAINT;
+ LeaveCriticalSection( &queue->cSection );
QUEUE_Unlock( queue );
}
@@ -1303,7 +1345,9 @@
MESSAGEQUEUE *queue;
if (!(queue = (MESSAGEQUEUE *)QUEUE_Lock( hQueue ))) return;
+ EnterCriticalSection( &queue->cSection );
queue->wTimerCount++;
+ LeaveCriticalSection( &queue->cSection );
QUEUE_SetWakeBit( queue, QS_TIMER );
QUEUE_Unlock( queue );
}
@@ -1317,8 +1361,10 @@
MESSAGEQUEUE *queue;
if (!(queue = (MESSAGEQUEUE *)QUEUE_Lock( hQueue ))) return;
+ EnterCriticalSection( &queue->cSection );
queue->wTimerCount--;
if (!queue->wTimerCount) queue->wakeBits &= ~QS_TIMER;
+ LeaveCriticalSection( &queue->cSection );
QUEUE_Unlock( queue );
}
@@ -1469,8 +1515,10 @@
DWORD ret;
if (!(queue = (MESSAGEQUEUE *)QUEUE_Lock( GetFastQueue16() ))) return 0;
+ EnterCriticalSection( &queue->cSection );
ret = MAKELONG( queue->changeBits, queue->wakeBits );
queue->changeBits = 0;
+ LeaveCriticalSection( &queue->cSection );
QUEUE_Unlock( queue );
return ret & MAKELONG( flags, flags );
@@ -1485,8 +1533,10 @@
DWORD ret;
if (!(queue = (MESSAGEQUEUE *)QUEUE_Lock( GetFastQueue16() ))) return 0;
+ EnterCriticalSection( &queue->cSection );
ret = MAKELONG( queue->changeBits, queue->wakeBits );
queue->changeBits = 0;
+ LeaveCriticalSection( &queue->cSection );
QUEUE_Unlock( queue );
return ret & MAKELONG( flags, flags );
@@ -1556,7 +1606,9 @@
if (!(queue = (MESSAGEQUEUE *)QUEUE_Lock( GetFastQueue16() )))
return FALSE;
+ EnterCriticalSection( &queue->cSection );
ret = queue->wakeBits & (QS_KEY | QS_MOUSEBUTTON);
+ LeaveCriticalSection( &queue->cSection );
QUEUE_Unlock( queue );
return ret;
@@ -1572,8 +1624,8 @@
/* Handle sent messages */
queue = (MESSAGEQUEUE *)QUEUE_Lock( GetFastQueue16() );
- while (queue && (queue->wakeBits & QS_SENDMESSAGE))
- QUEUE_ReceiveMessage( queue );
+ while ( queue && QUEUE_ReceiveMessage( queue ) )
+ ;
QUEUE_Unlock( queue );
@@ -1586,8 +1638,8 @@
/* Handle sent messages again */
queue = (MESSAGEQUEUE *)QUEUE_Lock( GetFastQueue16() );
- while (queue && (queue->wakeBits & QS_SENDMESSAGE))
- QUEUE_ReceiveMessage( queue );
+ while ( queue && QUEUE_ReceiveMessage( queue ) )
+ ;
QUEUE_Unlock( queue );
}
--
Dr. Ulrich Weigand
[EMAIL PROTECTED]
