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

[Pedro Alves <palves@redhat.com>]

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