Re: [PATCH 3/3] windows: Use ptid from regcache in register fetch/store

[Pedro Alves <palves@redhat.com>]

On 03/18/2017 05:08 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@ericsson.com>
> 
> Use the ptid from the regcache so we don't depend on the current value
> of the inferior_ptid global.
> 
> Also, change how the current thread is passed to sub-functions.  The
> windows_fetch_inferior_registers function sets current_thread then calls
> do_windows_fetch_inferior_registers, which reads current_thread.  This
> very much looks like passing a parameter through a global variable.  I
> think it would be more straightforward to pass the thread as a
> parameter.
> 
> gdb/ChangeLog:
> 
> 	* windows-nat.c (do_windows_fetch_inferior_registers): Add
> 	windows_thread_info parameter and use it instead of
> 	current_thread.
> 	(windows_fetch_inferior_registers): Don't set current_thread,
> 	pass the thread to do_windows_fetch_inferior_registers.  Use
> 	ptid from regcache instead of inferior_ptid.
> 	(do_windows_store_inferior_registers): Add windows_thread_info
> 	parameter and use it instead of current_thread.
> 	(windows_store_inferior_registers): Don't set current_thread,
> 	pass the thread to do_windows_store_inferior_registers.  Use
> 	ptid from regcache instead of inferior_ptid.
> ---
>  gdb/windows-nat.c | 43 +++++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 9cc755f0d4..32a9ee62cf 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -460,18 +460,15 @@ windows_delete_thread (ptid_t ptid, DWORD exit_code)
>  }
>  
>  static void
> -do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
> +do_windows_fetch_inferior_registers (struct regcache *regcache,
> +				     windows_thread_info *th, int r)
>  {
>    char *context_offset = ((char *) &current_thread->context) + mappings[r];

Is this reference to "current_thread" still correct?

>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    long l;
>  
> -  if (!current_thread)
> -    return;	/* Windows sometimes uses a non-existent thread id in its
> -		   events.  */
> -
> -  if (current_thread->reload_context)
> +  if (th->reload_context)
>      {
>  #ifdef __CYGWIN__
>        if (have_saved_context)
> @@ -480,14 +477,13 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
>  	     cygwin has informed us that we should consider the signal
>  	     to have occurred at another location which is stored in
>  	     "saved_context.  */
> -	  memcpy (&current_thread->context, &saved_context,
> +	  memcpy (&th->context, &saved_context,
>  		  __COPY_CONTEXT_SIZE);
>  	  have_saved_context = 0;
>  	}
>        else
>  #endif
>  	{
> -	  windows_thread_info *th = current_thread;
>  	  th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
>  	  CHECK (GetThreadContext (th->h, &th->context));
>  	  /* Copy dr values from that thread.
> @@ -503,7 +499,7 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
>  	      dr[7] = th->context.Dr7;
>  	    }
>  	}
> -      current_thread->reload_context = 0;
> +      th->reload_context = 0;
>      }
>  
>    if (r == I387_FISEG_REGNUM (tdep))
> @@ -529,7 +525,7 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
>    else
>      {
>        for (r = 0; r < gdbarch_num_regs (gdbarch); r++)
> -	do_windows_fetch_inferior_registers (regcache, r);
> +	do_windows_fetch_inferior_registers (regcache, th, r);
>      }
>  }
>  
> @@ -537,25 +533,26 @@ static void
>  windows_fetch_inferior_registers (struct target_ops *ops,
>  				  struct regcache *regcache, int r)
>  {
> -  current_thread = thread_rec (ptid_get_tid (inferior_ptid), TRUE);
> +  DWORD pid = ptid_get_tid (regcache_get_ptid (regcache));
> +  windows_thread_info *th = thread_rec (pid, TRUE);
> +
>    /* Check if current_thread exists.  Windows sometimes uses a non-existent
>       thread id in its events.  */

The comment is out of date now.

Did you look for the history around these comments?  I wonder whether
these NULL checks still make sense here if we always reference the
regcache's thread.  The equivalent code in gdbserver doesn't seem to
have them.

> -  if (current_thread)
> -    do_windows_fetch_inferior_registers (regcache, r);
> +  if (th != NULL)
> +    do_windows_fetch_inferior_registers (regcache, th, r);
>  }
>  
>  static void
> -do_windows_store_inferior_registers (const struct regcache *regcache, int r)
> +do_windows_store_inferior_registers (const struct regcache *regcache,
> +				     windows_thread_info *th, int r)
>  {
> -  if (!current_thread)
> -    /* Windows sometimes uses a non-existent thread id in its events.  */;
> -  else if (r >= 0)
> +  if (r >= 0)
>      regcache_raw_collect (regcache, r,
> -			  ((char *) &current_thread->context) + mappings[r]);
> +			  ((char *) &th->context) + mappings[r]);
>    else
>      {
>        for (r = 0; r < gdbarch_num_regs (get_regcache_arch (regcache)); r++)
> -	do_windows_store_inferior_registers (regcache, r);
> +	do_windows_store_inferior_registers (regcache, th, r);
>      }
>  }
>  
> @@ -564,11 +561,13 @@ static void
>  windows_store_inferior_registers (struct target_ops *ops,
>  				  struct regcache *regcache, int r)
>  {
> -  current_thread = thread_rec (ptid_get_tid (inferior_ptid), TRUE);
> +  DWORD pid = ptid_get_tid (regcache_get_ptid (regcache));
> +  windows_thread_info *th = thread_rec (pid, TRUE);
> +
>    /* Check if current_thread exists.  Windows sometimes uses a non-existent
>       thread id in its events.  */

Ditto.

> -  if (current_thread)
> -    do_windows_store_inferior_registers (regcache, r);
> +  if (th != NULL)
> +    do_windows_store_inferior_registers (regcache, th, r);
>  }
>  
>  /* Encapsulate the information required in a call to
> 

Thanks,
Pedro Alves