This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]