This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [win32] Fix suspend count handling
Christopher Faylor wrote:
-/* 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.
That's what the GNU coding standards prefer:
[5.2 Commenting Your Work]
"The comment on a function is much clearer if you use the argument
names to speak about the argument values. The variable name
itself should be lower case, but write it in upper case when
you are speaking about the value rather than the variable
itself. Thus, “the node number NODE_NUM” rather than “an inode”."
In fact, I shouldn't be capitalizing ID. Fixed that.
@@ -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.
Added a #define and checked in (see attached).
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.
I wonder that too. The docs say that we must not retrieve a thread
context while the thread is running. When the inferior is stopped due
to a debug event the whole process is frozen, so in that case it should
safe. I can give it a try later on, but it may take me a while to
confirm if it really is safe, especially because I currently only
have access to Windows XP SP2 and WinCE (from the archives, I
get the impression Win2000 is pickier.)
OTOH, we'll still need to suspend threads if we want
to implement support for debugging one thread while the
others are running, or to step just one thread while
the others are stopped, or if we want to support stopping the
inferior without relying on a console available to handle
GenerateConsoleCtrlEvent; but these are different stories.
--
Pedro Alves
2007-11-24 Pedro Alves <pedro_alves@portugalmail.pt>
* win32-nat.c (DR6_CLEAR_VALUE): New define.
(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 DR6_CLEAR_VALUE.
(win32_continue): Set Dr6 to DR6_CLEAR_VALUE. Update usage of the
`suspended' flag. Do ContinueDebugEvent after resuming the
suspended threads, not before. Set threads' contexts before
resuming them, not after.
(win32_resume): Set Dr6 to DR6_CLEAR_VALUE.
---
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-24 11:44:14.000000000 +0000
+++ src/gdb/win32-nat.c 2007-11-24 11:44:56.000000000 +0000
@@ -91,6 +91,7 @@ enum
static unsigned dr[8];
static int debug_registers_changed;
static int debug_registers_used;
+#define DR6_CLEAR_VALUE 0xffff0ff0
/* The string sent by cygwin when it processes a signal.
FIXME: This should be in a cygwin include file. */
@@ -112,14 +113,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 +245,9 @@ 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. 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. */
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 = DR6_CLEAR_VALUE;
th->context.Dr7 = dr[7];
CHECK (SetThreadContext (th->h, &th->context));
th->context.ContextFlags = 0;
@@ -1122,32 +1131,34 @@ win32_continue (DWORD continue_status, i
current_event.dwProcessId, current_event.dwThreadId,
continue_status == DBG_CONTINUE ?
"DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
+
+ for (th = &thread_head; (th = th->next) != NULL;)
+ if ((id == -1 || id == (int) th->id)
+ && th->suspended)
+ {
+ if (debug_registers_changed)
+ {
+ th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
+ th->context.Dr0 = dr[0];
+ th->context.Dr1 = dr[1];
+ th->context.Dr2 = dr[2];
+ th->context.Dr3 = dr[3];
+ th->context.Dr6 = DR6_CLEAR_VALUE;
+ th->context.Dr7 = dr[7];
+ }
+ if (th->context.ContextFlags)
+ {
+ CHECK (SetThreadContext (th->h, &th->context));
+ th->context.ContextFlags = 0;
+ }
+ if (th->suspended > 0)
+ (void) ResumeThread (th->h);
+ th->suspended = 0;
+ }
+
res = ContinueDebugEvent (current_event.dwProcessId,
current_event.dwThreadId,
continue_status);
- if (res)
- for (th = &thread_head; (th = th->next) != NULL;)
- if (((id == -1) || (id == (int) th->id)) && th->suspend_count)
- {
-
- for (i = 0; i < th->suspend_count; i++)
- (void) ResumeThread (th->h);
- th->suspend_count = 0;
- if (debug_registers_changed)
- {
- /* Only change the value of the debug registers */
- th->context.ContextFlags = CONTEXT_DEBUG_REGISTERS;
- th->context.Dr0 = dr[0];
- 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.Dr7 = dr[7];
- CHECK (SetThreadContext (th->h, &th->context));
- th->context.ContextFlags = 0;
- }
- }
debug_registers_changed = 0;
return res;
@@ -1233,8 +1244,7 @@ win32_resume (ptid_t ptid, int step, enu
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 = DR6_CLEAR_VALUE;
th->context.Dr7 = dr[7];
}
CHECK (SetThreadContext (th->h, &th->context));