This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [win32] Fix suspend count handling
On Fri, Nov 23, 2007 at 11:19:14AM +0100, Lerele wrote:
> Christopher Faylor escribi?:
>> 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.
>
>I think they are needed. They were anyway with the new gdbserver based
>(vs. Win32 API based) interrupt code I sent several days ago, and that
>so very kindly Pedro prepared for commitment, but that I still haven't
>found the time to sit down and look at them (however I'm absolutely
>sure they're just fine), I guess his changes must be similar to what I
>sent in the first place.
*Why* did you think you needed them in gdbserver? Did you actually try
not using them or did you just copy my code from win32-nat.c?
> Apart from this, there's also the case where (at least for gdbserver)
> socket data is received asynchronously while the child is running. This
> socket data could indicate gdbserver to set/enable/disable a breakpoint,
> read thread registers, etc., and this kind of things may require to stop
> the child using SuspendThread.
> Right?
I don't know exactly what you are referring to but if you are proposing
using SuspendThread/ResumeThread to work around non UNIX-like handling
of signals, I've previously mentioned that the use of SuspendThread is
dangerous since, the last I checked, Windows wasn't very good about not
avoiding reentracy problems if you suspend a thread and then use another
funcion from the Windows API. This used to cause strange random
behavior and even blue screens in older versions of Windows NT. I guess
it's possible that it's better these days but there is no way to really
know for sure.
cgf