Pedro Alves wrote:
Pedro Alves wrote:
On Nov 21, 2007 2:13 PM, Pierre Muller wrote:
That's not what I see here. Can you show me a run where you get 4
only this patch applied?
I did try that, but posted a log of not doing it :-).
I've just tried about 30 times, and only once I did see
a 4 coming out ... oh, well, one of those things.
OK. Back at my home laptop, I can reproduce that with
no problems. Let me clarify what the 4 problem really is.
It's a race between gdb and the inferior.
Take this slightly changed test case. The only difference
to the original version is the extra Sleep call.
#include <windows.h>
HANDLE started;
HANDLE stop;
DWORD WINAPI
thread_start (void *arg)
{
SetEvent (started);
WaitForSingleObject (stop, INFINITE);
return 0;
}
int
main (int argc, char **argv)
{
int i;
DWORD suspend_count;
started = CreateEvent (NULL, TRUE, FALSE, NULL);
stop = CreateEvent (NULL, TRUE, FALSE, NULL);
HANDLE h = CreateThread (NULL, 0, thread_start, NULL,
0, NULL);
WaitForSingleObject (started, INFINITE);
for (i = 0; i < 3; i++)
if (SuspendThread (h) == (DWORD) -1)
{
printf ("SuspendThreadFailed\n");
return 1;
}
Sleep (300);
suspend_count = ResumeThread (h); /* set breakpoint here */
printf ("%lu\n", suspend_count); /* should be 3 */
while ((suspend_count = ResumeThread (h)) != 0
&& suspend_count != -1)
;
SetEvent (stop);
WaitForSingleObject (h, INFINITE);
CloseHandle (h);
CloseHandle (started);
CloseHandle (stop);
return 0;
}
If you do the "break at ...", "run", "thread 3", "continue"
sequence, and "..." is the "Sleep" line, you'll get 3,
but if you put the break at the /* set breakpoint here */
line, you'll get 4 (if you're (un)lucky).
The race happens due to the fact that gdb is
doing something similar to this:
win32_continue()
{
ContinueDebugEvent (...); /* Resumes all non suspended
threads of the process. */
/* At this point, gdb is running concurrently with
the inferior threads that were not suspended - which
included the main thread of the testcase. */
foreach t in threads do
if t is suspended
ResumeThread t
fi
done
}
If you break at the Sleep call, when we resume, gdb will
have a bit of time to call ResumeThread on the suspended
thread of the testcase. If you instead break at the
ResumeThread line, you'll have a good chance that the
inferior wins the race, hence the "4" result (remember
that ResumeThread returns the previous suspend count).
If we put something like this after the ResumeThread call:
(...)
suspend_count = ResumeThread (h); /* set breakpoint here */
+ Sleep (300);
+ SuspendThread (h);
+ suspend_count = ResumeThread (h);
printf ("%lu\n", suspend_count); /* should be 3 */
(...)
... you'll see that eventually gdb will bring the
suspend count back to 3. (A SuspendThread, ResumeThread
pair is the way to get at the suspend count.)
Since the watchpoint patch should fix this, what shall I do? Shall
I merge
the two and resubmit, or leave it at that ? They've already been
tested
together without regressions.
Here is the merge from the patch I posted at the start of the
thread with this patch:
[win32] Fix watchpoint support
http://sourceware.org/ml/gdb-patches/2007-11/msg00390.html
This patch fixes both the suspend_count
handling, and the watchpoint support.
Thanks Pierre, for looking at it.
OK ?
--
Pedro Alves
---
gdb/win32-nat.c | 80
+++++++++++++++++++++++++++++++-------------------------
1 file changed, 45 insertions(+), 35 deletions(-)
Index: src/gdb/win32-nat.c
===================================================================
--- src.orig/gdb/win32-nat.c 2007-11-11 23:13:04.000000000 +0000
+++ src/gdb/win32-nat.c 2007-11-21 22:39:56.000000000 +0000
@@ -112,14 +112,14 @@ static enum target_signal last_sig = TAR
/* Set if a signal was received from the debugged process */
/* Thread information structure used to track information that is
- not available in gdb's thread structure. */
+ not available in gdb's thread structure. */
typedef struct thread_info_struct
{
struct thread_info_struct *next;
DWORD id;
HANDLE h;
char *name;
- int suspend_count;
+ int suspended;
int reload_context;
CONTEXT context;
STACKFRAME sf;
@@ -244,9 +244,10 @@ check (BOOL ok, const char *file, int li
GetLastError ());
}
-/* Find a thread record given a thread id.
- If get_context then also retrieve the context for this
- thread. */
+/* Find a thread record given a thread id passed in ID. If
+ GET_CONTEXT is not 0, then also retrieve the context for this
+ thread. If GET_CONTEXT is negative, then don't suspend the
+ thread. */