Mercurial > sdl-ios-xcode
annotate src/thread/win32/SDL_syssem.c @ 4105:84882a89ca50 SDL-1.2
Date: Thu, 27 Dec 2007 07:38:25 +0000
From: John Bartholomew
Subject: [SDL] SDL Semaphore implementation broken on Windows?
Hi,
Over the past couple of days, I've been battling with SDL, SDL_Mixer and SMPEG to try to find an audio hang bug. I believe I've found the problem, which I think is a race condition inside SDL's semaphore implementation (at least the Windows implementation). The semaphore code uses Windows' built in semaphore functions, but it also maintains a separate count value. This count value is updated with bare increment and decrement operations in SemPost and SemWaitTimeout - no locking primitives to protect them.
In tracking down the apparent audio bug, I found that at some point a semaphore's count value was being decremented to -1, which is clearly not a valid value for it to take.
I'm still not certain exactly what sequence of operations is occuring for this to happen, but I believe that overall it's a race condition between a thread calling SemPost (which increments the count) and the thread on the other end calling SemWait (which decrements it).
I will try to make a test case to verify this, but I'm not sure if I'll be able to (threading errors being difficult to reproduce even in the best circumstances).
However, assuming this is the cause of my problems, there is a very
simple fix:
Windows provides InterlockedIncrement() and InterlockedDecrement()
functions to perform increments and decrements which are guaranteed to be atomic. So the fix is in thread/win32/SDL_syssem.c: replace occurrences of --sem->count with InterlockedDecrement(&sem->count); and replace occurrences of ++sem->count with InterlockedIncrement(&sem->count);
This is using SDL v1.2.12, built with VC++ 2008 Express, running on a
Core 2 duo processor.
author | Sam Lantinga <slouken@libsdl.org> |
---|---|
date | Fri, 28 Dec 2007 22:05:17 +0000 |
parents | bb6839704ed6 |
children | a1b03ba2fcd0 |
rev | line source |
---|---|
0 | 1 /* |
2 SDL - Simple DirectMedia Layer | |
1312
c9b51268668f
Updated copyright information and removed rcs id lines (problematic in branch merges)
Sam Lantinga <slouken@libsdl.org>
parents:
769
diff
changeset
|
3 Copyright (C) 1997-2006 Sam Lantinga |
0 | 4 |
5 This library is free software; you can redistribute it and/or | |
1312
c9b51268668f
Updated copyright information and removed rcs id lines (problematic in branch merges)
Sam Lantinga <slouken@libsdl.org>
parents:
769
diff
changeset
|
6 modify it under the terms of the GNU Lesser General Public |
0 | 7 License as published by the Free Software Foundation; either |
1312
c9b51268668f
Updated copyright information and removed rcs id lines (problematic in branch merges)
Sam Lantinga <slouken@libsdl.org>
parents:
769
diff
changeset
|
8 version 2.1 of the License, or (at your option) any later version. |
0 | 9 |
10 This library is distributed in the hope that it will be useful, | |
11 but WITHOUT ANY WARRANTY; without even the implied warranty of | |
12 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | |
1312
c9b51268668f
Updated copyright information and removed rcs id lines (problematic in branch merges)
Sam Lantinga <slouken@libsdl.org>
parents:
769
diff
changeset
|
13 Lesser General Public License for more details. |
0 | 14 |
1312
c9b51268668f
Updated copyright information and removed rcs id lines (problematic in branch merges)
Sam Lantinga <slouken@libsdl.org>
parents:
769
diff
changeset
|
15 You should have received a copy of the GNU Lesser General Public |
c9b51268668f
Updated copyright information and removed rcs id lines (problematic in branch merges)
Sam Lantinga <slouken@libsdl.org>
parents:
769
diff
changeset
|
16 License along with this library; if not, write to the Free Software |
c9b51268668f
Updated copyright information and removed rcs id lines (problematic in branch merges)
Sam Lantinga <slouken@libsdl.org>
parents:
769
diff
changeset
|
17 Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA |
0 | 18 |
19 Sam Lantinga | |
252
e8157fcb3114
Updated the source with the correct e-mail address
Sam Lantinga <slouken@libsdl.org>
parents:
36
diff
changeset
|
20 slouken@libsdl.org |
0 | 21 */ |
1402
d910939febfa
Use consistent identifiers for the various platforms we support.
Sam Lantinga <slouken@libsdl.org>
parents:
1358
diff
changeset
|
22 #include "SDL_config.h" |
0 | 23 |
24 /* Semaphore functions using the Win32 API */ | |
25 | |
1433
bb6839704ed6
SDL_windows.h is no longer necessary
Sam Lantinga <slouken@libsdl.org>
parents:
1402
diff
changeset
|
26 #define WIN32_LEAN_AND_MEAN |
bb6839704ed6
SDL_windows.h is no longer necessary
Sam Lantinga <slouken@libsdl.org>
parents:
1402
diff
changeset
|
27 #include <windows.h> |
0 | 28 |
29 #include "SDL_thread.h" | |
30 #if defined(_WIN32_WCE) && (_WIN32_WCE < 300) | |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
31 #include "win_ce_semaphore.h" |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
32 #endif |
0 | 33 |
34 | |
35 struct SDL_semaphore { | |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
36 #if defined(_WIN32_WCE) && (_WIN32_WCE < 300) |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
37 SYNCHHANDLE id; |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
38 #else |
0 | 39 HANDLE id; |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
40 #endif |
0 | 41 Uint32 volatile count; |
42 }; | |
43 | |
44 | |
45 /* Create a semaphore */ | |
46 SDL_sem *SDL_CreateSemaphore(Uint32 initial_value) | |
47 { | |
48 SDL_sem *sem; | |
49 | |
50 /* Allocate sem memory */ | |
1336
3692456e7b0f
Use SDL_ prefixed versions of C library functions.
Sam Lantinga <slouken@libsdl.org>
parents:
1330
diff
changeset
|
51 sem = (SDL_sem *)SDL_malloc(sizeof(*sem)); |
0 | 52 if ( sem ) { |
53 /* Create the semaphore, with max value 32K */ | |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
54 #if defined(_WIN32_WCE) && (_WIN32_WCE < 300) |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
55 sem->id = CreateSemaphoreCE(NULL, initial_value, 32*1024, NULL); |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
56 #else |
0 | 57 sem->id = CreateSemaphore(NULL, initial_value, 32*1024, NULL); |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
58 #endif |
0 | 59 sem->count = initial_value; |
60 if ( ! sem->id ) { | |
61 SDL_SetError("Couldn't create semaphore"); | |
1336
3692456e7b0f
Use SDL_ prefixed versions of C library functions.
Sam Lantinga <slouken@libsdl.org>
parents:
1330
diff
changeset
|
62 SDL_free(sem); |
0 | 63 sem = NULL; |
64 } | |
65 } else { | |
66 SDL_OutOfMemory(); | |
67 } | |
68 return(sem); | |
69 } | |
70 | |
71 /* Free the semaphore */ | |
72 void SDL_DestroySemaphore(SDL_sem *sem) | |
73 { | |
74 if ( sem ) { | |
75 if ( sem->id ) { | |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
76 #if defined(_WIN32_WCE) && (_WIN32_WCE < 300) |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
77 CloseSynchHandle(sem->id); |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
78 #else |
0 | 79 CloseHandle(sem->id); |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
80 #endif |
0 | 81 sem->id = 0; |
82 } | |
1336
3692456e7b0f
Use SDL_ prefixed versions of C library functions.
Sam Lantinga <slouken@libsdl.org>
parents:
1330
diff
changeset
|
83 SDL_free(sem); |
0 | 84 } |
85 } | |
86 | |
87 int SDL_SemWaitTimeout(SDL_sem *sem, Uint32 timeout) | |
88 { | |
89 int retval; | |
90 DWORD dwMilliseconds; | |
91 | |
92 if ( ! sem ) { | |
93 SDL_SetError("Passed a NULL sem"); | |
94 return -1; | |
95 } | |
96 | |
97 if ( timeout == SDL_MUTEX_MAXWAIT ) { | |
98 dwMilliseconds = INFINITE; | |
99 } else { | |
100 dwMilliseconds = (DWORD)timeout; | |
101 } | |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
102 #if defined(_WIN32_WCE) && (_WIN32_WCE < 300) |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
103 switch (WaitForSemaphoreCE(sem->id, dwMilliseconds)) { |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
104 #else |
0 | 105 switch (WaitForSingleObject(sem->id, dwMilliseconds)) { |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
106 #endif |
0 | 107 case WAIT_OBJECT_0: |
4105
84882a89ca50
Date: Thu, 27 Dec 2007 07:38:25 +0000
Sam Lantinga <slouken@libsdl.org>
parents:
1433
diff
changeset
|
108 InterlockedDecrement(&sem->count); |
0 | 109 retval = 0; |
110 break; | |
111 case WAIT_TIMEOUT: | |
112 retval = SDL_MUTEX_TIMEDOUT; | |
113 break; | |
114 default: | |
115 SDL_SetError("WaitForSingleObject() failed"); | |
116 retval = -1; | |
117 break; | |
118 } | |
119 return retval; | |
120 } | |
121 | |
122 int SDL_SemTryWait(SDL_sem *sem) | |
123 { | |
124 return SDL_SemWaitTimeout(sem, 0); | |
125 } | |
126 | |
127 int SDL_SemWait(SDL_sem *sem) | |
128 { | |
129 return SDL_SemWaitTimeout(sem, SDL_MUTEX_MAXWAIT); | |
130 } | |
131 | |
132 /* Returns the current count of the semaphore */ | |
133 Uint32 SDL_SemValue(SDL_sem *sem) | |
134 { | |
135 if ( ! sem ) { | |
136 SDL_SetError("Passed a NULL sem"); | |
137 return 0; | |
138 } | |
139 return sem->count; | |
140 } | |
141 | |
142 int SDL_SemPost(SDL_sem *sem) | |
143 { | |
144 if ( ! sem ) { | |
145 SDL_SetError("Passed a NULL sem"); | |
146 return -1; | |
147 } | |
148 /* Increase the counter in the first place, because | |
149 * after a successful release the semaphore may | |
150 * immediately get destroyed by another thread which | |
151 * is waiting for this semaphore. | |
152 */ | |
4105
84882a89ca50
Date: Thu, 27 Dec 2007 07:38:25 +0000
Sam Lantinga <slouken@libsdl.org>
parents:
1433
diff
changeset
|
153 InterlockedIncrement(&sem->count); |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
154 #if defined(_WIN32_WCE) && (_WIN32_WCE < 300) |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
155 if ( ReleaseSemaphoreCE(sem->id, 1, NULL) == FALSE ) { |
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
156 #else |
0 | 157 if ( ReleaseSemaphore(sem->id, 1, NULL) == FALSE ) { |
36
13ee9f4834ea
Windows CE patches contributed by Rainer Loritz
Sam Lantinga <slouken@lokigames.com>
parents:
0
diff
changeset
|
158 #endif |
4105
84882a89ca50
Date: Thu, 27 Dec 2007 07:38:25 +0000
Sam Lantinga <slouken@libsdl.org>
parents:
1433
diff
changeset
|
159 InterlockedDecrement(&sem->count); /* restore */ |
0 | 160 SDL_SetError("ReleaseSemaphore() failed"); |
161 return -1; | |
162 } | |
163 return 0; | |
164 } |