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

  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(-)
  

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
  

On 2017-03-20 11:56, Pedro Alves wrote:
> 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?

Oops, I guess it should be th, like the rest:

   char *context_offset = ((char *) th->context) + mappings[r];

Fixed locally.

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

Fixed locally.

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

All I know is that this is the patch that introduced them:
https://sourceware.org/ml/gdb-patches/2003-12/msg00479.html

The PR 1048 seems to refer to a pre-bugzilla bug tracking system.  Do we 
still have them somewhere?

 From what I understand, it's the use case where you attach to a process 
whose main thread has already exited.  If the patch introduced these 
NULL checks, I suppose it's because they were necessary back then to 
work around the Windows bug.  I have no idea if they are still 
necessary, or if the Microsoft people fixed it.  In any case, the fact 
of whether the checks are needed is not impacted by the current patch: 
in the end, we call thread_rec with the same pid with which we would 
have called it before, so we should get the same result.

I'll wait for your input on this before sending a new version.

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

Fixed locally.

Thanks,

Simon
  

On 03/20/2017 10:22 PM, Simon Marchi wrote:

>>>  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?
> 
> Oops, I guess it should be th, like the rest:
> 
>   char *context_offset = ((char *) th->context) + mappings[r];
> 
> Fixed locally.

Thanks.

> 
>>> @@ -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.
> 
> Fixed locally.
> 
>> 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.
> 
> All I know is that this is the patch that introduced them:
> https://sourceware.org/ml/gdb-patches/2003-12/msg00479.html
> 
> The PR 1048 seems to refer to a pre-bugzilla bug tracking system.  Do we
> still have them somewhere?

Here:

 https://sourceware.org/gdb/wiki/DeveloperTips?highlight=%28gnats%29#Finding_Gnats_bug_entries_in_the_Bugzilla_database

gnats 1048 + 7105 -> bugzilla 8153:

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

> 
> From what I understand, it's the use case where you attach to a process
> whose main thread has already exited.  If the patch introduced these
> NULL checks, I suppose it's because they were necessary back then to
> work around the Windows bug.  I have no idea if they are still
> necessary, or if the Microsoft people fixed it.  

[...]

> In any case, the fact
> of whether the checks are needed is not impacted by the current patch:
> in the end, we call thread_rec with the same pid with which we would
> have called it before, so we should get the same result.

You're right.

> 
> I'll wait for your input on this before sending a new version.
> 

I don't have further input.

Thanks,
Pedro Alves
  

On 2017-03-21 10:27, Pedro Alves wrote:
>>> 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.
>> 
>> All I know is that this is the patch that introduced them:
>> https://sourceware.org/ml/gdb-patches/2003-12/msg00479.html
>> 
>> The PR 1048 seems to refer to a pre-bugzilla bug tracking system.  Do 
>> we
>> still have them somewhere?
> 
> Here:
> 
> 
> https://sourceware.org/gdb/wiki/DeveloperTips?highlight=%28gnats%29#Finding_Gnats_bug_entries_in_the_Bugzilla_database
> 
> gnats 1048 + 7105 -> bugzilla 8153:
> 
>  https://sourceware.org/bugzilla/show_bug.cgi?id=8153

Ah, thanks!

>> From what I understand, it's the use case where you attach to a 
>> process
>> whose main thread has already exited.  If the patch introduced these
>> NULL checks, I suppose it's because they were necessary back then to
>> work around the Windows bug.  I have no idea if they are still
>> necessary, or if the Microsoft people fixed it.
> 
> [...]
> 
>> In any case, the fact
>> of whether the checks are needed is not impacted by the current patch:
>> in the end, we call thread_rec with the same pid with which we would
>> have called it before, so we should get the same result.
> 
> You're right.
> 
>> 
>> I'll wait for your input on this before sending a new version.
>> 
> 
> I don't have further input.

Thanks, I'll push the patch including the fixes and send the final 
version for reference.