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

[Eli Zaretskii <eliz at gnu dot org>]

> 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.