RE: [PATCH v2 06/11] Move x86 register code into x86-windows-nat.c

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----Original Message-----
> From: Hannes Domani <ssbssa@yahoo.de>
> Sent: Samstag, 17. Januar 2026 14:36
> To: gdb-patches@sourceware.org
> Subject: [PATCH v2 06/11] Move x86 register code into x86-windows-nat.c
> 
> ---
> Changes in v2:
>   - updated comment for initialize_windows_arch() describing the ATTACHING
>     argument
> ---
>  gdb/windows-nat.c     | 144 ++----------------------------------------
>  gdb/windows-nat.h     |  29 +++++++--
>  gdb/x86-windows-nat.c | 137
> +++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 163 insertions(+), 147 deletions(-)
> 
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index
> e1da956b125..926b85d479b 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -57,9 +57,6 @@
>  #include "xml-support.h"
>  #include "inttypes.h"
> 
> -#include "i386-tdep.h"
> -#include "i387-tdep.h"
> -
>  #include "windows-tdep.h"
>  #include "windows-nat.h"
>  #include "complaints.h"
> @@ -389,79 +386,6 @@ windows_nat_target::delete_thread (ptid_t ptid,
> DWORD exit_code,
>      windows_process->thread_list.erase (iter);  }
> 
> -/* Fetches register number R from the given windows_thread_info,
> -   and supplies its value to the given regcache.
> -
> -   This function assumes that R is non-negative.  A failed assertion
> -   is raised if that is not true.
> -
> -   This function assumes that TH->RELOAD_CONTEXT is not set, meaning
> -   that the windows_thread_info has an up-to-date context.  A failed
> -   assertion is raised if that assumption is violated.  */
> -
> -static void
> -windows_fetch_one_register (struct regcache *regcache,
> -			    windows_thread_info *th, int r)
> -{
> -  gdb_assert (r >= 0);
> -  gdb_assert (!th->reload_context);
> -
> -  char *context_ptr = windows_process->with_context (th, [] (auto *context)
> -    {
> -      return (char *) context;
> -    });
> -
> -  char *context_offset = context_ptr + windows_process->mappings[r];
> -  struct gdbarch *gdbarch = regcache->arch ();
> -  i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
> -
> -  gdb_assert (!gdbarch_read_pc_p (gdbarch));
> -  gdb_assert (gdbarch_pc_regnum (gdbarch) >= 0);
> -  gdb_assert (!gdbarch_write_pc_p (gdbarch));
> -
> -  /* GDB treats some registers as 32-bit, where they are in fact only
> -     16 bits long.  These cases must be handled specially to avoid
> -     reading extraneous bits from the context.  */
> -  if (r == I387_FISEG_REGNUM (tdep) || windows_process-
> >segment_register_p (r))
> -    {
> -      gdb_byte bytes[4] = {};
> -      memcpy (bytes, context_offset, 2);
> -      regcache->raw_supply (r, bytes);
> -    }
> -  else if (r == I387_FOP_REGNUM (tdep))
> -    {
> -      long l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
> -      regcache->raw_supply (r, &l);
> -    }
> -  else
> -    {
> -      if (th->stopped_at_software_breakpoint
> -	  && !th->pc_adjusted
> -	  && r == gdbarch_pc_regnum (gdbarch))
> -	{
> -	  int size = register_size (gdbarch, r);
> -	  if (size == 4)
> -	    {
> -	      uint32_t value;
> -	      memcpy (&value, context_offset, size);
> -	      value -= gdbarch_decr_pc_after_break (gdbarch);
> -	      memcpy (context_offset, &value, size);
> -	    }
> -	  else
> -	    {
> -	      gdb_assert (size == 8);
> -	      uint64_t value;
> -	      memcpy (&value, context_offset, size);
> -	      value -= gdbarch_decr_pc_after_break (gdbarch);
> -	      memcpy (context_offset, &value, size);
> -	    }
> -	  /* Make sure we only rewrite the PC a single time.  */
> -	  th->pc_adjusted = true;
> -	}
> -      regcache->raw_supply (r, context_offset);
> -    }
> -}
> -
>  void
>  windows_nat_target::fetch_registers (struct regcache *regcache, int r)  { @@
> -482,51 +406,9 @@ windows_nat_target::fetch_registers (struct regcache
> *regcache, int r)
> 
>    if (r < 0)
>      for (r = 0; r < gdbarch_num_regs (regcache->arch()); r++)
> -      windows_fetch_one_register (regcache, th, r);
> -  else
> -    windows_fetch_one_register (regcache, th, r);
> -}
> -
> -/* Collect the register number R from the given regcache, and store
> -   its value into the corresponding area of the given thread's context.
> -
> -   This function assumes that R is non-negative.  A failed assertion
> -   assertion is raised if that is not true.  */
> -
> -static void
> -windows_store_one_register (const struct regcache *regcache,
> -			    windows_thread_info *th, int r)
> -{
> -  gdb_assert (r >= 0);
> -
> -  char *context_ptr = windows_process->with_context (th, [] (auto *context)
> -    {
> -      return (char *) context;
> -    });
> -
> -  struct gdbarch *gdbarch = regcache->arch ();
> -  i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
> -
> -  /* GDB treats some registers as 32-bit, where they are in fact only
> -     16 bits long.  These cases must be handled specially to avoid
> -     overwriting other registers in the context.  */
> -  if (r == I387_FISEG_REGNUM (tdep) || windows_process-
> >segment_register_p (r))
> -    {
> -      gdb_byte bytes[4];
> -      regcache->raw_collect (r, bytes);
> -      memcpy (context_ptr + windows_process->mappings[r], bytes, 2);
> -    }
> -  else if (r == I387_FOP_REGNUM (tdep))
> -    {
> -      gdb_byte bytes[4];
> -      regcache->raw_collect (r, bytes);
> -      /* The value of FOP occupies the top two bytes in the context,
> -	 so write the two low-order bytes from the cache into the
> -	 appropriate spot.  */
> -      memcpy (context_ptr + windows_process->mappings[r] + 2, bytes, 2);
> -    }
> +      fetch_one_register (regcache, th, r);
>    else
> -    regcache->raw_collect (r, context_ptr + windows_process->mappings[r]);
> +    fetch_one_register (regcache, th, r);
>  }
> 
>  /* Store a new register value into the context of the thread tied to @@ -
> 545,9 +427,9 @@ windows_nat_target::store_registers (struct regcache
> *regcache, int r)
> 
>    if (r < 0)
>      for (r = 0; r < gdbarch_num_regs (regcache->arch ()); r++)
> -      windows_store_one_register (regcache, th, r);
> +      store_one_register (regcache, th, r);
>    else
> -    windows_store_one_register (regcache, th, r);
> +    store_one_register (regcache, th, r);
>  }
> 
>  bool
> @@ -1472,7 +1354,7 @@ windows_nat_target::do_initial_windows_stuff
> (DWORD pid, bool attaching)  {
>    struct inferior *inf;
> 
> -  initialize_windows_arch ();
> +  initialize_windows_arch (attaching);
> 
>    windows_process->last_sig = GDB_SIGNAL_0;
>    windows_process->open_process_used = 0; @@ -1490,22 +1372,6 @@
> windows_nat_target::do_initial_windows_stuff (DWORD pid, bool attaching)
>    clear_proceed_status (0);
>    init_wait_for_inferior ();
> 
> -#ifdef __x86_64__
> -  windows_process->ignore_first_breakpoint
> -    = !attaching && windows_process->wow64_process;
> -
> -  if (!windows_process->wow64_process)
> -    {
> -      windows_process->mappings  = amd64_mappings;
> -      windows_process->segment_register_p =
> amd64_windows_segment_register_p;
> -    }
> -  else
> -#endif
> -    {
> -      windows_process->mappings  = i386_mappings;
> -      windows_process->segment_register_p =
> i386_windows_segment_register_p;
> -    }
> -
>    inferior_appeared (inf, pid);
>    inf->attach_flag = attaching;
> 
> diff --git a/gdb/windows-nat.h b/gdb/windows-nat.h index
> 6553052f420..df872907535 100644
> --- a/gdb/windows-nat.h
> +++ b/gdb/windows-nat.h
> @@ -83,10 +83,6 @@ struct windows_per_inferior : public
> windows_nat::windows_process_info
> 
>    const int *mappings = nullptr;
> 
> -  /* The function to use in order to determine whether a register is
> -     a segment register or not.  */
> -  segment_register_p_ftype *segment_register_p = nullptr;
> -
>    std::vector<windows_solib> solibs;
> 
>  #ifdef __CYGWIN__
> @@ -184,8 +180,9 @@ struct windows_nat_target : public inf_child_target
>  protected:
> 
>    /* Initialize arch-specific data for a new inferior (debug registers,
> -     register mappings).  */
> -  virtual void initialize_windows_arch () = 0;
> +     register mappings).  If ATTACHING is true, we're attaching to an
> +     already-running process.  */
> +  virtual void initialize_windows_arch (bool attaching) = 0;
>    /* Cleanup arch-specific data after inferior exit.  */
>    virtual void cleanup_windows_arch () = 0;
> 
> @@ -198,6 +195,26 @@ struct windows_nat_target : public inf_child_target
>    /* Set the stepping bit in the thread context.  */
>    virtual void thread_context_step (windows_thread_info *th) = 0;
> 
> +  /* Fetches register number R from the given windows_thread_info,
> +     and supplies its value to the given regcache.
> +
> +     This function assumes that R is non-negative.  A failed assertion
> +     is raised if that is not true.
> +
> +     This function assumes that TH->RELOAD_CONTEXT is not set, meaning
> +     that the windows_thread_info has an up-to-date context.  A failed
> +     assertion is raised if that assumption is violated.  */
> +  virtual void fetch_one_register (struct regcache *regcache,
> +				   windows_thread_info *th, int r) = 0;
> +
> +  /* Collect the register number R from the given regcache, and store
> +     its value into the corresponding area of the given thread's context.
> +
> +     This function assumes that R is non-negative.  A failed assertion
> +     assertion is raised if that is not true.  */
> +  virtual void store_one_register (const struct regcache *regcache,
> +				   windows_thread_info *th, int r) = 0;
> +
>  private:
> 
>    windows_thread_info *add_thread (ptid_t ptid, HANDLE h, void *tlb, diff --
> git a/gdb/x86-windows-nat.c b/gdb/x86-windows-nat.c index
> 88844e0675a..96dd61b39fb 100644
> --- a/gdb/x86-windows-nat.c
> +++ b/gdb/x86-windows-nat.c
> @@ -23,6 +23,9 @@
> 
>  #include "x86-nat.h"
> 
> +#include "i386-tdep.h"
> +#include "i387-tdep.h"
> +
>  using namespace windows_nat;
> 
>  /* If we're not using the old Cygwin header file set, define the @@ -53,17
> +56,26 @@ check (BOOL ok, const char *file, int line)  struct
> x86_windows_per_inferior : public windows_per_inferior  {
>    uintptr_t dr[8] {};
> +
> +  /* The function to use in order to determine whether a register is
> +     a segment register or not.  */
> +  segment_register_p_ftype *segment_register_p = nullptr;
>  };
> 
>  struct x86_windows_nat_target final : public
> x86_nat_target<windows_nat_target>
>  {
> -  void initialize_windows_arch () override;
> +  void initialize_windows_arch (bool attaching) override;
>    void cleanup_windows_arch () override;
> 
>    void fill_thread_context (windows_thread_info *th) override;
> 
>    void thread_context_continue (windows_thread_info *th, int killed)
> override;
>    void thread_context_step (windows_thread_info *th) override;
> +
> +  void fetch_one_register (struct regcache *regcache,
> +			   windows_thread_info *th, int r) override;
> +  void store_one_register (const struct regcache *regcache,
> +			   windows_thread_info *th, int r) override;
>  };
> 
>  /* The current process.  */
> @@ -72,9 +84,25 @@ static x86_windows_per_inferior
> x86_windows_process;
>  /* See windows-nat.h.  */
> 
>  void
> -x86_windows_nat_target::initialize_windows_arch ()
> +x86_windows_nat_target::initialize_windows_arch (bool attaching)
>  {
>    memset (x86_windows_process.dr, 0, sizeof (x86_windows_process.dr));
> +
> +#ifdef __x86_64__
> +  x86_windows_process.ignore_first_breakpoint
> +    = !attaching && x86_windows_process.wow64_process;
> +
> +  if (!x86_windows_process.wow64_process)
> +    {
> +      x86_windows_process.mappings  = amd64_mappings;
> +      x86_windows_process.segment_register_p =
> amd64_windows_segment_register_p;
> +    }
> +  else
> +#endif
> +    {
> +      x86_windows_process.mappings  = i386_mappings;
> +      x86_windows_process.segment_register_p =
> i386_windows_segment_register_p;
> +    }
>  }
> +    {
> +      x86_windows_process.mappings  = i386_mappings;
> +      x86_windows_process.segment_register_p =
> i386_windows_segment_register_p;
> +    }
> 
>  /* See windows-nat.h.  */
> @@ -158,6 +186,111 @@ x86_windows_nat_target::thread_context_step
> (windows_thread_info *th)
>      });
>  }
> 
> +/* See windows-nat.h.  */
> +
> +void
> +x86_windows_nat_target::fetch_one_register (struct regcache *regcache,
> +					    windows_thread_info *th, int r) {
> +  gdb_assert (r >= 0);
> +  gdb_assert (!th->reload_context);
> +  char *context_ptr = x86_windows_process.with_context (th, [] (auto
> *context)
> +    {
> +      return (char *) context;
> +    });
> +
> +  char *context_offset = context_ptr + x86_windows_process.mappings[r];
> + struct gdbarch *gdbarch = regcache->arch ();  i386_gdbarch_tdep *tdep
> + = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
> +
> +  gdb_assert (!gdbarch_read_pc_p (gdbarch));  gdb_assert
> + (gdbarch_pc_regnum (gdbarch) >= 0);  gdb_assert (!gdbarch_write_pc_p
> + (gdbarch));
> +
> +  /* GDB treats some registers as 32-bit, where they are in fact only
> +     16 bits long.  These cases must be handled specially to avoid
> +     reading extraneous bits from the context.  */
> +  if (r == I387_FISEG_REGNUM (tdep)
> +      || x86_windows_process.segment_register_p (r))
> +    {
> +      gdb_byte bytes[4] = {};
> +      memcpy (bytes, context_offset, 2);
> +      regcache->raw_supply (r, bytes);
> +    }
> +  else if (r == I387_FOP_REGNUM (tdep))
> +    {
> +      long l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
> +      regcache->raw_supply (r, &l);
> +    }
> +  else
> +    {
> +      if (th->stopped_at_software_breakpoint
> +	  && !th->pc_adjusted
> +	  && r == gdbarch_pc_regnum (gdbarch))
> +	{
> +	  int size = register_size (gdbarch, r);
> +	  if (size == 4)
> +	    {
> +	      uint32_t value;
> +	      memcpy (&value, context_offset, size);
> +	      value -= gdbarch_decr_pc_after_break (gdbarch);
> +	      memcpy (context_offset, &value, size);
> +	    }
> +	  else
> +	    {
> +	      gdb_assert (size == 8);
> +	      uint64_t value;
> +	      memcpy (&value, context_offset, size);
> +	      value -= gdbarch_decr_pc_after_break (gdbarch);
> +	      memcpy (context_offset, &value, size);
> +	    }
> +	  /* Make sure we only rewrite the PC a single time.  */
> +	  th->pc_adjusted = true;
> +	}
> +      regcache->raw_supply (r, context_offset);
> +    }
> +}
> +
> +/* See windows-nat.h.  */
> +
> +void
> +x86_windows_nat_target::store_one_register (const struct regcache
> *regcache,
> +					    windows_thread_info *th, int r) {
> +  gdb_assert (r >= 0);
> +
> +  char *context_ptr = x86_windows_process.with_context (th, [] (auto
> *context)
> +    {
> +      return (char *) context;
> +    });
> +
> +  struct gdbarch *gdbarch = regcache->arch ();  i386_gdbarch_tdep *tdep
> + = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
> +
> +  /* GDB treats some registers as 32-bit, where they are in fact only
> +     16 bits long.  These cases must be handled specially to avoid
> +     overwriting other registers in the context.  */
> +  if (r == I387_FISEG_REGNUM (tdep)
> +      || x86_windows_process.segment_register_p (r))
> +    {
> +      gdb_byte bytes[4];
> +      regcache->raw_collect (r, bytes);
> +      memcpy (context_ptr + x86_windows_process.mappings[r], bytes, 2);
> +    }
> +  else if (r == I387_FOP_REGNUM (tdep))
> +    {
> +      gdb_byte bytes[4];
> +      regcache->raw_collect (r, bytes);
> +      /* The value of FOP occupies the top two bytes in the context,
> +	 so write the two low-order bytes from the cache into the
> +	 appropriate spot.  */
> +      memcpy (context_ptr + x86_windows_process.mappings[r] + 2, bytes,
> 2);
> +    }
> +  else
> +    regcache->raw_collect (r, context_ptr +
> +x86_windows_process.mappings[r]); }
> +
>  /* Hardware watchpoint support, adapted from go32-nat.c code.  */
> 
>  /* Pass the address ADDR to the inferior in the I'th debug register.
> --
> 2.52.0
> 

This looks good to me, but I don't have much experience on windows. 
Due to that and because this patch also contains files which are not x86 specific,
I'll add my reviewed-by tag (and not the approved-by tag).

Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>

Christina
Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928