[PATCH] Fix "PC register is not available" issue

[PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

This problem was raised and mentioned several times over the last few
years.  It was discussed here in the thread which started with this
message:

  https://sourceware.org/ml/gdb-patches/2013-06/msg00237.html

In the ensuing discussion, there was a consensus that these failures
shouldn't be fatal, and perhaps we should even ignore them entirely.

The most annoying problem with this is that after the message about
failure to suspend a thread, there's another message:

  warning: SuspendThread (tid=0x720) failed. (winerr 5)
  PC register is not available  <<<<<<<<<<<<<<<<<<<<<<<<<

and the debug session becomes useless after that: the debuggee stops,
and you cannot continue it.  You can only examine memory.

A very similar, perhaps the same, problem is described here:

  https://sourceware.org/bugzilla/show_bug.cgi?id=14018

There you can find a short test program that demonstrates the issue,
and some analysis.  After hitting this problem myself many times, I
came to the same conclusion: namely, that GDB is trying to suspend a
thread for which it doesn't have the necessary privileges.  My
hypothesis is that those are the threads that Windows starts in the
context of the process being debugged, perhaps for the purposes of
handling some debug events.  (I can see the "New thread" messages from
time to time, although I know for certain that the inferior didn't
start any application threads.)

So I came up with the patch below.  The idea is that setting
th->suspended to -1 just signals to GDB that the thread isn't
suspended, and shouldn't be resumed; otherwise, we simply ignore the
failure to suspend the thread, although the warning is still printed.

I am running with this patch for almost a month, and the dreaded "PC
register is not available" message didn't appear even once.  No more
botched debugging sessions!  The test program in PR/14018 also runs
indefinitely until interrupted, instead of stopping.

So I suggest to install this.  OK?  Should I also close the Bugzilla
PR?

--- gdb/windows-nat.c~0	2014-02-06 04:21:29.000000000 +0200
+++ gdb/windows-nat.c	2014-02-26 22:27:10.225625000 +0200
@@ -313,9 +313,10 @@ thread_rec (DWORD id, int get_context)
 		    warning (_("SuspendThread (tid=0x%x) failed."
 			       " (winerr %u)"),
 			     (unsigned) id, (unsigned) err);
-		    return NULL;
+		    th->suspended = -1;
 		  }
-		th->suspended = 1;
+		else
+		  th->suspended = 1;
 	      }
 	    else if (get_context < 0)
 	      th->suspended = -1;

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

Hi Eli,

On Mon, Mar 17, 2014 at 09:43:23PM +0200, Eli Zaretskii wrote:
> This problem was raised and mentioned several times over the last few
> years.  It was discussed here in the thread which started with this
> message:
> 
>   https://sourceware.org/ml/gdb-patches/2013-06/msg00237.html

Thanks for looking into this!

> So I came up with the patch below.  The idea is that setting
> th->suspended to -1 just signals to GDB that the thread isn't
> suspended, and shouldn't be resumed; otherwise, we simply ignore the
> failure to suspend the thread, although the warning is still printed.
> 
> I am running with this patch for almost a month, and the dreaded "PC
> register is not available" message didn't appear even once.  No more
> botched debugging sessions!  The test program in PR/14018 also runs
> indefinitely until interrupted, instead of stopping.
> 
> So I suggest to install this.  OK?  Should I also close the Bugzilla
> PR?
> 
> --- gdb/windows-nat.c~0	2014-02-06 04:21:29.000000000 +0200
> +++ gdb/windows-nat.c	2014-02-26 22:27:10.225625000 +0200
> @@ -313,9 +313,10 @@ thread_rec (DWORD id, int get_context)
>  		    warning (_("SuspendThread (tid=0x%x) failed."
>  			       " (winerr %u)"),
>  			     (unsigned) id, (unsigned) err);
> -		    return NULL;
> +		    th->suspended = -1;
>  		  }
> -		th->suspended = 1;
> +		else
> +		  th->suspended = 1;
>  	      }
>  	    else if (get_context < 0)
>  	      th->suspended = -1;

Generally speaking, it seems to make sense to me that we would
mark as unsuspended threads that we cannot suspend. But one question
that rises from doing that is: how does the rest of GDB handle
this thread? In particular, does the thread show up in "info thread"
and is the user able to switch to that thread? etc? Certainly, if
the current situation is to leave the user stranded, the suggested
approach cannot only be an improvement...

Another thought I had on your patch is that we might want to limit
the warning to situation where the return code is not a permission
denied.

It would also have been nice to double-check that the thread in
question, when the error shows up, is indeed on a system thread
unrelated to our program (Eg: the thread created when we try to
interrupt the program with ctrl-c or ctrl-break). Not sure if
there is a way to determine that, though.

I would have looked into that, but I don't have much time this week, and
might be equally busy the following 2 weeks, so I didn't want to delay
my answer any further. Overall, your patch looks very promising.

Sorry I can't be anymore support for the moment.
-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Tue, 18 Mar 2014 09:16:08 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> Generally speaking, it seems to make sense to me that we would
> mark as unsuspended threads that we cannot suspend. But one question
> that rises from doing that is: how does the rest of GDB handle
> this thread? In particular, does the thread show up in "info thread"
> and is the user able to switch to that thread? etc?

I can try finding out, but it's not easy: since I made that change,
these situations became so much rarer that it's a challenge to bump
into them.  I'll see what I can do.

> Certainly, if the current situation is to leave the user stranded,
> the suggested approach cannot only be an improvement...

Exactly.

> Another thought I had on your patch is that we might want to limit
> the warning to situation where the return code is not a permission
> denied.

I'm not sure we should bother.  After all, if the problem is real, we
will get an error further down the line, when we use the handle to
that thread to do something with it.

IOW, I see no need to thrash the entire session because of something
that isn't fatal.

> It would also have been nice to double-check that the thread in
> question, when the error shows up, is indeed on a system thread
> unrelated to our program (Eg: the thread created when we try to
> interrupt the program with ctrl-c or ctrl-break). Not sure if
> there is a way to determine that, though.

I never use Ctrl-C/Ctrl/BREAK during debugging.  The threads that I
was talking about just appear out of thin air, when the debuggee hits
a breakpoint, for example.

> I would have looked into that, but I don't have much time this week, and
> might be equally busy the following 2 weeks, so I didn't want to delay
> my answer any further. Overall, your patch looks very promising.
> 
> Sorry I can't be anymore support for the moment.

There's no rush.  My problem is solved, so I can wait patiently ;-)

Thanks.

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

> > Another thought I had on your patch is that we might want to limit
> > the warning to situation where the return code is not a permission
> > denied.
> 
> I'm not sure we should bother.  After all, if the problem is real, we
> will get an error further down the line, when we use the handle to
> that thread to do something with it.
> 
> IOW, I see no need to thrash the entire session because of something
> that isn't fatal.

I didn't mean to change the behavior - only hide the warning.
In this case, if it is normal that we can't suspend the thread,
then there is no point in warning (scaring) the user about it.
I would only generate a warning if something abnormal that we should
fix occured.

> > It would also have been nice to double-check that the thread in
> > question, when the error shows up, is indeed on a system thread
> > unrelated to our program (Eg: the thread created when we try to
> > interrupt the program with ctrl-c or ctrl-break). Not sure if
> > there is a way to determine that, though.
> 
> I never use Ctrl-C/Ctrl/BREAK during debugging.  The threads that I
> was talking about just appear out of thin air, when the debuggee hits
> a breakpoint, for example.

I think we've seen that as well, I was just citing this thread as
an example, because that's a case where I know that an unrelated
thread gets created.

> There's no rush.  My problem is solved, so I can wait patiently ;-)

OK :-). but don't let me stop you. I think you have as good a handle
as I do, and perhaps others will comment as well. I think the only
part we need to look at before putting your patch in is analyzing
its side-effects. It cannot be all that bad, since you've been running
with the patch for quite a while, now.

-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Tue, 18 Mar 2014 09:54:13 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > > Another thought I had on your patch is that we might want to limit
> > > the warning to situation where the return code is not a permission
> > > denied.
> > 
> > I'm not sure we should bother.  After all, if the problem is real, we
> > will get an error further down the line, when we use the handle to
> > that thread to do something with it.
> > 
> > IOW, I see no need to thrash the entire session because of something
> > that isn't fatal.
> 
> I didn't mean to change the behavior - only hide the warning.

Ah, OK.  I think I'll do that in the next version of the patch.

> I think the only part we need to look at before putting your patch
> in is analyzing its side-effects.

I'll look into that and post the results.

Thanks.

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 03/18/2014 05:12 PM, Eli Zaretskii wrote:
>> Date: Tue, 18 Mar 2014 09:54:13 -0700
>> From: Joel Brobecker <brobecker@adacore.com>
>> Cc: gdb-patches@sourceware.org
>>
>>>> Another thought I had on your patch is that we might want to limit
>>>> the warning to situation where the return code is not a permission
>>>> denied.
>>>
>>> I'm not sure we should bother.  After all, if the problem is real, we
>>> will get an error further down the line, when we use the handle to
>>> that thread to do something with it.
>>>
>>> IOW, I see no need to thrash the entire session because of something
>>> that isn't fatal.
>>
>> I didn't mean to change the behavior - only hide the warning.
> 
> Ah, OK.  I think I'll do that in the next version of the patch.
> 
>> I think the only part we need to look at before putting your patch
>> in is analyzing its side-effects.
> 
> I'll look into that and post the results.

I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
actually read the registers off of this thread, and if so, what's the
thread's backtrace like.

(*) - note the other place that calls GetThreadContext does it under
a CHECK:
      CHECK (GetThreadContext (th->h, &th->context));
-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Tue, 18 Mar 2014 17:33:16 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org
> 
> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
> actually read the registers off of this thread

How to see those registers?

> and if so, what's the thread's backtrace like.

Why would we be interested in that info?

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 03/19/2014 03:40 AM, Eli Zaretskii wrote:
>> Date: Tue, 18 Mar 2014 17:33:16 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org
>>
>> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
>> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
>> actually read the registers off of this thread
> 
> How to see those registers?

Just "info registers" ?

If we can't even read registers off of it, and GetThreadContext
is failing, it means after your patch we'll be showing bogus
register contents for these threads.  But I think GetThreadContext
will indeed succeed for these threads.

AFAIK, we don't really need the SuspendThread calls when handling
a debug event, given that when WaitForDebugEvent returns a
stop event, all threads have already been stopped by the OS for us.
We really only need to SuspendThread threads when we might want
to leave most threads paused on the next resume, for e.g., when
stepping over a breakpoint.  The suspend count handling in
windows-nat.c is quite messy, and looking at the code, it doesn't
look like we actually get that right, given we only SuspendThread
threads if we try to read their registers, and so if nothing reads
registers off all threads when e.g., handling a breakpoint that
we decide needs to be stepped over (which we don't), then we end
up resuming threads we shouldn't.  But it might be I'm missing
something.  I wonder whether schedlock.exp is passing on
Windows/Cygwin cleanly.  IMO, this whole SuspendThread business
is ripe for simplification/cleanup.

>> and if so, what's the thread's backtrace like.
> 
> Why would we be interested in that info?

It'll likely show us the thread is stopped at some ntdll.dll function
or some such, and from the function name we will likely
be able to infer what/which thread is this, like, e.g., whether
it's a thread injected with DebugBreakProcess or some such
(internally by one of the system dlls or the dlls your app
links with).

-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Wed, 19 Mar 2014 10:06:51 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: brobecker@adacore.com, gdb-patches@sourceware.org
> 
> On 03/19/2014 03:40 AM, Eli Zaretskii wrote:
> >> Date: Tue, 18 Mar 2014 17:33:16 +0000
> >> From: Pedro Alves <palves@redhat.com>
> >> CC: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org
> >>
> >> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
> >> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
> >> actually read the registers off of this thread
> > 
> > How to see those registers?
> 
> Just "info registers" ?

That's what I thought, but ...

> If we can't even read registers off of it, and GetThreadContext
> is failing, it means after your patch we'll be showing bogus
> register contents for these threads.

...how do you tell bogus register contents from correct contents?
It's not like I know which register should have what value at any
given time, do I?

> But I think GetThreadContext will indeed succeed for these threads.

Well, at least MSDN begs to differ:

  You cannot get a valid context for a running thread. Use the
  SuspendThread function to suspend the thread before calling
  GetThreadContext.

> AFAIK, we don't really need the SuspendThread calls when handling
> a debug event, given that when WaitForDebugEvent returns a
> stop event, all threads have already been stopped by the OS for us.

Yes, AFAIK that's true.

> We really only need to SuspendThread threads when we might want
> to leave most threads paused on the next resume, for e.g., when
> stepping over a breakpoint.  The suspend count handling in
> windows-nat.c is quite messy, and looking at the code, it doesn't
> look like we actually get that right, given we only SuspendThread
> threads if we try to read their registers, and so if nothing reads
> registers off all threads when e.g., handling a breakpoint that
> we decide needs to be stepped over (which we don't), then we end
> up resuming threads we shouldn't.

That's assuming that stepping resumes threads.  I'm not sure, but I
really don't know enough about debugging APIs on Windows.

> It'll likely show us the thread is stopped at some ntdll.dll function
> or some such, and from the function name we will likely
> be able to infer what/which thread is this, like, e.g., whether
> it's a thread injected with DebugBreakProcess or some such
> (internally by one of the system dlls or the dlls your app
> links with).

I'll see what I can find about that, but I doubt you'd see something
telltale in the backtrace.  (The thread started by Windows for
debugging is not part of this issue; I never saw the threads that are
to have any debug-related functions on their callstacks.)

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 03/19/2014 04:24 PM, Eli Zaretskii wrote:
>> Date: Wed, 19 Mar 2014 10:06:51 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: brobecker@adacore.com, gdb-patches@sourceware.org
>>
>> On 03/19/2014 03:40 AM, Eli Zaretskii wrote:
>>>> Date: Tue, 18 Mar 2014 17:33:16 +0000
>>>> From: Pedro Alves <palves@redhat.com>
>>>> CC: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org
>>>>
>>>> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
>>>> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
>>>> actually read the registers off of this thread
>>>
>>> How to see those registers?
>>
>> Just "info registers" ?
> 
> That's what I thought, but ...
> 
>> If we can't even read registers off of it, and GetThreadContext
>> is failing, it means after your patch we'll be showing bogus
>> register contents for these threads.
> 
> ...how do you tell bogus register contents from correct contents?
> It's not like I know which register should have what value at any
> given time, do I?

The point is that GDB ignores GetThreadContext errors, and so
if indeed GetThreadContext fails, GDB happily proceeds
decoding a bogus th->context.  I mean, we should do this
in do_windows_fetch_inferior_registers:

-      GetThreadContext (th->h, &th->context);
+      CHECK (GetThreadContext (th->h, &th->context));

So that GetThreadContext fails, we at least see a warning.
I assume that if GetThreadContext does not fail, then the
register contents are correct.

> 
>> But I think GetThreadContext will indeed succeed for these threads.
> 
> Well, at least MSDN begs to differ:
> 
>   You cannot get a valid context for a running thread. Use the
>   SuspendThread function to suspend the thread before calling
>   GetThreadContext.

I mean it'll succeed because we only ever read registers when
threads are stopped for debug event.  I don't mean to imply
that those threads are special WRT to GetThreadContext.  It's
not valid to get a context for a _running_ thread.  But after a
debug event, no thread is running at all.  The OS already
stopped threads for us.

> 
>> AFAIK, we don't really need the SuspendThread calls when handling
>> a debug event, given that when WaitForDebugEvent returns a
>> stop event, all threads have already been stopped by the OS for us.
> 
> Yes, AFAIK that's true.

Alright, we were talking past each other then.

I did a little websearch, and I found evidence of other debuggers
also not using SuspendThread after events:

 http://www.ollydbg.de/Help/t_run.htm

"indebugevent
Application is paused on debug event, therefore Suspendallthreads() does not need to call SuspendThread()"

> 
>> We really only need to SuspendThread threads when we might want
>> to leave most threads paused on the next resume, for e.g., when
>> stepping over a breakpoint.  The suspend count handling in
>> windows-nat.c is quite messy, and looking at the code, it doesn't
>> look like we actually get that right, given we only SuspendThread
>> threads if we try to read their registers, and so if nothing reads
>> registers off all threads when e.g., handling a breakpoint that
>> we decide needs to be stepped over (which we don't), then we end
>> up resuming threads we shouldn't.
> 
> That's assuming that stepping resumes threads.  I'm not sure, but I
> really don't know enough about debugging APIs on Windows.

There's no special step request in the debug API.  The way to set
a thread stepping is to enable the trace flag in eflags:

      if (step)
	{
	  /* Single step by setting t bit.  */
	  struct regcache *regcache = get_current_regcache ();
	  struct gdbarch *gdbarch = get_regcache_arch (regcache);
	  windows_fetch_inferior_registers (ops, regcache,
					    gdbarch_ps_regnum (gdbarch));
	  th->context.EFlags |= FLAG_TRACE_BIT;
	}

>> It'll likely show us the thread is stopped at some ntdll.dll function
>> or some such, and from the function name we will likely
>> be able to infer what/which thread is this, like, e.g., whether
>> it's a thread injected with DebugBreakProcess or some such
>> (internally by one of the system dlls or the dlls your app
>> links with).
> 
> I'll see what I can find about that, but I doubt you'd see something
> telltale in the backtrace.  (The thread started by Windows for
> debugging is not part of this issue; I never saw the threads that are
> to have any debug-related functions on their callstacks.)

Thanks!

-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

This describes the results of my looking into this issue, given the
comments and suggestions by Joel and Pedro.  Sorry about the length.

> I didn't mean to change the behavior - only hide the warning.
> In this case, if it is normal that we can't suspend the thread,
> then there is no point in warning (scaring) the user about it.
> I would only generate a warning if something abnormal that we should
> fix occured.

The patch near the end of this message indeed includes code to ignore
the warning in these cases.

> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
> actually read the registers off of this thread, and if so, what's the
> thread's backtrace like.

I added CHECK to that call to GetThreadContext.  It never produced a
warning in all my testing, and it looks like we do succeed to get the
registers.  At least the registers of 2 such threads show different
contents, and the EIP value is consistent with what "info threads"
displays.

> >> and if so, what's the thread's backtrace like.
> > 
> > Why would we be interested in that info?
> 
> It'll likely show us the thread is stopped at some ntdll.dll function
> or some such, and from the function name we will likely
> be able to infer what/which thread is this, like, e.g., whether
> it's a thread injected with DebugBreakProcess or some such
> (internally by one of the system dlls or the dlls your app
> links with).

I can show you 2 typical examples.  This is from Emacs, where the
application has 3 threads, and one more is started by the debugger.
The rest, threads 5 and 6 in these examples, are those mysterious
threads we are talking about.

  (gdb) info threads
    Id   Target Id         Frame
    6    Thread 15492.0x1f28 0x77a41f46 in ntdll!ZwWaitForWorkViaWorkerFactory
      () from C:\Windows\system32\ntdll.dll
    5    Thread 15492.0x73c0 0x77a41f46 in ntdll!ZwWaitForWorkViaWorkerFactory
      () from C:\Windows\system32\ntdll.dll
    4    Thread 15492.0x2300 0x75ac78d7 in USER32!DispatchMessageW ()
     from C:\Windows\syswow64\user32.dll
    3    Thread 15492.0x1860 0x77a3fd91 in ntdll!ZwDelayExecution ()
     from C:\Windows\system32\ntdll.dll
    2    Thread 15492.0x2410 0x77a4015d in ntdll!ZwWaitForMultipleObjects ()
     from C:\Windows\system32\ntdll.dll
  * 1    Thread 15492.0x44a0 cleanup_vector (vector=0x62daeb0) at alloc.c:2917

  (gdb) info threads
    Id   Target Id         Frame
    6    Thread 15492.0x1f28 0x77a3f8d1 in ntdll!ZwWaitForSingleObject ()
     from C:\Windows\system32\ntdll.dll
    5    Thread 15492.0x73c0 0x77a72880 in ntdll!RtlFillMemoryUlong ()
     from C:\Windows\system32\ntdll.dll
    4    Thread 15492.0x2300 0x75ac78d7 in USER32!DispatchMessageW ()
     from C:\Windows\syswow64\user32.dll
    3    Thread 15492.0x1860 0x77a3fd91 in ntdll!ZwDelayExecution ()
     from C:\Windows\system32\ntdll.dll
    2    Thread 15492.0x2410 0x77a4015d in ntdll!ZwWaitForMultipleObjects ()
     from C:\Windows\system32\ntdll.dll
  * 1    Thread 15492.0x44a0 cleanup_vector (vector=0x388ca58) at alloc.c:2917

The first display is what I usually see: several (I've seen up to 4)
threads waiting inside ZwWaitForWorkViaWorkerFactory.  But sometimes
they do perform some work, as can be seen from the second display.

My theory is that we get those Access Denied (winerr = 5) errors when
we try to suspend these threads when they are about to exit.  That's
because I normally see a "Thread exited" message immediately after the
warning with the error code.  However, testing whether the thread
exited, and avoiding the SuspendThread call if it did, didn't stop the
warnings, so I guess the issue is more subtle.  E.g., it could be that
during the final stages of thread shutdown, the thread runs some
privileged code or something, which precludes suspension.  It might
also be related to the fact that we are debugging 32-bit threads on a
64-bit OS, so WOW64 is at work here.

In any case, the patch below ignores Access Denied errors from
SuspendThread.

In addition, I tried to solve warnings like these:

  error return windows-nat.c:1306 was 5
  [Thread 15492.0x68a0 exited with code 0]
  (gdb) q
  A debugging session is active.

	  Inferior 1 [process 15492] will be killed.

  Quit anyway? (y or n) y
  error return windows-nat.c:1306 was 5

These come from SetThreadContext in windows_continue.  The second
occurrence is because we already killed the inferior by calling
TerminateProcess, so its threads begin to shut down, and trying to set
their context might indeed fail.  The first warning is about one of
those same threads, and again happens just before the thread exit is
announced.

My solution, which you can see below, is (a) pass an additional
parameter to windows_continue telling it that the inferior was killed,
in which case it doesn't bother checking errors from the
SetThreadContext call; and (b) check if the thread already exited, and
if so, avoid calling SetThreadContext on it.  This completely avoids
the warning when killing the inferior, and removes most (but not all)
of the warnings for the other threads.

Finally, here's the full patch.  I hope this research answered all the
questions, and we can now get the patch in.

--- gdb/windows-nat.c~0	2014-02-06 04:21:29.000000000 +0200
+++ gdb/windows-nat.c	2014-03-25 19:18:16.814250000 +0200
@@ -310,12 +310,18 @@ thread_rec (DWORD id, int get_context)
 		  {
 		    DWORD err = GetLastError ();
 
-		    warning (_("SuspendThread (tid=0x%x) failed."
-			       " (winerr %u)"),
-			     (unsigned) id, (unsigned) err);
-		    return NULL;
+		    /* We get Access Denied (5) when trying to suspend
+		       threads that Windows started on behalf of the
+		       debuggee, usually when those threads are just
+		       about to exit.  */
+		    if (err != ERROR_ACCESS_DENIED)
+		      warning (_("SuspendThread (tid=0x%x) failed."
+				 " (winerr %u)"),
+			       (unsigned) id, (unsigned) err);
+		    th->suspended = -1;
 		  }
-		th->suspended = 1;
+		else
+		  th->suspended = 1;
 	      }
 	    else if (get_context < 0)
 	      th->suspended = -1;
@@ -445,7 +451,7 @@ do_windows_fetch_inferior_registers (str
 	{
 	  thread_info *th = current_thread;
 	  th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
-	  GetThreadContext (th->h, &th->context);
+	  CHECK (GetThreadContext (th->h, &th->context));
 	  /* Copy dr values from that thread.
 	     But only if there were not modified since last stop.
 	     PR gdb/2388 */
@@ -1266,10 +1272,12 @@ handle_exception (struct target_waitstat
   return 1;
 }
 
-/* Resume all artificially suspended threads if we are continuing
-   execution.  */
+/* Resume thread specified by ID, or all artificially suspended
+   threads, if we are continuing execution.  KILLED non-zero means we
+   have killed the inferior, so we should ignore weird errors due to
+   threads shutting down.  */
 static BOOL
-windows_continue (DWORD continue_status, int id)
+windows_continue (DWORD continue_status, int id, int killed)
 {
   int i;
   thread_info *th;
@@ -1297,7 +1305,16 @@ windows_continue (DWORD continue_status,
 	  }
 	if (th->context.ContextFlags)
 	  {
-	    CHECK (SetThreadContext (th->h, &th->context));
+	    DWORD ec = 0;
+
+	    if (GetExitCodeThread (th->h, &ec)
+		&& ec == STILL_ACTIVE)
+	      {
+		if (killed)
+		  SetThreadContext (th->h, &th->context);
+		else
+		  CHECK (SetThreadContext (th->h, &th->context));
+	      }
 	    th->context.ContextFlags = 0;
 	  }
 	if (th->suspended > 0)
@@ -1425,9 +1442,9 @@ windows_resume (struct target_ops *ops,
      Otherwise complain.  */
 
   if (resume_all)
-    windows_continue (continue_status, -1);
+    windows_continue (continue_status, -1, 0);
   else
-    windows_continue (continue_status, ptid_get_tid (ptid));
+    windows_continue (continue_status, ptid_get_tid (ptid), 0);
 }
 
 /* Ctrl-C handler used when the inferior is not run in the same console.  The
@@ -1645,7 +1662,7 @@ get_windows_debug_event (struct target_o
       if (continue_status == -1)
 	windows_resume (ops, minus_one_ptid, 0, 1);
       else
-	CHECK (windows_continue (continue_status, -1));
+	CHECK (windows_continue (continue_status, -1, 0));
     }
   else
     {
@@ -2382,13 +2399,13 @@ windows_create_inferior (struct target_o
 
   do_initial_windows_stuff (ops, pi.dwProcessId, 0);
 
-  /* windows_continue (DBG_CONTINUE, -1); */
+  /* windows_continue (DBG_CONTINUE, -1, 0); */
 }
 
 static void
 windows_mourn_inferior (struct target_ops *ops)
 {
-  (void) windows_continue (DBG_CONTINUE, -1);
+  (void) windows_continue (DBG_CONTINUE, -1, 0);
   i386_cleanup_dregs();
   if (open_process_used)
     {
@@ -2456,7 +2473,7 @@ windows_kill_inferior (struct target_ops
 
   for (;;)
     {
-      if (!windows_continue (DBG_CONTINUE, -1))
+      if (!windows_continue (DBG_CONTINUE, -1, 1))
 	break;
       if (!WaitForDebugEvent (&current_event, INFINITE))
 	break;

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

Hi Eli,

Thanks for doing all this.

> I added CHECK to that call to GetThreadContext.  It never produced a
> warning in all my testing, and it looks like we do succeed to get the
> registers.  At least the registers of 2 such threads show different
> contents, and the EIP value is consistent with what "info threads"
> displays.

I think "info threads" uses those register values to produce that
listing, so the consistency would be expected. But as long as you
have meaningful values, I think that's some evidence of success.

> My theory is that we get those Access Denied (winerr = 5) errors when
> we try to suspend these threads when they are about to exit.  That's
> because I normally see a "Thread exited" message immediately after the
> warning with the error code.  However, testing whether the thread
> exited, and avoiding the SuspendThread call if it did, didn't stop the
> warnings, so I guess the issue is more subtle.  E.g., it could be that
> during the final stages of thread shutdown, the thread runs some
> privileged code or something, which precludes suspension.

Perhaps a simpler theory is that these threads are typically
short-lived, and so you'd be always be seeing their exit soon
after they are created.

> In any case, the patch below ignores Access Denied errors from
> SuspendThread.

I think that makes sense.

> In addition, I tried to solve warnings like these:
> 
>   error return windows-nat.c:1306 was 5
>   [Thread 15492.0x68a0 exited with code 0]
>   (gdb) q
>   A debugging session is active.
> 
> 	  Inferior 1 [process 15492] will be killed.
> 
>   Quit anyway? (y or n) y
>   error return windows-nat.c:1306 was 5

Yay! :)

> These come from SetThreadContext in windows_continue.  The second
> occurrence is because we already killed the inferior by calling
> TerminateProcess, so its threads begin to shut down, and trying to set
> their context might indeed fail.  The first warning is about one of
> those same threads, and again happens just before the thread exit is
> announced.
> 
> My solution, which you can see below, is (a) pass an additional
> parameter to windows_continue telling it that the inferior was killed,
> in which case it doesn't bother checking errors from the
> SetThreadContext call; and (b) check if the thread already exited, and
> if so, avoid calling SetThreadContext on it.  This completely avoids
> the warning when killing the inferior, and removes most (but not all)
> of the warnings for the other threads.

I am missing the command that caused the first error. From what you
are saying, was it "kill"? If it was, it seems to me that if we
cured the first error, the second one would not happen. Otherwise,
it'd be interesting to understand why we send a TerminateProcess
first, and then try to SetThreadContext later on. It does not seem
to make sense.

Perhaps one way to address the problem more globally is to have
a version of the CHECK macro that ignores access-denied errors,
and use this one on operations where we know we might get that
error?

> Finally, here's the full patch.  I hope this research answered all the
> questions, and we can now get the patch in.

I'm good with the first half of the patch that handles SuspendThread
more gracefully. For the additional argument to windows_continue,
I propose we handle that as a separate patch. It's the right thing
to do procedurally anyway, and it'll give us a chance to investigate
more without holding the first half up.

Pedro, WDYT?

> 
> --- gdb/windows-nat.c~0	2014-02-06 04:21:29.000000000 +0200
> +++ gdb/windows-nat.c	2014-03-25 19:18:16.814250000 +0200
> @@ -310,12 +310,18 @@ thread_rec (DWORD id, int get_context)
>  		  {
>  		    DWORD err = GetLastError ();
>  
> -		    warning (_("SuspendThread (tid=0x%x) failed."
> -			       " (winerr %u)"),
> -			     (unsigned) id, (unsigned) err);
> -		    return NULL;
> +		    /* We get Access Denied (5) when trying to suspend
> +		       threads that Windows started on behalf of the
> +		       debuggee, usually when those threads are just
> +		       about to exit.  */
> +		    if (err != ERROR_ACCESS_DENIED)
> +		      warning (_("SuspendThread (tid=0x%x) failed."
> +				 " (winerr %u)"),
> +			       (unsigned) id, (unsigned) err);
> +		    th->suspended = -1;
>  		  }
> -		th->suspended = 1;
> +		else
> +		  th->suspended = 1;
>  	      }
>  	    else if (get_context < 0)
>  	      th->suspended = -1;
> @@ -445,7 +451,7 @@ do_windows_fetch_inferior_registers (str
>  	{
>  	  thread_info *th = current_thread;
>  	  th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
> -	  GetThreadContext (th->h, &th->context);
> +	  CHECK (GetThreadContext (th->h, &th->context));
>  	  /* Copy dr values from that thread.
>  	     But only if there were not modified since last stop.
>  	     PR gdb/2388 */
> @@ -1266,10 +1272,12 @@ handle_exception (struct target_waitstat
>    return 1;
>  }
>  
> -/* Resume all artificially suspended threads if we are continuing
> -   execution.  */
> +/* Resume thread specified by ID, or all artificially suspended
> +   threads, if we are continuing execution.  KILLED non-zero means we
> +   have killed the inferior, so we should ignore weird errors due to
> +   threads shutting down.  */
>  static BOOL
> -windows_continue (DWORD continue_status, int id)
> +windows_continue (DWORD continue_status, int id, int killed)
>  {
>    int i;
>    thread_info *th;
> @@ -1297,7 +1305,16 @@ windows_continue (DWORD continue_status,
>  	  }
>  	if (th->context.ContextFlags)
>  	  {
> -	    CHECK (SetThreadContext (th->h, &th->context));
> +	    DWORD ec = 0;
> +
> +	    if (GetExitCodeThread (th->h, &ec)
> +		&& ec == STILL_ACTIVE)
> +	      {
> +		if (killed)
> +		  SetThreadContext (th->h, &th->context);
> +		else
> +		  CHECK (SetThreadContext (th->h, &th->context));
> +	      }
>  	    th->context.ContextFlags = 0;
>  	  }
>  	if (th->suspended > 0)
> @@ -1425,9 +1442,9 @@ windows_resume (struct target_ops *ops,
>       Otherwise complain.  */
>  
>    if (resume_all)
> -    windows_continue (continue_status, -1);
> +    windows_continue (continue_status, -1, 0);
>    else
> -    windows_continue (continue_status, ptid_get_tid (ptid));
> +    windows_continue (continue_status, ptid_get_tid (ptid), 0);
>  }
>  
>  /* Ctrl-C handler used when the inferior is not run in the same console.  The
> @@ -1645,7 +1662,7 @@ get_windows_debug_event (struct target_o
>        if (continue_status == -1)
>  	windows_resume (ops, minus_one_ptid, 0, 1);
>        else
> -	CHECK (windows_continue (continue_status, -1));
> +	CHECK (windows_continue (continue_status, -1, 0));
>      }
>    else
>      {
> @@ -2382,13 +2399,13 @@ windows_create_inferior (struct target_o
>  
>    do_initial_windows_stuff (ops, pi.dwProcessId, 0);
>  
> -  /* windows_continue (DBG_CONTINUE, -1); */
> +  /* windows_continue (DBG_CONTINUE, -1, 0); */
>  }
>  
>  static void
>  windows_mourn_inferior (struct target_ops *ops)
>  {
> -  (void) windows_continue (DBG_CONTINUE, -1);
> +  (void) windows_continue (DBG_CONTINUE, -1, 0);
>    i386_cleanup_dregs();
>    if (open_process_used)
>      {
> @@ -2456,7 +2473,7 @@ windows_kill_inferior (struct target_ops
>  
>    for (;;)
>      {
> -      if (!windows_continue (DBG_CONTINUE, -1))
> +      if (!windows_continue (DBG_CONTINUE, -1, 1))
>  	break;
>        if (!WaitForDebugEvent (&current_event, INFINITE))
>  	break;
> 

-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Thu, 27 Mar 2014 05:56:46 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
> 
> > My theory is that we get those Access Denied (winerr = 5) errors when
> > we try to suspend these threads when they are about to exit.  That's
> > because I normally see a "Thread exited" message immediately after the
> > warning with the error code.  However, testing whether the thread
> > exited, and avoiding the SuspendThread call if it did, didn't stop the
> > warnings, so I guess the issue is more subtle.  E.g., it could be that
> > during the final stages of thread shutdown, the thread runs some
> > privileged code or something, which precludes suspension.
> 
> Perhaps a simpler theory is that these threads are typically
> short-lived, and so you'd be always be seeing their exit soon
> after they are created.

They live for several minutes, so not so short-lived.

> > In addition, I tried to solve warnings like these:
> > 
> >   error return windows-nat.c:1306 was 5
> >   [Thread 15492.0x68a0 exited with code 0]
> >   (gdb) q
> >   A debugging session is active.
> > 
> > 	  Inferior 1 [process 15492] will be killed.
> > 
> >   Quit anyway? (y or n) y
> >   error return windows-nat.c:1306 was 5
> 
> Yay! :)
> 
> > These come from SetThreadContext in windows_continue.  The second
> > occurrence is because we already killed the inferior by calling
> > TerminateProcess, so its threads begin to shut down, and trying to set
> > their context might indeed fail.  The first warning is about one of
> > those same threads, and again happens just before the thread exit is
> > announced.
> > 
> > My solution, which you can see below, is (a) pass an additional
> > parameter to windows_continue telling it that the inferior was killed,
> > in which case it doesn't bother checking errors from the
> > SetThreadContext call; and (b) check if the thread already exited, and
> > if so, avoid calling SetThreadContext on it.  This completely avoids
> > the warning when killing the inferior, and removes most (but not all)
> > of the warnings for the other threads.
> 
> I am missing the command that caused the first error. From what you
> are saying, was it "kill"?

No, it was "continue", which caused the inferior to run until a
breakpoint, at which point I typed "q".  The thread exit event
happened while the inferior was running.

> it'd be interesting to understand why we send a TerminateProcess
> first, and then try to SetThreadContext later on. It does not seem
> to make sense.

That happens because windows_kill_inferior does this:

  static void
  windows_kill_inferior (struct target_ops *ops)
  {
    CHECK (TerminateProcess (current_process_handle, 0));

    for (;;)
      {
	if (!windows_continue (DBG_CONTINUE, -1, 1))
	  break;
	if (!WaitForDebugEvent (&current_event, INFINITE))
	  break;
	if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
	  break;
      }

    target_mourn_inferior ();	/* Or just windows_mourn_inferior?  */
  }

IOW, we resume the inferior (perhaps to collect all the debug
events?).

> Perhaps one way to address the problem more globally is to have
> a version of the CHECK macro that ignores access-denied errors,
> and use this one on operations where we know we might get that
> error?

We only have one or 2 places for that right now, so I wouldn't think a
separate macro is justified.

> > Finally, here's the full patch.  I hope this research answered all the
> > questions, and we can now get the patch in.
> 
> I'm good with the first half of the patch that handles SuspendThread
> more gracefully. For the additional argument to windows_continue,
> I propose we handle that as a separate patch. It's the right thing
> to do procedurally anyway, and it'll give us a chance to investigate
> more without holding the first half up.

Given what I told above, what additional investigations are needed?

Note that the second part is not entirely separate: those phantom
threads hit the problem with SetThreadContext as well, and checking
whether the thread already exited does let through fewer of those
warnings.

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

> > it'd be interesting to understand why we send a TerminateProcess
> > first, and then try to SetThreadContext later on. It does not seem
> > to make sense.
> 
> That happens because windows_kill_inferior does this:
> 
>   static void
>   windows_kill_inferior (struct target_ops *ops)
>   {
>     CHECK (TerminateProcess (current_process_handle, 0));
> 
>     for (;;)
>       {
> 	if (!windows_continue (DBG_CONTINUE, -1, 1))
> 	  break;
> 	if (!WaitForDebugEvent (&current_event, INFINITE))
> 	  break;
> 	if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
> 	  break;
>       }
> 
>     target_mourn_inferior ();	/* Or just windows_mourn_inferior?  */
>   }
> 
> IOW, we resume the inferior (perhaps to collect all the debug
> events?).

I see - I didn't realize we were doing that, but a quick read
of the corresponding MSDN page confirms that this is necessary:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx

> Given what I told above, what additional investigations are needed?
> 
> Note that the second part is not entirely separate: those phantom
> threads hit the problem with SetThreadContext as well, and checking
> whether the thread already exited does let through fewer of those
> warnings.

In light of everything, I have no further comment at the moment
regarding your latest patch. Perhaps one tiny detail:

> +             if (killed)
> +               SetThreadContext (th->h, &th->context);
> +             else
> +               CHECK (SetThreadContext (th->h, &th->context));

Rather than duplicate the call to SetThreadContext, perhaps
another way of doing it would be:

    DWORD status;

    status = SetThreadContext (th->h, &th->context);
    if (!killed)
      CHECK (status)

(not a strong suggestion, feel free to ignore)

-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 03/26/2014 06:49 PM, Eli Zaretskii wrote:
> This describes the results of my looking into this issue, given the
> comments and suggestions by Joel and Pedro.  Sorry about the length.
> 
>> I didn't mean to change the behavior - only hide the warning.
>> In this case, if it is normal that we can't suspend the thread,
>> then there is no point in warning (scaring) the user about it.
>> I would only generate a warning if something abnormal that we should
>> fix occured.
> 
> The patch near the end of this message indeed includes code to ignore
> the warning in these cases.
> 
>> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
>> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
>> actually read the registers off of this thread, and if so, what's the
>> thread's backtrace like.
> 
> I added CHECK to that call to GetThreadContext.  It never produced a
> warning in all my testing, and it looks like we do succeed to get the
> registers.  At least the registers of 2 such threads show different
> contents, and the EIP value is consistent with what "info threads"
> displays.

It isn't clear to me whether you're saying that you saw the
SuspendThread failure trigger in all your new testing, so that
we'd know for sure whether GetThreadContext suceeds in that case,
or whether it might have been that you just were "lucky" enough
to not trigger the SuspendThread failure issue.

Does your patch fix the test case in PR14018, without producing
a CHECK warning from the new CHECK in GetThreadContext you've
added?  That'd confirm it, I think.

> I can show you 2 typical examples.  This is from Emacs, where the
> application has 3 threads, and one more is started by the debugger.
> The rest, threads 5 and 6 in these examples, are those mysterious
> threads we are talking about.
> 
>   (gdb) info threads
>     Id   Target Id         Frame
>     6    Thread 15492.0x1f28 0x77a41f46 in ntdll!ZwWaitForWorkViaWorkerFactory
>       () from C:\Windows\system32\ntdll.dll
>     5    Thread 15492.0x73c0 0x77a41f46 in ntdll!ZwWaitForWorkViaWorkerFactory
>       () from C:\Windows\system32\ntdll.dll
>     4    Thread 15492.0x2300 0x75ac78d7 in USER32!DispatchMessageW ()
>      from C:\Windows\syswow64\user32.dll
>     3    Thread 15492.0x1860 0x77a3fd91 in ntdll!ZwDelayExecution ()
>      from C:\Windows\system32\ntdll.dll
>     2    Thread 15492.0x2410 0x77a4015d in ntdll!ZwWaitForMultipleObjects ()
>      from C:\Windows\system32\ntdll.dll
>   * 1    Thread 15492.0x44a0 cleanup_vector (vector=0x62daeb0) at alloc.c:2917
> 
>   (gdb) info threads
>     Id   Target Id         Frame
>     6    Thread 15492.0x1f28 0x77a3f8d1 in ntdll!ZwWaitForSingleObject ()
>      from C:\Windows\system32\ntdll.dll
>     5    Thread 15492.0x73c0 0x77a72880 in ntdll!RtlFillMemoryUlong ()
>      from C:\Windows\system32\ntdll.dll
>     4    Thread 15492.0x2300 0x75ac78d7 in USER32!DispatchMessageW ()
>      from C:\Windows\syswow64\user32.dll
>     3    Thread 15492.0x1860 0x77a3fd91 in ntdll!ZwDelayExecution ()
>      from C:\Windows\system32\ntdll.dll
>     2    Thread 15492.0x2410 0x77a4015d in ntdll!ZwWaitForMultipleObjects ()
>      from C:\Windows\system32\ntdll.dll
>   * 1    Thread 15492.0x44a0 cleanup_vector (vector=0x388ca58) at alloc.c:2917
>
> The first display is what I usually see: several (I've seen up to 4)
> threads waiting inside ZwWaitForWorkViaWorkerFactory.  But sometimes
> they do perform some work, as can be seen from the second display.

OK, but these don't appear to be backtraces taken right after
SuspendThread failed.  That is, you're showing that these threads
exist during your normal emacs debugging.  IOW, you're not showing
where the thread is actually stopped when the SuspendThread failed.

> In any case, the patch below ignores Access Denied errors from
> SuspendThread.
> 
> In addition, I tried to solve warnings like these:
> 
>   error return windows-nat.c:1306 was 5
>   [Thread 15492.0x68a0 exited with code 0]
>   (gdb) q
>   A debugging session is active.
> 
> 	  Inferior 1 [process 15492] will be killed.
> 
>   Quit anyway? (y or n) y
>   error return windows-nat.c:1306 was 5
> 
> These come from SetThreadContext in windows_continue.  The second
> occurrence is because we already killed the inferior by calling
> TerminateProcess, so its threads begin to shut down, and trying to set
> their context might indeed fail.  The first warning is about one of
> those same threads, and again happens just before the thread exit is
> announced.
> 
> My solution, which you can see below, is (a) pass an additional
> parameter to windows_continue telling it that the inferior was killed,
> in which case it doesn't bother checking errors from the
> SetThreadContext call; and (b) check if the thread already exited, and
> if so, avoid calling SetThreadContext on it.  This completely avoids
> the warning when killing the inferior, and removes most (but not all)
> of the warnings for the other threads.

Why bother calling SetThreadContext at all if we just killed
the process?  That is, it seems to me this would be little
less magical:

	    if (!killed
		&& GetExitCodeThread (th->h, &ec)
		&& ec == STILL_ACTIVE)
  	     CHECK (SetThreadContext (th->h, &th->context));

> Finally, here's the full patch.  I hope this research answered all the
> questions, and we can now get the patch in.

I'm not sure it did, but in any case the patch looks good to me.

Sounds like GDBserver might have this problem too.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Fri, 28 Mar 2014 05:59:55 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: palves@redhat.com, gdb-patches@sourceware.org
> 
> > +             if (killed)
> > +               SetThreadContext (th->h, &th->context);
> > +             else
> > +               CHECK (SetThreadContext (th->h, &th->context));
> 
> Rather than duplicate the call to SetThreadContext, perhaps
> another way of doing it would be:
> 
>     DWORD status;
> 
>     status = SetThreadContext (th->h, &th->context);
>     if (!killed)
>       CHECK (status)

Fine with me.

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Fri, 28 Mar 2014 14:50:31 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org
> 
> On 03/26/2014 06:49 PM, Eli Zaretskii wrote:
> > This describes the results of my looking into this issue, given the
> > comments and suggestions by Joel and Pedro.  Sorry about the length.
> > 
> >> I didn't mean to change the behavior - only hide the warning.
> >> In this case, if it is normal that we can't suspend the thread,
> >> then there is no point in warning (scaring) the user about it.
> >> I would only generate a warning if something abnormal that we should
> >> fix occured.
> > 
> > The patch near the end of this message indeed includes code to ignore
> > the warning in these cases.
> > 
> >> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
> >> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
> >> actually read the registers off of this thread, and if so, what's the
> >> thread's backtrace like.
> > 
> > I added CHECK to that call to GetThreadContext.  It never produced a
> > warning in all my testing, and it looks like we do succeed to get the
> > registers.  At least the registers of 2 such threads show different
> > contents, and the EIP value is consistent with what "info threads"
> > displays.
> 
> It isn't clear to me whether you're saying that you saw the
> SuspendThread failure trigger in all your new testing, so that
> we'd know for sure whether GetThreadContext suceeds in that case,
> or whether it might have been that you just were "lucky" enough
> to not trigger the SuspendThread failure issue.

The former.

> Does your patch fix the test case in PR14018, without producing
> a CHECK warning from the new CHECK in GetThreadContext you've
> added?

Yes.

> > I can show you 2 typical examples.  This is from Emacs, where the
> > application has 3 threads, and one more is started by the debugger.
> > The rest, threads 5 and 6 in these examples, are those mysterious
> > threads we are talking about.
> > 
> >   (gdb) info threads
> >     Id   Target Id         Frame
> >     6    Thread 15492.0x1f28 0x77a41f46 in ntdll!ZwWaitForWorkViaWorkerFactory
> >       () from C:\Windows\system32\ntdll.dll
> >     5    Thread 15492.0x73c0 0x77a41f46 in ntdll!ZwWaitForWorkViaWorkerFactory
> >       () from C:\Windows\system32\ntdll.dll
> >     4    Thread 15492.0x2300 0x75ac78d7 in USER32!DispatchMessageW ()
> >      from C:\Windows\syswow64\user32.dll
> >     3    Thread 15492.0x1860 0x77a3fd91 in ntdll!ZwDelayExecution ()
> >      from C:\Windows\system32\ntdll.dll
> >     2    Thread 15492.0x2410 0x77a4015d in ntdll!ZwWaitForMultipleObjects ()
> >      from C:\Windows\system32\ntdll.dll
> >   * 1    Thread 15492.0x44a0 cleanup_vector (vector=0x62daeb0) at alloc.c:2917
> > 
> >   (gdb) info threads
> >     Id   Target Id         Frame
> >     6    Thread 15492.0x1f28 0x77a3f8d1 in ntdll!ZwWaitForSingleObject ()
> >      from C:\Windows\system32\ntdll.dll
> >     5    Thread 15492.0x73c0 0x77a72880 in ntdll!RtlFillMemoryUlong ()
> >      from C:\Windows\system32\ntdll.dll
> >     4    Thread 15492.0x2300 0x75ac78d7 in USER32!DispatchMessageW ()
> >      from C:\Windows\syswow64\user32.dll
> >     3    Thread 15492.0x1860 0x77a3fd91 in ntdll!ZwDelayExecution ()
> >      from C:\Windows\system32\ntdll.dll
> >     2    Thread 15492.0x2410 0x77a4015d in ntdll!ZwWaitForMultipleObjects ()
> >      from C:\Windows\system32\ntdll.dll
> >   * 1    Thread 15492.0x44a0 cleanup_vector (vector=0x388ca58) at alloc.c:2917
> >
> > The first display is what I usually see: several (I've seen up to 4)
> > threads waiting inside ZwWaitForWorkViaWorkerFactory.  But sometimes
> > they do perform some work, as can be seen from the second display.
> 
> OK, but these don't appear to be backtraces taken right after
> SuspendThread failed.

Yes, they are after SuspendThread failed.

> Why bother calling SetThreadContext at all if we just killed
> the process?

See my other mail and Joel's response.

> > Finally, here's the full patch.  I hope this research answered all the
> > questions, and we can now get the patch in.
> 
> I'm not sure it did, but in any case the patch looks good to me.

If that's an approval, I will happily commit the changes.

> Sounds like GDBserver might have this problem too.

If there's an easy way to verify that, without having 2 systems
talking via some communications line, please tell how, and I will try
that.

Thanks.

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 03/28/2014 05:35 PM, Eli Zaretskii wrote:
>> Date: Fri, 28 Mar 2014 14:50:31 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org
>>
>> On 03/26/2014 06:49 PM, Eli Zaretskii wrote:
>>> This describes the results of my looking into this issue, given the
>>> comments and suggestions by Joel and Pedro.  Sorry about the length.
>>>
>>>> I didn't mean to change the behavior - only hide the warning.
>>>> In this case, if it is normal that we can't suspend the thread,
>>>> then there is no point in warning (scaring) the user about it.
>>>> I would only generate a warning if something abnormal that we should
>>>> fix occured.
>>>
>>> The patch near the end of this message indeed includes code to ignore
>>> the warning in these cases.
>>>
>>>> I see that the GetThreadContext call (do_windows_fetch_inferior_registers)
>>>> doesn't check for errors (I think it should (*)).  It'd be interesting to know whether gdb can
>>>> actually read the registers off of this thread, and if so, what's the
>>>> thread's backtrace like.
>>>
>>> I added CHECK to that call to GetThreadContext.  It never produced a
>>> warning in all my testing, and it looks like we do succeed to get the
>>> registers.  At least the registers of 2 such threads show different
>>> contents, and the EIP value is consistent with what "info threads"
>>> displays.
>>
>> It isn't clear to me whether you're saying that you saw the
>> SuspendThread failure trigger in all your new testing, so that
>> we'd know for sure whether GetThreadContext suceeds in that case,
>> or whether it might have been that you just were "lucky" enough
>> to not trigger the SuspendThread failure issue.
> 
> The former.
>
>> Does your patch fix the test case in PR14018, without producing
>> a CHECK warning from the new CHECK in GetThreadContext you've
>> added?
> 
> Yes.

Ah, excellent!  Thank you.  Please remember adding the PR number
to the ChangeLog entry then.


>> Why bother calling SetThreadContext at all if we just killed
>> the process?
> 
> See my other mail and Joel's response.

Not sure what you mean.  TerminateProcess is asynchronous, and
we need to resume the inferior and collect the debug events
until we see the process terminate.  But, my question is
why would we write the thread's registers at all if we
just told it to die?  Seems to be we could just skip
calling SetThreadContext instead of calling it but
ignoring the result.

> 
>>> Finally, here's the full patch.  I hope this research answered all the
>>> questions, and we can now get the patch in.
>>
>> I'm not sure it did, but in any case the patch looks good to me.
> 
> If that's an approval, I will happily commit the changes.

It's an approval from my end.

>> Sounds like GDBserver might have this problem too.
> 
> If there's an easy way to verify that, without having 2 systems
> talking via some communications line, please tell how, and I will try
> that.

Sure, you can run gdbserver and gdb on the same machine, and connect
with tcp.  Just:

 $ gdbserver :9999 myprogram.exe

in one terminal, and:

 $ gdb myprogram.exe -ex "tar rem :9999" -ex "b main" -ex "c"

in another.

-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Fri, 28 Mar 2014 17:49:13 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: brobecker@adacore.com, gdb-patches@sourceware.org
> 
> >> Why bother calling SetThreadContext at all if we just killed
> >> the process?
> > 
> > See my other mail and Joel's response.
> 
> Not sure what you mean.  TerminateProcess is asynchronous, and
> we need to resume the inferior and collect the debug events
> until we see the process terminate.  But, my question is
> why would we write the thread's registers at all if we
> just told it to die?  Seems to be we could just skip
> calling SetThreadContext instead of calling it but
> ignoring the result.

If you say so, I don't know enough about this stuff.

> >> Sounds like GDBserver might have this problem too.
> > 
> > If there's an easy way to verify that, without having 2 systems
> > talking via some communications line, please tell how, and I will try
> > that.
> 
> Sure, you can run gdbserver and gdb on the same machine, and connect
> with tcp.  Just:
> 
>  $ gdbserver :9999 myprogram.exe
> 
> in one terminal, and:
> 
>  $ gdb myprogram.exe -ex "tar rem :9999" -ex "b main" -ex "c"
> 
> in another.

OK, will try that.

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Fri, 28 Mar 2014 21:30:10 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: brobecker@adacore.com, gdb-patches@sourceware.org
> 
> > Date: Fri, 28 Mar 2014 17:49:13 +0000
> > From: Pedro Alves <palves@redhat.com>
> > CC: brobecker@adacore.com, gdb-patches@sourceware.org
> > 
> > >> Why bother calling SetThreadContext at all if we just killed
> > >> the process?
> > > 
> > > See my other mail and Joel's response.
> > 
> > Not sure what you mean.  TerminateProcess is asynchronous, and
> > we need to resume the inferior and collect the debug events
> > until we see the process terminate.  But, my question is
> > why would we write the thread's registers at all if we
> > just told it to die?  Seems to be we could just skip
> > calling SetThreadContext instead of calling it but
> > ignoring the result.
> 
> If you say so, I don't know enough about this stuff.

Actually, upon second thought: we continue the inferior after
TerminateProcess call to let it be killed, right?  If so, shouldn't we
continue it with the right context?

> > >> Sounds like GDBserver might have this problem too.
> > > 
> > > If there's an easy way to verify that, without having 2 systems
> > > talking via some communications line, please tell how, and I will try
> > > that.
> > 
> > Sure, you can run gdbserver and gdb on the same machine, and connect
> > with tcp.  Just:
> > 
> >  $ gdbserver :9999 myprogram.exe
> > 
> > in one terminal, and:
> > 
> >  $ gdb myprogram.exe -ex "tar rem :9999" -ex "b main" -ex "c"
> > 
> > in another.
> 
> OK, will try that.

Funnily enough, I cannot get GDBserver to emit similar warnings in the
same situation.  I don't understand the reasons for that, since the
code is very similar, and with a single exception, we do check the
return values of calls to GetThreadContext, SetThreadContext, and
SuspendThread in GDBserver.  But the fact remains that no warnings
about these threads are ever seen when debugging remotely.  I do see
the extra threads under GDBserver as well.

Does anyone have any further ideas?

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

Ping!  If there are no further suggestions, I'd like to commit the
changes posted in this thread.

> Date: Mon, 31 Mar 2014 18:31:43 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: brobecker@adacore.com, gdb-patches@sourceware.org
> 
> > Date: Fri, 28 Mar 2014 21:30:10 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: brobecker@adacore.com, gdb-patches@sourceware.org
> > 
> > > Date: Fri, 28 Mar 2014 17:49:13 +0000
> > > From: Pedro Alves <palves@redhat.com>
> > > CC: brobecker@adacore.com, gdb-patches@sourceware.org
> > > 
> > > >> Why bother calling SetThreadContext at all if we just killed
> > > >> the process?
> > > > 
> > > > See my other mail and Joel's response.
> > > 
> > > Not sure what you mean.  TerminateProcess is asynchronous, and
> > > we need to resume the inferior and collect the debug events
> > > until we see the process terminate.  But, my question is
> > > why would we write the thread's registers at all if we
> > > just told it to die?  Seems to be we could just skip
> > > calling SetThreadContext instead of calling it but
> > > ignoring the result.
> > 
> > If you say so, I don't know enough about this stuff.
> 
> Actually, upon second thought: we continue the inferior after
> TerminateProcess call to let it be killed, right?  If so, shouldn't we
> continue it with the right context?
> 
> > > >> Sounds like GDBserver might have this problem too.
> > > > 
> > > > If there's an easy way to verify that, without having 2 systems
> > > > talking via some communications line, please tell how, and I will try
> > > > that.
> > > 
> > > Sure, you can run gdbserver and gdb on the same machine, and connect
> > > with tcp.  Just:
> > > 
> > >  $ gdbserver :9999 myprogram.exe
> > > 
> > > in one terminal, and:
> > > 
> > >  $ gdb myprogram.exe -ex "tar rem :9999" -ex "b main" -ex "c"
> > > 
> > > in another.
> > 
> > OK, will try that.
> 
> Funnily enough, I cannot get GDBserver to emit similar warnings in the
> same situation.  I don't understand the reasons for that, since the
> code is very similar, and with a single exception, we do check the
> return values of calls to GetThreadContext, SetThreadContext, and
> SuspendThread in GDBserver.  But the fact remains that no warnings
> about these threads are ever seen when debugging remotely.  I do see
> the extra threads under GDBserver as well.
> 
> Does anyone have any further ideas?

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

> Ping!  If there are no further suggestions, I'd like to commit the
> changes posted in this thread.

FWIW, I think you can go ahead. Your patches seem to be an improvement,
and I don't see how they could make things worse should we want to
work on another way to implement this part of the code again.

> 
> > Date: Mon, 31 Mar 2014 18:31:43 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: brobecker@adacore.com, gdb-patches@sourceware.org
> > 
> > > Date: Fri, 28 Mar 2014 21:30:10 +0300
> > > From: Eli Zaretskii <eliz@gnu.org>
> > > Cc: brobecker@adacore.com, gdb-patches@sourceware.org
> > > 
> > > > Date: Fri, 28 Mar 2014 17:49:13 +0000
> > > > From: Pedro Alves <palves@redhat.com>
> > > > CC: brobecker@adacore.com, gdb-patches@sourceware.org
> > > > 
> > > > >> Why bother calling SetThreadContext at all if we just killed
> > > > >> the process?
> > > > > 
> > > > > See my other mail and Joel's response.
> > > > 
> > > > Not sure what you mean.  TerminateProcess is asynchronous, and
> > > > we need to resume the inferior and collect the debug events
> > > > until we see the process terminate.  But, my question is
> > > > why would we write the thread's registers at all if we
> > > > just told it to die?  Seems to be we could just skip
> > > > calling SetThreadContext instead of calling it but
> > > > ignoring the result.
> > > 
> > > If you say so, I don't know enough about this stuff.
> > 
> > Actually, upon second thought: we continue the inferior after
> > TerminateProcess call to let it be killed, right?  If so, shouldn't we
> > continue it with the right context?

Sure, but what context would that be?

> > 
> > > > >> Sounds like GDBserver might have this problem too.
> > > > > 
> > > > > If there's an easy way to verify that, without having 2 systems
> > > > > talking via some communications line, please tell how, and I will try
> > > > > that.
> > > > 
> > > > Sure, you can run gdbserver and gdb on the same machine, and connect
> > > > with tcp.  Just:
> > > > 
> > > >  $ gdbserver :9999 myprogram.exe
> > > > 
> > > > in one terminal, and:
> > > > 
> > > >  $ gdb myprogram.exe -ex "tar rem :9999" -ex "b main" -ex "c"
> > > > 
> > > > in another.
> > > 
> > > OK, will try that.
> > 
> > Funnily enough, I cannot get GDBserver to emit similar warnings in the
> > same situation.  I don't understand the reasons for that, since the
> > code is very similar, and with a single exception, we do check the
> > return values of calls to GetThreadContext, SetThreadContext, and
> > SuspendThread in GDBserver.  But the fact remains that no warnings
> > about these threads are ever seen when debugging remotely.  I do see
> > the extra threads under GDBserver as well.
> > 
> > Does anyone have any further ideas?

-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 03/31/2014 04:31 PM, Eli Zaretskii wrote:
>> Date: Fri, 28 Mar 2014 21:30:10 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: brobecker@adacore.com, gdb-patches@sourceware.org
>>
>>> Date: Fri, 28 Mar 2014 17:49:13 +0000
>>> From: Pedro Alves <palves@redhat.com>
>>> CC: brobecker@adacore.com, gdb-patches@sourceware.org
>>>
>>>>> Why bother calling SetThreadContext at all if we just killed
>>>>> the process?
>>>>
>>>> See my other mail and Joel's response.
>>>
>>> Not sure what you mean.  TerminateProcess is asynchronous, and
>>> we need to resume the inferior and collect the debug events
>>> until we see the process terminate.  But, my question is
>>> why would we write the thread's registers at all if we
>>> just told it to die?  Seems to be we could just skip
>>> calling SetThreadContext instead of calling it but
>>> ignoring the result.
>>
>> If you say so, I don't know enough about this stuff.
> 
> Actually, upon second thought: we continue the inferior after
> TerminateProcess call to let it be killed, right?  If so, shouldn't we
> continue it with the right context?

I don't think the threads are going to run whatever context
you set them them to.  They'll surely die before ever getting
scheduled to run any further userspace code?

> 
>>>>> Sounds like GDBserver might have this problem too.
>>>>
>>>> If there's an easy way to verify that, without having 2 systems
>>>> talking via some communications line, please tell how, and I will try
>>>> that.
>>>
>>> Sure, you can run gdbserver and gdb on the same machine, and connect
>>> with tcp.  Just:
>>>
>>>  $ gdbserver :9999 myprogram.exe
>>>
>>> in one terminal, and:
>>>
>>>  $ gdb myprogram.exe -ex "tar rem :9999" -ex "b main" -ex "c"
>>>
>>> in another.
>>
>> OK, will try that.
> 
> Funnily enough, I cannot get GDBserver to emit similar warnings in the
> same situation.  I don't understand the reasons for that, since the
> code is very similar, and with a single exception, we do check the
> return values of calls to GetThreadContext, SetThreadContext, and
> SuspendThread in GDBserver.  But the fact remains that no warnings
> about these threads are ever seen when debugging remotely.  I do see
> the extra threads under GDBserver as well.

GDBserver's warnings are guarded by 'if (debug_threads)' (see OUTMSG2).
That means you'll need to start gdbserver with --debug to see them.
Did you do that?

-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Mon, 07 Apr 2014 18:09:16 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: brobecker@adacore.com, gdb-patches@sourceware.org
> 
> > Funnily enough, I cannot get GDBserver to emit similar warnings in the
> > same situation.  I don't understand the reasons for that, since the
> > code is very similar, and with a single exception, we do check the
> > return values of calls to GetThreadContext, SetThreadContext, and
> > SuspendThread in GDBserver.  But the fact remains that no warnings
> > about these threads are ever seen when debugging remotely.  I do see
> > the extra threads under GDBserver as well.
> 
> GDBserver's warnings are guarded by 'if (debug_threads)' (see OUTMSG2).

But the warnings I was talking about are output with OUTMSG, which
doesn't depend on debug_threads.  Here's an example:

  static void
  suspend_one_thread (struct inferior_list_entry *entry)
  {
    struct thread_info *thread = (struct thread_info *) entry;
    win32_thread_info *th = inferior_target_data (thread);

    if (!th->suspended)
      {
	if (SuspendThread (th->h) == (DWORD) -1)
	  {
	    DWORD err = GetLastError ();
	    OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
		     "(error %d): %s\n", (int) err, strwinerror (err)));

Did I miss something?

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

> But the warnings I was talking about are output with OUTMSG, which
> doesn't depend on debug_threads.  Here's an example:
> 
>   static void
>   suspend_one_thread (struct inferior_list_entry *entry)
>   {
>     struct thread_info *thread = (struct thread_info *) entry;
>     win32_thread_info *th = inferior_target_data (thread);
> 
>     if (!th->suspended)
>       {
> 	if (SuspendThread (th->h) == (DWORD) -1)
> 	  {
> 	    DWORD err = GetLastError ();
> 	    OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
> 		     "(error %d): %s\n", (int) err, strwinerror (err)));
> 
> Did I miss something?

This code not going to be used at all if you debug through GDBserver
(it's the code in remote.c that does). But if the GDBserver code does
the same thing as windows-nat in terms of inferior management,
it might trigger the same error and output them on GDBserver's
stdout/stderr if --debug is enabled. Usually, we try to keep the
GDB and GDBserver code in sync, and that means that we fix a problem
in inferior management, it's a good bet that the same problem exists
on both sides.

I might be able to take a look sometime next week. I think the issue
we'll have is with testing, since the problem is not necessarily always
reproduceable in Eli's scenario, and he would need to debug repeatedly
with GDBserver in order to gain a good level of confidence that the
problem is gone for good.

-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Mon, 7 Apr 2014 14:39:13 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
> 
> > But the warnings I was talking about are output with OUTMSG, which
> > doesn't depend on debug_threads.  Here's an example:
> > 
> >   static void
> >   suspend_one_thread (struct inferior_list_entry *entry)
> >   {
> >     struct thread_info *thread = (struct thread_info *) entry;
> >     win32_thread_info *th = inferior_target_data (thread);
> > 
> >     if (!th->suspended)
> >       {
> > 	if (SuspendThread (th->h) == (DWORD) -1)
> > 	  {
> > 	    DWORD err = GetLastError ();
> > 	    OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
> > 		     "(error %d): %s\n", (int) err, strwinerror (err)));
> > 
> > Did I miss something?
> 
> This code not going to be used at all if you debug through GDBserver
> (it's the code in remote.c that does).

Sorry, I don't understand: remote.c is not specific to Windows, so it
cannot include any Windows-specific calls like SuspendThread.

> But if the GDBserver code does the same thing as windows-nat in
> terms of inferior management, it might trigger the same error and
> output them on GDBserver's stdout/stderr if --debug is enabled.

Again, I don't see where --debug comes into play here.  Can you tell
me what am I missing?

> Usually, we try to keep the GDB and GDBserver code in sync, and that
> means that we fix a problem in inferior management, it's a good bet
> that the same problem exists on both sides.

As I said, I don't see the same problem in the server.

> I might be able to take a look sometime next week. I think the issue
> we'll have is with testing, since the problem is not necessarily always
> reproduceable in Eli's scenario, and he would need to debug repeatedly
> with GDBserver in order to gain a good level of confidence that the
> problem is gone for good.

I can reproduce it with some effort, so verifying a fix is not a
problem.

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

> Sorry, I don't understand: remote.c is not specific to Windows, so it
> cannot include any Windows-specific calls like SuspendThread.

I think the easiest is probably for you to take a look at the code
in gdb/gdbserver/win32-low.c. This file actually also has a routine
called thread_rec, and generally speaking, the code in gdb/*-nat.c
and the corresponding gdb/gdbserver/*-low.c can (and probably should)
be very similar - at least until we can factorize that code and reuse
the gdbserver code in GDB.

The reason I was mentioning remote.c is because, when you use GDBserver,
GDBserver does all the inferior control for GDB. It's like a mini
debugger, except that it does not have a user interface, only a serial
interface that GDB can used to communicate with it and send it orders
such as read or write memory, next/step operations, continue, kill,
etc. So, even if you are using a Windows native debugger, as soon as
you type the "target remote [...]" command, the windows-nat target
gets swapped out in favor of the "remote" target, which then delegates
the inferior control to the agent (gdbserver). The code that does that
delegation is in remote.c.

So, to try to reproduce with GDBserver, you'd have to do the following.
In one terminal, start your program with gdbserver. For instance:

    % gdbserver --debug :4567 YOUR_PROGRAM

It'll write a message saying that the program has been started,
and that it's now waiting for a connection on port 4567. The --debug
switch is not normally necessary, but allows us to turn warnings on,
which then allows us to determine whether or not we reproduced the
problem in GDB.

Next, start GDB in a second terminal, and connect to it via:

    % gdb YOUR_PROGRAM
    (gdb) target remote :4567

The "target remote [...]" command replaces the "run" command.
From there, everything else should be the same as the reproducer
you have for the case where GDBserver isn't involved.

And instead of seeing the warning in the GDB console, you would
see it in the terminal where gdbserver was started. The idea for
solving the issue on the gdbserver side would be to apply the very
same sort of change you applied to windows-nat, but to win32-low,
essentially keeping the implementations in sync.

-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 04/07/2014 07:24 PM, Eli Zaretskii wrote:
>> Date: Mon, 07 Apr 2014 18:09:16 +0100
>> From: Pedro Alves <palves@redhat.com>
>> CC: brobecker@adacore.com, gdb-patches@sourceware.org
>>
>>> Funnily enough, I cannot get GDBserver to emit similar warnings in the
>>> same situation.  I don't understand the reasons for that, since the
>>> code is very similar, and with a single exception, we do check the
>>> return values of calls to GetThreadContext, SetThreadContext, and
>>> SuspendThread in GDBserver.  But the fact remains that no warnings
>>> about these threads are ever seen when debugging remotely.  I do see
>>> the extra threads under GDBserver as well.
>>
>> GDBserver's warnings are guarded by 'if (debug_threads)' (see OUTMSG2).
> 
> But the warnings I was talking about are output with OUTMSG, which
> doesn't depend on debug_threads.  Here's an example:
> 
>   static void
>   suspend_one_thread (struct inferior_list_entry *entry)
>   {
>     struct thread_info *thread = (struct thread_info *) entry;
>     win32_thread_info *th = inferior_target_data (thread);
> 
>     if (!th->suspended)
>       {
> 	if (SuspendThread (th->h) == (DWORD) -1)
> 	  {
> 	    DWORD err = GetLastError ();
> 	    OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
> 		     "(error %d): %s\n", (int) err, strwinerror (err)));
> 
> Did I miss something?

No, it was I who missed that.  I found a Windows machine and am taking
a look.

-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Mon, 7 Apr 2014 21:23:31 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: palves@redhat.com, gdb-patches@sourceware.org
> 
> > Sorry, I don't understand: remote.c is not specific to Windows, so it
> > cannot include any Windows-specific calls like SuspendThread.
> 
> I think the easiest is probably for you to take a look at the code
> in gdb/gdbserver/win32-low.c.

I altready did, but you seemed to say that code will not be used,
which confused me.

> This file actually also has a routine
> called thread_rec, and generally speaking, the code in gdb/*-nat.c
> and the corresponding gdb/gdbserver/*-low.c can (and probably should)
> be very similar - at least until we can factorize that code and reuse
> the gdbserver code in GDB.

Yes, I know.  I've read the code.

> The reason I was mentioning remote.c is because, when you use GDBserver,
> GDBserver does all the inferior control for GDB. It's like a mini
> debugger, except that it does not have a user interface, only a serial
> interface that GDB can used to communicate with it and send it orders
> such as read or write memory, next/step operations, continue, kill,
> etc. So, even if you are using a Windows native debugger, as soon as
> you type the "target remote [...]" command, the windows-nat target
> gets swapped out in favor of the "remote" target, which then delegates
> the inferior control to the agent (gdbserver). The code that does that
> delegation is in remote.c.

I know that, too.  What I don't understand is how all this is relevant
to the issue at hand, which is why the same system calls in
win32-low.c as we have in windows-nat.c don't trigger similar warning
messages.

> So, to try to reproduce with GDBserver, you'd have to do the following.
> In one terminal, start your program with gdbserver. For instance:
> 
>     % gdbserver --debug :4567 YOUR_PROGRAM

Why do I need --debug?  The warnings in question are displayed by
OUTMSG, which AFAIU does not need --debug to print its messages.

> It'll write a message saying that the program has been started,
> and that it's now waiting for a connection on port 4567. The --debug
> switch is not normally necessary, but allows us to turn warnings on,
> which then allows us to determine whether or not we reproduced the
> problem in GDB.
> 
> Next, start GDB in a second terminal, and connect to it via:
> 
>     % gdb YOUR_PROGRAM
>     (gdb) target remote :4567
> 
> The "target remote [...]" command replaces the "run" command.
> >From there, everything else should be the same as the reproducer
> you have for the case where GDBserver isn't involved.

I already did all that, per Pedro's instructions here:

  https://sourceware.org/ml/gdb-patches/2014-03/msg00668.html

> And instead of seeing the warning in the GDB console, you would
> see it in the terminal where gdbserver was started.

But that's it: I see no warnings when I run GDBserver like that.

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 21900 bytes --]

On 04/08/2014 12:32 PM, Pedro Alves wrote:
> On 04/07/2014 07:24 PM, Eli Zaretskii wrote:
>>> Date: Mon, 07 Apr 2014 18:09:16 +0100
>>> From: Pedro Alves <palves@redhat.com>
>>> CC: brobecker@adacore.com, gdb-patches@sourceware.org
>>>
>>>> Funnily enough, I cannot get GDBserver to emit similar warnings in the
>>>> same situation.  I don't understand the reasons for that, since the
>>>> code is very similar, and with a single exception, we do check the
>>>> return values of calls to GetThreadContext, SetThreadContext, and
>>>> SuspendThread in GDBserver.  But the fact remains that no warnings
>>>> about these threads are ever seen when debugging remotely.  I do see
>>>> the extra threads under GDBserver as well.
>>>
>>> GDBserver's warnings are guarded by 'if (debug_threads)' (see OUTMSG2).
>>
>> But the warnings I was talking about are output with OUTMSG, which
>> doesn't depend on debug_threads.  Here's an example:
>>
>>   static void
>>   suspend_one_thread (struct inferior_list_entry *entry)
>>   {
>>     struct thread_info *thread = (struct thread_info *) entry;
>>     win32_thread_info *th = inferior_target_data (thread);
>>
>>     if (!th->suspended)
>>       {
>> 	if (SuspendThread (th->h) == (DWORD) -1)
>> 	  {
>> 	    DWORD err = GetLastError ();
>> 	    OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
>> 		     "(error %d): %s\n", (int) err, strwinerror (err)));
>>
>> Did I miss something?
> 
> No, it was I who missed that.  I found a Windows machine and am taking
> a look.

D**n, I had forgotten how slow Windows is...


OK, so I looked for a Windows machine to try it.  I can definitely
reproduce the warning with gdbserver, using the testcase from PR14018.
Like:

$ cat test.c
#include <windows.h>

int main()
{    
    while (1)
        DebugBreakProcess(GetCurrentProcess());
}

$ gcc win32_suspend_issue.c -o win32_suspend_issue.exe -g3 -O0
$
$ ./gdb -data-directory=data-directory --args ./win32_suspend_issue.exe
[snip]
Reading symbols from ./win32_suspend_issue.exe...done.
(gdb) tbreak main
Temporary breakpoint 1 at 0x4010ba: file win32_suspend_issue.c, line 6.
(gdb) tar remote :9999
Remote debugging using :9999
0x770c1004 in ntdll!RtlUpcaseUnicodeToOemN () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
(gdb) c
Continuing.

Temporary breakpoint 1, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());
(gdb) b 6
Breakpoint 2 at 0x4010ba: file win32_suspend_issue.c, line 6.
(gdb) commands 
Type commands for breakpoint(s) 2, one per line.
End with a line saying just "end".
>continue
>end
(gdb) handle SIGTRAP nostop
SIGTRAP is used by the debugger.
Are you sure you want to change it? (y or n) y
Signal        Stop      Print   Pass to program Description
SIGTRAP       No        Yes     No              Trace/breakpoint trap
(gdb) set pagination off 
(gdb) c

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());
[New Thread 3156]

Program received signal SIGTRAP, Trace/breakpoint trap.

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());

Breakpoint 2, main () at win32_suspend_issue.c:6
6               DebugBreakProcess(GetCurrentProcess());
[New Thread 3524]

Program received signal SIGTRAP, Trace/breakpoint trap.

[snip lots of the same, forever]


While on the gdbserver terminal:

$ gdbserver/gdbserver.exe :9999 ./win32_suspend_issue.exe
Process ./win32_suspend_issue.exe created; pid = 5888
Listening on port 9999
Remote debugging from host 127.0.0.1
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
warning: SuspendThread failed in thread_rec, (error 5): Acesso negado.
...

Now, unlike in the GDB case,  gdbserver reads registers
from the thread anyway, so we don't see the "PC register is not
available" error.  So I went a step further, and patched gdbserver
like this, to force it to error out on the subsequent resume,
if previously suspending the thread failed:

---
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index bc2506c..ea3d70b 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -177,8 +177,11 @@ thread_rec (ptid_t ptid, int get_context)
 	  if (SuspendThread (th->h) == (DWORD) -1)
 	    {
 	      DWORD err = GetLastError ();
-	      OUTMSG (("warning: SuspendThread failed in thread_rec, "
-		       "(error %d): %s\n", (int) err, strwinerror (err)));
+	      OUTMSG (("warning: SuspendThread failed in thread_rec(%s), "
+		       "(error %d): %s\n",
+		       target_pid_to_str (ptid),
+		       (int) err, strwinerror (err)));
+	      th->suspended = -1;
 	    }
 	  else
 	    th->suspended = 1;
@@ -420,8 +423,15 @@ continue_one_thread (struct inferior_list_entry *this_thread, void *id_ptr)
   int thread_id = * (int *) id_ptr;
   win32_thread_info *th = inferior_target_data (thread);
 
-  if ((thread_id == -1 || thread_id == th->tid)
-      && th->suspended)
+  if ((thread_id == -1 || thread_id == th->tid))
+    {
+      if (th->suspended == -1)
+	{
+	  th->suspended = 1;
+	  error ("SuspendThread had failed for thread %d.\n", (int) th->tid);
+	}
+
+      if (th->suspended)
     {
       if (th->context.ContextFlags)
 	{
@@ -437,6 +447,7 @@ continue_one_thread (struct inferior_list_entry *this_thread, void *id_ptr)
 	}
       th->suspended = 0;
     }
+    }
 
   return 0;
 }


That allows us letting the test case run free, and when
SuspendThread fails, "continue" fails, and we can then
get the backtrace of the failing thread.

Re-running the test under the patched gdbserver, I
now get:

$ ./gdbserver.exe :9999 ../win32_suspend_issue.exe
Process ../win32_suspend_issue.exe created; pid = 5564
Listening on port 9999
Remote debugging from host 127.0.0.1
warning: SuspendThread failed in thread_rec(LWP 5564.2428), (error 5): Acesso negado.
SuspendThread had failed for thread 2428.

And on the GDB side, we see:

(gdb) info threads 
[New Thread 4216]
[New Thread 1096]
[New Thread 5412]
[New Thread 3248]
  Id   Target Id         Frame 
  10   Thread 3248       0x770301b4 in ntdll!KiIntSystemCall () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
  9    Thread 5412       0x770301b4 in ntdll!KiIntSystemCall () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
  8    Thread 1096       0x770301b4 in ntdll!KiIntSystemCall () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
  7    Thread 4216       0x7703f905 in ntdll!RtlUpcaseUnicodeChar () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
  6    Thread 2428       0x77040096 in ntdll!RtlDosApplyFileIsolationRedirection_Ustr () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
* 1    Thread 4756       0x004010bf in main () at /home/pedro/host-pedro/win32_suspend_issue.c:6
(gdb) t 6
[Switching to thread 6 (Thread 2428)]
#0  0x77040096 in ntdll!RtlDosApplyFileIsolationRedirection_Ustr () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
(gdb) bt
#0  0x77040096 in ntdll!RtlDosApplyFileIsolationRedirection_Ustr () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
#1  0x77040096 in ntdll!RtlDosApplyFileIsolationRedirection_Ustr () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
#2  0x77076795 in ntdll!RtlDeleteAtomFromAtomTable () from /cygdrive/c/Windows/SysWOW64/ntdll.dll
#3  0x00000000 in ?? ()


Now, that backtrace is unfortunately doesn't look complete,
as I'm using a 32-bit gdb/gdbserver here, but I'm going to guess
RtlDeleteAtomFromAtomTable is being called during thread
termination.  Searching the web for that function at least finds
this backtrace, at
<http://www.windowsbbs.com/windows-xp/59014-help-stop-errors.html>:

STACK_TEXT:  
b857275c 804fe507 0000008e c0000005 bf802ade nt!KeBugCheckEx+0x1b
b8572b24 80541075 b8572b40 00000000 b8572b94 nt!KiDispatchException+0x3b1
b8572b8c 80541026 b8572c18 bf802ade badb0d00 nt!CommonDispatchException+0x4d
b8572ba8 805dbbc3 bc63da10 bc630000 0000057b nt!Kei386EoiHelper+0x18a
b8572c18 bf8c0bbb e47b808c 86475f18 e4867568 nt!RtlDeleteAtomFromAtomTable+0xcf
b8572c2c bf8209f6 e47b8008 85466da8 00000000 win32k!DestroyProcessesClasses+0x18
b8572c54 bf80f0b6 00000001 b8572c7c bf80f17a win32k!xxxDestroyThreadInfo+0x21d
b8572c60 bf80f17a 85466da8 00000001 00000000 win32k!UserThreadCallout+0x4b
b8572c7c 805d0c80 85466da8 00000001 85466da8 win32k!W32pThreadCallout+0x3d
b8572d08 805d1098 00000000 85466da8 00000000 nt!PspExitThread+0x3cc
b8572d28 805d1273 85466da8 00000000 b8572d64 nt!PspTerminateThreadByPointer+0x52
b8572d54 8054060c 00000000 00000000 0006fee4 nt!NtTerminateProcess+0x105
b8572d54 7c90eb94 00000000 00000000 0006fee4 nt!KiFastCallEntry+0xfc
0006fde4 7c90e89a 7c81cd96 ffffffff 00000000 ntdll!KiFastSystemCallRet
0006fde8 7c81cd96 ffffffff 00000000 00000000 ntdll!ZwTerminateProcess+0xc
0006fee4 7c81cdee 00000000 77e8f3b0 ffffffff kernel32!_ExitProcess+0x62
0006fef8 77c39d45 00000000 0006ff14 77c39e78 kernel32!ExitProcess+0x14
0006ff04 77c39e78 00000000 00000000 0006ff28 msvcrt!__crtExitProcess+0x32
0006ff14 77c39e90 00000000 00000000 00000002 msvcrt!_cinit+0xee
0006ff28 01019cf6 00000000 00fdcd5c 7c90e1fe msvcrt!exit+0x12
0006ffc0 7c816fd7 00fdcd5c 7c90e1fe 7ffdf000 wordpad!wWinMainCRTStartup+0x1a9
0006fff0 00000000 01019b4d 00000000 78746341 kernel32!BaseProcessStart+0x23

(shows RtlDeleteAtomFromAtomTable being called from within NtTerminateProcess.)


My next step was then to apply a similar patch on the GDB
side.  Like so:

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index fe40c4d..6a0c861 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -312,9 +312,10 @@ thread_rec (DWORD id, int get_context)
 		    warning (_("SuspendThread (tid=0x%x) failed."
 			       " (winerr %u)"),
 			     (unsigned) id, (unsigned) err);
-		    return NULL;
+		    th->suspended = -2;
 		  }
-		th->suspended = 1;
+		else
+		  th->suspended = 1;
 	      }
 	    else if (get_context < 0)
 	      th->suspended = -1;
@@ -1200,6 +1201,12 @@ windows_continue (DWORD continue_status, int id)
     if ((id == -1 || id == (int) th->id)
 	&& th->suspended)
       {
+	if (th->suspended == -2)
+	  {
+	    th->suspended = 1;
+	    error ("SuspendThread had failed.\n");
+	  }
+
 	if (debug_registers_changed)
 	  {
 	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;



That is, again, if SuspendThread fails, GDB still fetches registers,
like in your patch, but in addition, prevent resuming (once)
so we can get the failing thread's backtrace.

Still using the test case from the PR, I see, with the native
gdb:

Program received signal SIGTRAP, Trace/breakpoint trap.
warning: SuspendThread (tid=0x1318) failed. (winerr 5)

Breakpoint 4, main () at /home/pedro/win32_suspend_issue.c:6
6       in /home/pedro/win32_suspend_issue.c
0x0000000000401514 in main () at /home/pedro/win32_suspend_issue.c:6
6       in /home/pedro/win32_suspend_issue.c
SuspendThread had failed.


And tid=0x1318 is inside ZwTerminateThread:

(gdb) info threads
  Id   Target Id         Frame 
  123  Thread 5316.0x1318 0x0000000076e90aea in ntdll!ZwTerminateThread () from C:\Windows\SYSTEM32\ntdll.dll
* 1    Thread 5316.0x1004 0x0000000000401514 in main () at /home/pedro/win32_suspend_issue.c:6
(gdb) t 123
[Switching to thread 123 (Thread 5316.0x1318)]
#0  0x0000000076e90aea in ntdll!ZwTerminateThread () from C:\Windows\SYSTEM32\ntdll.dll
(gdb) bt
#0  0x0000000076e90aea in ntdll!ZwTerminateThread () from C:\Windows\SYSTEM32\ntdll.dll
#1  0x0000000076e85c78 in ntdll!RtlExitUserThread () from C:\Windows\SYSTEM32\ntdll.dll
#2  0x0000000076f37a61 in ntdll!DbgUiRemoteBreakin () from C:\Windows\SYSTEM32\ntdll.dll
#3  0x0000000076d3651d in KERNEL32!BaseThreadInitThunk () from C:\Windows\system32\kernel32.dll
#4  0x0000000076e6ba01 in ntdll!RtlUserThreadStart () from C:\Windows\SYSTEM32\ntdll.dll
#5  0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)


With the native debugger, the fail always happens in 
ZwTerminateThread (and I tried both 64-bit and 32-bit native
gdbs), while with gdbserver the fail always happens in
RtlDosApplyFileIsolationRedirection.  My currently hypothesis for this is
that gdbserver affects timings differently then when running under
gdb (remote protocol traffic, etc.).

(Oh this is a Windows 7 box, btw)

Then I tried simplifying the test case, and remove
remote thread injection from the picture (DebugBreakProcess
uses remote thread injection, and I don't know whether
that's special in terms of SuspendThread permissions, or whether
that's somehow short circuited in terms of breaking
GetCurrentProcess()), with this alternative test case:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#include <windows.h>

static DWORD WINAPI
thread_entry (void *arg)
{
  return 0;
}

int
main ()
{
  DWORD threadId;

	
  while (1)
    {
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
    }
}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

And sourcing this GDB script:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
start
tb main
continue

break 17
command
	continue
end

set pagination off

continue
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

And sure enough, eventually, I get:

(...)
[Thread 11432.0x1208 exited with code 0]
[Thread 11432.0x813c exited with code 0]
[New Thread 11432.0x8290]
[Thread 11432.0x8290 exited with code 0]
[New Thread 11432.0x78d4]
[Thread 11432.0x78d4 exited with code 0]
[New Thread 11432.0x6ed8]
[Thread 11432.0x6ed8 exited with code 0]

Breakpoint 1, main () at win32_spawn_threads.c:17
17      in win32_spawn_threads.c
[New Thread 11432.0x8334]
warning: SuspendThread (tid=0x8334) failed. (winerr 5)
0x0000000000401520 in main () at win32_spawn_threads.c:17
17      in win32_spawn_threads.c
./win32_spawn_threads.gdb:14: Error in sourced command file:
SuspendThread had failed.

(gdb) info threads
  Id   Target Id         Frame 
  6870 Thread 11432.0x8334 0x0000000076e90aea in ntdll!ZwTerminateThread () from C:\Windows\SYSTEM32\ntdll.dll
  6863 Thread 11432.0x7030 0x0000000076e905fa in ntdll!ZwWaitForSingleObject () from C:\Windows\SYSTEM32\ntdll.dll
  6862 Thread 11432.0x8294 0x0000000076e905fa in ntdll!ZwWaitForSingleObject () from C:\Windows\SYSTEM32\ntdll.dll
...
(gdb) t 6870
[Switching to thread 6870 (Thread 11432.0x8334)]
#0  0x0000000076e90aea in ntdll!ZwTerminateThread () from C:\Windows\SYSTEM32\ntdll.dll
(gdb) bt
#0  0x0000000076e90aea in ntdll!ZwTerminateThread () from C:\Windows\SYSTEM32\ntdll.dll
#1  0x0000000076e85c78 in ntdll!RtlExitUserThread () from C:\Windows\SYSTEM32\ntdll.dll
#2  0x0000000076d36525 in KERNEL32!BaseThreadInitThunk () from C:\Windows\system32\kernel32.dll
#3  0x0000000076e6ba01 in ntdll!RtlUserThreadStart () from C:\Windows\SYSTEM32\ntdll.dll
#4  0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb)


and in 32-bit mode, the equivalent:

[New Thread 13192.0x8248]
[New Thread 13192.0x82d0]
[New Thread 13192.0x718c]
[New Thread 13192.0x7214]
[New Thread 13192.0x8238]
warning: SuspendThread (tid=0x8238) failed. (winerr 5)

Breakpoint 3, main () at win32_spawn_threads.c:17
17      in win32_spawn_threads.c
0x00401585 in main () at win32_spawn_threads.c:17
17      in win32_spawn_threads.c
./win32_spawn_threads.gdb:14: Error in sourced command file:
SuspendThread had failed.

(gdb) info threads
  Id   Target Id         Frame 
  635  Thread 13192.0x8238 0x77040096 in ntdll!ZwTerminateThread () from C:\Windows\SysWOW64\ntdll.dll
  634  Thread 13192.0x7214 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  633  Thread 13192.0x718c 0x770301b4 in ntdll!RtlUserThreadStart () from C:\Windows\SysWOW64\ntdll.dll
  632  Thread 13192.0x82d0 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  631  Thread 13192.0x8248 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  630  Thread 13192.0x8258 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  629  Thread 13192.0x82a0 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  628  Thread 13192.0x6544 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  627  Thread 13192.0x8280 0x770301b4 in ntdll!RtlUserThreadStart () from C:\Windows\SysWOW64\ntdll.dll
  616  Thread 13192.0x71a4 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  590  Thread 13192.0x8298 0x770301b4 in ntdll!RtlUserThreadStart () from C:\Windows\SysWOW64\ntdll.dll
  578  Thread 13192.0x2ba0 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  574  Thread 13192.0x2560 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  558  Thread 13192.0x6a94 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  557  Thread 13192.0x15dc 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
  544  Thread 13192.0x2d90 0x7703f9d9 in ntdll!ZwSetEvent () from C:\Windows\SysWOW64\ntdll.dll
  519  Thread 13192.0x118c 0x7703f8d1 in ntdll!ZwWaitForSingleObject () from C:\Windows\SysWOW64\ntdll.dll
* 1    Thread 13192.0x3390 0x00401585 in main () at win32_spawn_threads.c:17
(gdb) t 635
[Switching to thread 635 (Thread 13192.0x8238)]
#0  0x77040096 in ntdll!ZwTerminateThread () from C:\Windows\SysWOW64\ntdll.dll
(gdb) bt
#0  0x77040096 in ntdll!ZwTerminateThread () from C:\Windows\SysWOW64\ntdll.dll
#1  0x77040096 in ntdll!ZwTerminateThread () from C:\Windows\SysWOW64\ntdll.dll
#2  0x77076795 in ntdll!RtlExitUserThread () from C:\Windows\SysWOW64\ntdll.dll
#3  0x76903371 in KERNEL32!BaseThreadInitThunk () from C:\Windows\syswow64\kernel32.dll
#4  0x7705bf32 in ntdll!RtlInitializeExceptionChain () from C:\Windows\SysWOW64\ntdll.dll
#5  0x7705bf05 in ntdll!RtlInitializeExceptionChain () from C:\Windows\SysWOW64\ntdll.dll
#6  0x00000000 in ?? ()
(gdb) 


So at least this test case shows that the issue very much looks
like indeed caused by trying to suspend threads that are more dead
than alive.  I'd be very curious to see the backtrace you get
for the failing thread in your test case (I guess emacs?).

+		    /* We get Access Denied (5) when trying to suspend
+		       threads that Windows started on behalf of the
+		       debuggee, usually when those threads are just
+		       about to exit.  */
+		    if (err != ERROR_ACCESS_DENIED)

I've shown above that whether it was Windows or the program
itself that started the threads is irrelevant, it'd be good to 
reword this comment.

-- 
Pedro Alves


[-- Attachment #2: 0001-force-resume-fail-on-suspend-fail.patch --]
[-- Type: text/x-patch, Size: 2606 bytes --]

From d935c6d43e035c15ba64c1e8094943ac63d8e711 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 8 Apr 2014 11:37:26 +0100
Subject: [PATCH] Force the next resume to fail when SuspendThread fails

---
 gdb/gdbserver/win32-low.c | 19 +++++++++++++++----
 gdb/windows-nat.c         | 11 +++++++++--
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index bc2506c..ea3d70b 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -177,8 +177,11 @@ thread_rec (ptid_t ptid, int get_context)
 	  if (SuspendThread (th->h) == (DWORD) -1)
 	    {
 	      DWORD err = GetLastError ();
-	      OUTMSG (("warning: SuspendThread failed in thread_rec, "
-		       "(error %d): %s\n", (int) err, strwinerror (err)));
+	      OUTMSG (("warning: SuspendThread failed in thread_rec(%s), "
+		       "(error %d): %s\n",
+		       target_pid_to_str (ptid),
+		       (int) err, strwinerror (err)));
+	      th->suspended = -1;
 	    }
 	  else
 	    th->suspended = 1;
@@ -420,8 +423,15 @@ continue_one_thread (struct inferior_list_entry *this_thread, void *id_ptr)
   int thread_id = * (int *) id_ptr;
   win32_thread_info *th = inferior_target_data (thread);
 
-  if ((thread_id == -1 || thread_id == th->tid)
-      && th->suspended)
+  if ((thread_id == -1 || thread_id == th->tid))
+    {
+      if (th->suspended == -1)
+	{
+	  th->suspended = 1;
+	  error ("SuspendThread had failed for thread %d.\n", (int) th->tid);
+	}
+
+      if (th->suspended)
     {
       if (th->context.ContextFlags)
 	{
@@ -437,6 +447,7 @@ continue_one_thread (struct inferior_list_entry *this_thread, void *id_ptr)
 	}
       th->suspended = 0;
     }
+    }
 
   return 0;
 }
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index fe40c4d..6a0c861 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -312,9 +312,10 @@ thread_rec (DWORD id, int get_context)
 		    warning (_("SuspendThread (tid=0x%x) failed."
 			       " (winerr %u)"),
 			     (unsigned) id, (unsigned) err);
-		    return NULL;
+		    th->suspended = -2;
 		  }
-		th->suspended = 1;
+		else
+		  th->suspended = 1;
 	      }
 	    else if (get_context < 0)
 	      th->suspended = -1;
@@ -1200,6 +1201,12 @@ windows_continue (DWORD continue_status, int id)
     if ((id == -1 || id == (int) th->id)
 	&& th->suspended)
       {
+	if (th->suspended == -2)
+	  {
+	    th->suspended = 1;
+	    error ("SuspendThread had failed.\n");
+	  }
+
 	if (debug_registers_changed)
 	  {
 	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
-- 
1.7.11.7


[-- Attachment #3: win32_spawn_threads.c --]
[-- Type: text/x-csrc, Size: 2641 bytes --]

#include <windows.h>

static DWORD WINAPI
thread_entry (void *arg)
{
  return 0;
}

int
main ()
{
  DWORD threadId;

	
  while (1)
    {
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
      CreateThread (NULL, 0, thread_entry, NULL, 0, &threadId);
    }
}

[-- Attachment #4: win32_spawn_threads.gdb --]
[-- Type: text/plain, Size: 110 bytes --]

#start
tb main
continue

break 17
command
	continue
end

#handle SIGTRAP nostop

set pagination off

continue

[-- Attachment #5: win32_suspend_issue.c --]
[-- Type: text/x-csrc, Size: 103 bytes --]

#include <windows.h>

int main()
{    
    while (1)
        DebugBreakProcess(GetCurrentProcess());
}

[-- Attachment #6: win32_suspend_issue.gdb --]
[-- Type: text/plain, Size: 108 bytes --]

#start
tb main
continue

break 6
command
	continue
end

handle SIGTRAP nostop

set pagination off

continue

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Tue, 08 Apr 2014 17:42:56 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: brobecker@adacore.com, gdb-patches@sourceware.org
> 
> I'd be very curious to see the backtrace you get
> for the failing thread in your test case (I guess emacs?).

Yes, it's Emacs.  Do you mean the backtrace I see when debugging
natively?  Because when debugging Emacs with gdbserver, I cannot
reproduce the problem with SuspendThread.

> +		    /* We get Access Denied (5) when trying to suspend
> +		       threads that Windows started on behalf of the
> +		       debuggee, usually when those threads are just
> +		       about to exit.  */
> +		    if (err != ERROR_ACCESS_DENIED)
> 
> I've shown above that whether it was Windows or the program
> itself that started the threads is irrelevant, it'd be good to 
> reword this comment.

OK.  But now I'm confused: what is the conclusion from what you saw?

Re: [PATCH] Fix "PC register is not available" issue

[Pedro Alves]

On 04/08/2014 06:10 PM, Eli Zaretskii wrote:
>> Date: Tue, 08 Apr 2014 17:42:56 +0100
>> From: Pedro Alves <palves@redhat.com>
>> CC: brobecker@adacore.com, gdb-patches@sourceware.org
>>
>> I'd be very curious to see the backtrace you get
>> for the failing thread in your test case (I guess emacs?).
> 
> Yes, it's Emacs.  Do you mean the backtrace I see when debugging
> natively?

Yes, but you'll need to use the patch I attached, not yours.
When SuspendThread fails, the warning says which thread failed.
The next "continue", "next", whatever will fail when that
happens (just once, so you can continue debugging as usual
in case you're in the middle of a real debug session).  At
that point, "info threads", see which is the gdb thread id
for the thread that failed, switch to it, and get a backtrace,
like I showed in the previous email.

> Because when debugging Emacs with gdbserver, I cannot
> reproduce the problem with SuspendThread.

You mean you can't get the warning in GDBserver's console,
or you don't see the "PC register is not available" issue?
Even if the exact test case as in the PR (and as I attached
to the previous email) -- that is, one that continues banging
in a loop until the failure triggers?  Or you mean with
emacs?

As I mentioned in the previous email, you won't get the
"PC register not available" issue with GDBserver because GDBserver's
thread_rec returns the thread's info structure anyway even if
SuspendThread failed, unlike GDB, which returns NULL.  If
the SuspendThread issue triggers, then the
GetThreadContext/SetThreadContext issue should trigger too.
But, GDBserver actually currently ignores SetThreadContext
fails ...:

static void
i386_set_thread_context (win32_thread_info *th, DEBUG_EVENT* current_event)
{
  if (debug_registers_changed)
    {
      struct i386_debug_reg_state *dr = &debug_reg_state;
      th->context.Dr0 = dr->dr_mirror[0];
      th->context.Dr1 = dr->dr_mirror[1];
      th->context.Dr2 = dr->dr_mirror[2];
      th->context.Dr3 = dr->dr_mirror[3];
      /* th->context.Dr6 = dr->dr_status_mirror;
	 FIXME: should we set dr6 also ?? */
      th->context.Dr7 = dr->dr_control_mirror;
    }

  SetThreadContext (th->h, &th->context);
}



Given I see different backtraces on the same machine in native vs
gdbserver debugging of the same test, and that in gdbserver's case the
failing thread appears to always be further down into thread termination,
it may just be that your machine is a little slower or faster than
mine, and it's harder to stop threads at exactly within the time window
when the SuspendThread problem can trigger.

>> +		    /* We get Access Denied (5) when trying to suspend
>> +		       threads that Windows started on behalf of the
>> +		       debuggee, usually when those threads are just
>> +		       about to exit.  */
>> +		    if (err != ERROR_ACCESS_DENIED)
>>
>> I've shown above that whether it was Windows or the program
>> itself that started the threads is irrelevant, it'd be good to 
>> reword this comment.
> 
> OK.  But now I'm confused: what is the conclusion from what you saw?

My conclusion so far is that this happens exactly when we try to
suspend a thread that is already half-dead, no matter who started it,
and that we do need your patch to GDB, and that GDBserver will also
need to the patched.  I'm just curious to see your emacs backtrace
to 99% confirm that it's the same thread-exiting scenario, mainly
to put any doubts to rest, for us and for future generations (the
archives).

-- 
Pedro Alves

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Tue, 08 Apr 2014 18:36:37 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: brobecker@adacore.com, gdb-patches@sourceware.org
> 
> On 04/08/2014 06:10 PM, Eli Zaretskii wrote:
> >> Date: Tue, 08 Apr 2014 17:42:56 +0100
> >> From: Pedro Alves <palves@redhat.com>
> >> CC: brobecker@adacore.com, gdb-patches@sourceware.org
> >>
> >> I'd be very curious to see the backtrace you get
> >> for the failing thread in your test case (I guess emacs?).
> > 
> > Yes, it's Emacs.  Do you mean the backtrace I see when debugging
> > natively?
> 
> Yes, but you'll need to use the patch I attached, not yours.
> When SuspendThread fails, the warning says which thread failed.
> The next "continue", "next", whatever will fail when that
> happens (just once, so you can continue debugging as usual
> in case you're in the middle of a real debug session).  At
> that point, "info threads", see which is the gdb thread id
> for the thread that failed, switch to it, and get a backtrace,
> like I showed in the previous email.

I can know which thread's suspension fails even without the patch,
because GDB immediately announces its exit.  Is the patch just for
showing the thread ID, or is it needed for something else?

> > Because when debugging Emacs with gdbserver, I cannot
> > reproduce the problem with SuspendThread.
> 
> You mean you can't get the warning in GDBserver's console,
> or you don't see the "PC register is not available" issue?

The former, of course.

> Even if the exact test case as in the PR (and as I attached
> to the previous email) -- that is, one that continues banging
> in a loop until the failure triggers?  Or you mean with
> emacs?

I mean with Emacs.  The PR test case is an artificial toy program
which also floods the system with threads.  I wouldn't consider it a
serious real-life use case.

> Given I see different backtraces on the same machine in native vs
> gdbserver debugging of the same test, and that in gdbserver's case the
> failing thread appears to always be further down into thread termination,
> it may just be that your machine is a little slower or faster than
> mine, and it's harder to stop threads at exactly within the time window
> when the SuspendThread problem can trigger.

I think multiple cores also come into play here.  (My machine is Core
i7.)

> > OK.  But now I'm confused: what is the conclusion from what you saw?
> 
> My conclusion so far is that this happens exactly when we try to
> suspend a thread that is already half-dead, no matter who started it,
> and that we do need your patch to GDB, and that GDBserver will also
> need to the patched.  I'm just curious to see your emacs backtrace
> to 99% confirm that it's the same thread-exiting scenario, mainly
> to put any doubts to rest, for us and for future generations (the
> archives).

OK, I will try to get that info.

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

Hello,

I was wondering what the status of the GDB-side of the patch was.
I thought we were all set to commit it, while also looking at
applying the same patch on the GDBserver side? I don't think we are
in any hurry to get anything committed, but I'd had to see all
this work get forgotten and lost... Also, I don't think the patch
is all that risky, but the earlier we put it in, the more exposure
it'll get before we release 7.8!

Thanks,
-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Fri, 11 Apr 2014 13:06:09 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
> 
> I was wondering what the status of the GDB-side of the patch was.
> I thought we were all set to commit it, while also looking at
> applying the same patch on the GDBserver side? I don't think we are
> in any hurry to get anything committed, but I'd had to see all
> this work get forgotten and lost... Also, I don't think the patch
> is all that risky, but the earlier we put it in, the more exposure
> it'll get before we release 7.8!

As I wrote on gdb@, I have committed the patch to fix the GDB side of
the problem.  See below for what I actually committed, both to master
and to the GDB 7.7 branch.

Should we now close the Bugzilla PR?  If I can do that, any pointers
as to how?

commit 17617f2d366ca969ccbc784be4f75931a1afd20f
Author: Eli Zaretskii <eliz@gnu.org>
Date:   Sat Apr 19 11:12:19 2014 +0300

    PR gdb/14018 -- avoid "PC register not available" errors.
    
    gdb/windows-nat.c (thread_rec): Don't display a warning when
    SuspendThread fails with ERROR_ACCESS_DENIED.  If SuspendThread
    fails for any reason, set th->suspended to -1, so that we don't
    try to resume such a thread.  Also, don't return NULL in these
    cases, to avoid completely ruin the session due to "PC register is
    not available" error.
    (do_windows_fetch_inferior_registers): Check errors in
    GetThreadContext call.
    (windows_continue): Accept an additional argument KILLED; if not
    zero, ignore errors in the SetThreadContext call, since the
    inferior was killed and is shutting down.
    (windows_resume, get_windows_debug_event)
    (windows_create_inferior, windows_mourn_inferior)
    (windows_kill_inferior): All callers of windows_continue changed
    to adjust to its new calling sequence.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fd9677b..23ca6c0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,22 @@
+2014-04-19  Eli Zaretskii  <eliz@gnu.org>
+
+	PR gdb/14018
+	* windows-nat.c (thread_rec): Don't display a warning when
+	SuspendThread fails with ERROR_ACCESS_DENIED.  If SuspendThread
+	fails for any reason, set th->suspended to -1, so that we don't
+	try to resume such a thread.  Also, don't return NULL in these
+	cases, to avoid completely ruin the session due to "PC register is
+	not available" error.
+	(do_windows_fetch_inferior_registers): Check errors in
+	GetThreadContext call.
+	(windows_continue): Accept an additional argument KILLED; if not
+	zero, ignore errors in the SetThreadContext call, since the
+	inferior was killed and is shutting down.
+	(windows_resume, get_windows_debug_event)
+	(windows_create_inferior, windows_mourn_inferior)
+	(windows_kill_inferior): All callers of windows_continue changed
+	to adjust to its new calling sequence.
+
 2014-04-19  Yao Qi  <yao@codesourcery.com>
 
 	* ctf.c (ctf_open): Call post_create_inferior.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index fe40c4d..bad7408 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -309,12 +309,18 @@ thread_rec (DWORD id, int get_context)
 		  {
 		    DWORD err = GetLastError ();
 
-		    warning (_("SuspendThread (tid=0x%x) failed."
-			       " (winerr %u)"),
-			     (unsigned) id, (unsigned) err);
-		    return NULL;
+		    /* We get Access Denied (5) when trying to suspend
+		       threads that Windows started on behalf of the
+		       debuggee, usually when those threads are just
+		       about to exit.  */
+		    if (err != ERROR_ACCESS_DENIED)
+		      warning (_("SuspendThread (tid=0x%x) failed."
+				 " (winerr %u)"),
+			       (unsigned) id, (unsigned) err);
+		    th->suspended = -1;
 		  }
-		th->suspended = 1;
+		else
+		  th->suspended = 1;
 	      }
 	    else if (get_context < 0)
 	      th->suspended = -1;
@@ -444,7 +450,7 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
 	{
 	  thread_info *th = current_thread;
 	  th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
-	  GetThreadContext (th->h, &th->context);
+	  CHECK (GetThreadContext (th->h, &th->context));
 	  /* Copy dr values from that thread.
 	     But only if there were not modified since last stop.
 	     PR gdb/2388 */
@@ -1181,10 +1187,12 @@ handle_exception (struct target_waitstatus *ourstatus)
   return 1;
 }
 
-/* Resume all artificially suspended threads if we are continuing
-   execution.  */
+/* Resume thread specified by ID, or all artificially suspended
+   threads, if we are continuing execution.  KILLED non-zero means we
+   have killed the inferior, so we should ignore weird errors due to
+   threads shutting down.  */
 static BOOL
-windows_continue (DWORD continue_status, int id)
+windows_continue (DWORD continue_status, int id, int killed)
 {
   int i;
   thread_info *th;
@@ -1212,7 +1220,16 @@ windows_continue (DWORD continue_status, int id)
 	  }
 	if (th->context.ContextFlags)
 	  {
-	    CHECK (SetThreadContext (th->h, &th->context));
+	    DWORD ec = 0;
+
+	    if (GetExitCodeThread (th->h, &ec)
+		&& ec == STILL_ACTIVE)
+	      {
+		BOOL status = SetThreadContext (th->h, &th->context);
+
+		if (!killed)
+		  CHECK (status);
+	      }
 	    th->context.ContextFlags = 0;
 	  }
 	if (th->suspended > 0)
@@ -1340,9 +1357,9 @@ windows_resume (struct target_ops *ops,
      Otherwise complain.  */
 
   if (resume_all)
-    windows_continue (continue_status, -1);
+    windows_continue (continue_status, -1, 0);
   else
-    windows_continue (continue_status, ptid_get_tid (ptid));
+    windows_continue (continue_status, ptid_get_tid (ptid), 0);
 }
 
 /* Ctrl-C handler used when the inferior is not run in the same console.  The
@@ -1560,7 +1577,7 @@ get_windows_debug_event (struct target_ops *ops,
       if (continue_status == -1)
 	windows_resume (ops, minus_one_ptid, 0, 1);
       else
-	CHECK (windows_continue (continue_status, -1));
+	CHECK (windows_continue (continue_status, -1, 0));
     }
   else
     {
@@ -2337,13 +2354,13 @@ windows_create_inferior (struct target_ops *ops, char *exec_file,
 
   do_initial_windows_stuff (ops, pi.dwProcessId, 0);
 
-  /* windows_continue (DBG_CONTINUE, -1); */
+  /* windows_continue (DBG_CONTINUE, -1, 0); */
 }
 
 static void
 windows_mourn_inferior (struct target_ops *ops)
 {
-  (void) windows_continue (DBG_CONTINUE, -1);
+  (void) windows_continue (DBG_CONTINUE, -1, 0);
   i386_cleanup_dregs();
   if (open_process_used)
     {
@@ -2412,7 +2429,7 @@ windows_kill_inferior (struct target_ops *ops)
 
   for (;;)
     {
-      if (!windows_continue (DBG_CONTINUE, -1))
+      if (!windows_continue (DBG_CONTINUE, -1, 1))
 	break;
       if (!WaitForDebugEvent (&current_event, INFINITE))
 	break;

Re: [PATCH] Fix "PC register is not available" issue

[Joel Brobecker]

> As I wrote on gdb@, I have committed the patch to fix the GDB side of
> the problem.  See below for what I actually committed, both to master
> and to the GDB 7.7 branch.

I didn't know we were planning on putting the change in the GDB 7.7
branch as well, but since you've tested it for several weeks, and
it's been seen by Pedro as well, we can indeed try to have it for
7.7.1 as well. The one thing I do ask when pushing a patch to the branch
is to update the release Wiki page to add a reference to the fix:
https://sourceware.org/gdb/wiki/GDB_7.7_Release

I've done it for you, since there was already a PR.

> Should we now close the Bugzilla PR?  If I can do that, any pointers
> as to how?

Just log in, and change the state to "RESOLVED" + "FIXED".
I was already logged in, so did it as well.

-- 
Joel

Re: [PATCH] Fix "PC register is not available" issue

[Eli Zaretskii]

> Date: Mon, 21 Apr 2014 08:43:33 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: palves@redhat.com, gdb-patches@sourceware.org
> 
> > As I wrote on gdb@, I have committed the patch to fix the GDB side of
> > the problem.  See below for what I actually committed, both to master
> > and to the GDB 7.7 branch.
> 
> I didn't know we were planning on putting the change in the GDB 7.7
> branch as well, but since you've tested it for several weeks, and
> it's been seen by Pedro as well, we can indeed try to have it for
> 7.7.1 as well. The one thing I do ask when pushing a patch to the branch
> is to update the release Wiki page to add a reference to the fix:
> https://sourceware.org/gdb/wiki/GDB_7.7_Release
> 
> I've done it for you, since there was already a PR.
> 
> > Should we now close the Bugzilla PR?  If I can do that, any pointers
> > as to how?
> 
> Just log in, and change the state to "RESOLVED" + "FIXED".
> I was already logged in, so did it as well.

Thanks, on both accounts.