This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/3] windows: Use ptid from regcache in register fetch/store
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 *) ¤t_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