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
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at polymtl dot ca>, gdb-patches at sourceware dot org
- Cc: Simon Marchi <simon dot marchi at ericsson dot com>
- Date: Mon, 20 Mar 2017 15:56:38 +0000
- Subject: Re: [PATCH 3/3] windows: Use ptid from regcache in register fetch/store
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 3187E4DD7D
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3187E4DD7D
- References: <20170318170801.22988-1-simon.marchi@polymtl.ca> <20170318170801.22988-3-simon.marchi@polymtl.ca>
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?
> struct gdbarch *gdbarch = get_regcache_arch (regcache);
> struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> long l;
>
> - if (!current_thread)
> - return; /* Windows sometimes uses a non-existent thread id in its
> - events. */
> -
> - if (current_thread->reload_context)
> + if (th->reload_context)
> {
> #ifdef __CYGWIN__
> if (have_saved_context)
> @@ -480,14 +477,13 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
> cygwin has informed us that we should consider the signal
> to have occurred at another location which is stored in
> "saved_context. */
> - memcpy (¤t_thread->context, &saved_context,
> + memcpy (&th->context, &saved_context,
> __COPY_CONTEXT_SIZE);
> have_saved_context = 0;
> }
> else
> #endif
> {
> - windows_thread_info *th = current_thread;
> th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
> CHECK (GetThreadContext (th->h, &th->context));
> /* Copy dr values from that thread.
> @@ -503,7 +499,7 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
> dr[7] = th->context.Dr7;
> }
> }
> - current_thread->reload_context = 0;
> + th->reload_context = 0;
> }
>
> if (r == I387_FISEG_REGNUM (tdep))
> @@ -529,7 +525,7 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
> else
> {
> for (r = 0; r < gdbarch_num_regs (gdbarch); r++)
> - do_windows_fetch_inferior_registers (regcache, r);
> + do_windows_fetch_inferior_registers (regcache, th, r);
> }
> }
>
> @@ -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.
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.
> - if (current_thread)
> - do_windows_fetch_inferior_registers (regcache, r);
> + if (th != NULL)
> + do_windows_fetch_inferior_registers (regcache, th, r);
> }
>
> static void
> -do_windows_store_inferior_registers (const struct regcache *regcache, int r)
> +do_windows_store_inferior_registers (const struct regcache *regcache,
> + windows_thread_info *th, int r)
> {
> - if (!current_thread)
> - /* Windows sometimes uses a non-existent thread id in its events. */;
> - else if (r >= 0)
> + if (r >= 0)
> regcache_raw_collect (regcache, r,
> - ((char *) ¤t_thread->context) + mappings[r]);
> + ((char *) &th->context) + mappings[r]);
> else
> {
> for (r = 0; r < gdbarch_num_regs (get_regcache_arch (regcache)); r++)
> - do_windows_store_inferior_registers (regcache, r);
> + do_windows_store_inferior_registers (regcache, th, r);
> }
> }
>
> @@ -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.
> - if (current_thread)
> - do_windows_store_inferior_registers (regcache, r);
> + if (th != NULL)
> + do_windows_store_inferior_registers (regcache, th, r);
> }
>
> /* Encapsulate the information required in a call to
>
Thanks,
Pedro Alves