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

[Pedro Alves <palves@redhat.com>]

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