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 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 *) ¤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.
@@ -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