This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 *) &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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]