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

[Simon Marchi <simon.marchi@polymtl.ca>]

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