This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [win32] Fix suspend count handling
On Wed, Nov 21, 2007 at 11:18:38PM +0000, Pedro Alves wrote:
> 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
>2007-11-21 Pedro Alves <pedro_alves@portugalmail.pt>
>
> * win32-nat.c (thread_info_struct): Rename suspend_count to
> suspended, to be used as a flag.
> (thread_rec): Only suspend the thread if it wasn't suspended by
> gdb before. Warn if suspending failed.
> (win32_add_thread): Set Dr6 to 0xffff0ff0.
> (win32_resume): Likewise.
> (win32_continue): Set Dr6 to 0xffff0ff0. Update usage of the
> `suspended' flag. Do ContinueDebugEvent after resuming the
> suspended threads, not before. Set threads' contexts before
> resuming them, not after.
>---
> 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. */
I don't see any reason to capitalize get_context in the comment.
> static thread_info *
> thread_rec (DWORD id, int get_context)
> {
>@@ -255,12 +256,21 @@ thread_rec (DWORD id, int get_context)
> for (th = &thread_head; (th = th->next) != NULL;)
> if (th->id == id)
> {
>- if (!th->suspend_count && get_context)
>+ if (!th->suspended && get_context)
> {
> if (get_context > 0 && id != current_event.dwThreadId)
>- th->suspend_count = SuspendThread (th->h) + 1;
>+ {
>+ if (SuspendThread (th->h) == (DWORD) -1)
>+ {
>+ DWORD err = GetLastError ();
>+ warning (_("SuspendThread failed. (winerr %d)"),
>+ (int) err);
>+ return NULL;
>+ }
>+ th->suspended = 1;
>+ }
> else if (get_context < 0)
>- th->suspend_count = -1;
>+ th->suspended = -1;
> th->reload_context = 1;
> }
> return th;
>@@ -294,8 +304,7 @@ win32_add_thread (DWORD id, HANDLE h)
> th->context.Dr1 = dr[1];
> th->context.Dr2 = dr[2];
> th->context.Dr3 = dr[3];
>- /* th->context.Dr6 = dr[6];
>- FIXME: should we set dr6 also ?? */
>+ th->context.Dr6 = 0xffff0ff0;
This, and similar cases, needs to use a #define with an explanatory comment.
With the above minor changes, this looks ok.
I have to ask, however, if the SuspendThread's are even needed at all.
When I was writing this code, I wasn't entirely sure that gdb needed to
do anything like this but I erred on the side of caution. So, I'm
wondering if things would still work ok if the
SuspendThread/ResumeThread stuff was gone.
cgf