RE: [PATCH v4 7/9] Windows gdb: Implement AVX register support

["Rohr, Stephan" <stephan.rohr@intel.com>]

Hi Hannes,

some minor findings below.

With those fixed.

Reviewed-By: Rohr, Stephan <stephan.rohr@intel.com>

Thanks
Stephan

> -----Original Message-----
> From: Hannes Domani <ssbssa@yahoo.de>
> Sent: Wednesday, 30 September 2026 17:12
> To: gdb-patches@sourceware.org
> Subject: [PATCH v4 7/9] Windows gdb: Implement AVX register support
> 
> This adds support for the Intel AVX registers on Windows.
> It enables accessing registers $ymm0 - $ymm15 where they are available.
> 
> After this patch gdb.arch/i386-avx.exp passes on windows.
> ---
> v3:
>   - merged gdb+gdbserver parts, and split again AVX/AVX-512 parts
> v4:
>   - fixed $ymm15 in NEWS
>   - Add common function windows_thread_info::zero_xstate_features,
>     and fix maximum supported feature in for-loop
>   - Add check for enabled xstate_features in get_context_reg_ptr of
>     x86-windows-nat.c
>   - Move amd64 variable in get_context_reg_ptr of win32-i386-low.cc
>   - Changed all NULL to nullptr
>   - Don't remove 'r == 0' from child_store_inferior_registers
> ---
>  gdb/NEWS                    |  3 +++
>  gdb/nat/windows-nat.c       | 36 +++++++++++++++++++++++++-
>  gdb/nat/windows-nat.h       |  6 +++++
>  gdb/x86-windows-nat.c       | 51
> ++++++++++++++++++++++++++++++++++---
>  gdbserver/win32-i386-low.cc | 47 +++++++++++++++++++++++++++++-----
>  gdbserver/win32-low.cc      | 15 +++++++----
>  6 files changed, 143 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 5d0c4745653..da4bc811c3a 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -194,6 +194,9 @@ info locals
>    intent to remove it in a future release.
>    The s390 64-bit target (s390x-*) remains supported.
> 
> +* Support for Intel AVX registers on Windows.
> +  Support displaying and modifying Intel AVX registers $ymm0 - $ymm15.
> +
>  * Configure changes
> 
>  ** --with-babeltrace has been removed.  The babeltrace library was
> diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
> index 58be5368a79..cd3c827a5d5 100644
> --- a/gdb/nat/windows-nat.c
> +++ b/gdb/nat/windows-nat.c
> @@ -271,6 +271,40 @@ windows_thread_info::xfer_siginfo (gdb_byte
> *readbuf,
>    return true;
>  }
> 
> +#if defined __i386__ || defined __x86_64__
> +/* See nat/windows-nat.h.  */
> +
> +void
> +windows_thread_info::zero_xstate_features ()
> +{
> +  if (xstate_features == 0)
> +    return;
> +
> +  proc->with_context (this, [] (auto *context)
> +    {
> +      DWORD64 features = 0;
> +      if (!get_xstate_features_mask (context, &features))
> +	return;
> +
> +      DWORD64 zeroed_features = xstate_features & ~features;
> +      if (zeroed_features == 0)
> +	return;
> +
> +      for (int f = X86_XSTATE_AVX_ID; f <= X86_XSTATE_AVX_ID; f++)

Start == End.

> +	{
> +	  DWORD64 flag = 1ULL << f;
> +	  if ((zeroed_features & flag) != 0)
> +	    {
> +	      DWORD size = 0;
> +	      void *loc = locate_xstate_feature (context, f, &size);
> +	      if (loc != nullptr && size > 0)
> +		memset (loc, 0, size);
> +	    }
> +	}
> +    });
> +}
> +#endif
> +
>  /* Try to determine the executable filename.
> 
>     EXE_NAME_RET is a pointer to a buffer whose size is EXE_NAME_MAX_LEN.
> @@ -1339,7 +1373,7 @@ initialize_loadable ()
>      {
>        /* Available XState features masked with implemented features.  */
>        xstate_features = (GetEnabledXStateFeatures ()
> -			 & X86_XSTATE_SSE_MASK);
> +			 & X86_XSTATE_AVX_MASK);
>        /* The extended XState functions are only needed if the available
>  	 features exceed SSE.  */
>        if ((xstate_features & ~X86_XSTATE_SSE_MASK) == 0)
> diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
> index a7d4af84e13..4cd75cb6a0e 100644
> --- a/gdb/nat/windows-nat.h
> +++ b/gdb/nat/windows-nat.h
> @@ -90,6 +90,12 @@ struct windows_thread_info
>  		     ULONGEST offset, ULONGEST len,
>  		     ULONGEST *xfered_len);
> 
> +#if defined __i386__ || defined __x86_64__
> +  /* Zero all XState feature areas that were not provided by
> +     GetThreadContext.  */
> +  void zero_xstate_features ();
> +#endif
> +
>    /* The process this thread belongs to.  */
>    windows_process_info *const proc;
> 
> diff --git a/gdb/x86-windows-nat.c b/gdb/x86-windows-nat.c
> index 3af5ef4dae0..1e44a45171c 100644
> --- a/gdb/x86-windows-nat.c
> +++ b/gdb/x86-windows-nat.c
> @@ -27,6 +27,9 @@
> 
>  #include "i386-tdep.h"
>  #include "i387-tdep.h"
> +#ifdef __x86_64__
> +#include "amd64-tdep.h"
> +#endif
> 
>  using namespace windows_nat;
> 
> @@ -70,6 +73,8 @@ struct x86_windows_nat_target final : public
> x86_nat_target<windows_nat_target>
>  			   windows_thread_info *th, int r) override;
> 
>    bool is_sw_breakpoint (const EXCEPTION_RECORD *er) const override;
> +
> +  const struct target_desc *read_description () override;
>  };
> 
>  /* The current process.  */
> @@ -109,7 +114,14 @@ x86_windows_per_inferior::fill_thread_context
> (windows_thread_info *th)
>        if (context->ContextFlags == 0)
>  	{
>  	  context->ContextFlags = WindowsContext<decltype(context)>::all;
> +	  if (xstate_features != 0)
> +	    {
> +	      context->ContextFlags |= CONTEXT_XSTATE_FLAG;
> +	      set_xstate_features_mask (context, xstate_features);
> +	    }
>  	  CHECK (get_thread_context (th->h, context));
> +
> +	  th->zero_xstate_features ();
>  	}
>      });
>  }
> @@ -198,6 +210,14 @@
> x86_windows_nat_target::thread_context_continue (windows_thread_info
> *th,
>  	  if (GetExitCodeThread (th->h, &ec)
>  	      && ec == STILL_ACTIVE)
>  	    {
> +	      DWORD debug_registers =
> WindowsContext<decltype(context)>::debug;
> +	      if (xstate_features != 0
> +		  && (context->ContextFlags & ~debug_registers) != 0)
> +		{
> +		  context->ContextFlags |= CONTEXT_XSTATE_FLAG;
> +		  set_xstate_features_mask (context, xstate_features);
> +		}
> +
>  	      BOOL status = set_thread_context (th->h, context);
> 
>  	      if (!killed)
> @@ -227,7 +247,7 @@ x86_windows_nat_target::thread_context_step
> (windows_thread_info *th,
> 
>  template<typename Context>
>  static char *
> -get_context_reg_ptr (Context *context, int r)
> +get_context_reg_ptr (Context *context, int r, i386_gdbarch_tdep *tdep)
>  {
>    const int *mappings;
>    int mappings_count;
> @@ -247,6 +267,14 @@ get_context_reg_ptr (Context *context, int r)
>    char *context_offset;
>    if (r < mappings_count)
>      context_offset = (char *) context + mappings[r];
> +  else if ((xstate_features & X86_XSTATE_AVX) != 0
> +	   && I387_YMM0H_REGNUM (tdep) > 0 && r >=
> I387_YMM0H_REGNUM (tdep)
> +	   && r < I387_YMMENDH_REGNUM (tdep))
> +    {
> +      context_offset = (char *) locate_xstate_feature
> +	(context, X86_XSTATE_AVX_ID, nullptr);
> +      context_offset += 16 * (r - I387_YMM0H_REGNUM (tdep));
> +    }
>    else
>      gdb_assert_not_reached ("invalid register number %d", r);
> 
> @@ -267,7 +295,7 @@ x86_windows_nat_target::fetch_one_register (struct
> regcache *regcache,
>    char *context_offset
>      = x86_windows_process.with_context (th, [&] (auto *context)
>      {
> -      return get_context_reg_ptr (context, r);
> +      return get_context_reg_ptr (context, r, tdep);
>      });
> 
>    gdb_assert (!gdbarch_read_pc_p (gdbarch));
> @@ -333,7 +361,7 @@ x86_windows_nat_target::store_one_register (const
> struct regcache *regcache,
>      = x86_windows_process.with_context (th, [&] (auto *context)
>      {
>        gdb_assert (context->ContextFlags != 0);
> -      return get_context_reg_ptr (context, r);
> +      return get_context_reg_ptr (context, r, tdep);
>      });
> 
>    /* GDB treats some registers as 32-bit, where they are in fact only
> @@ -368,6 +396,23 @@ x86_windows_nat_target::is_sw_breakpoint (const
> EXCEPTION_RECORD *er) const
>  	  || er->ExceptionCode == STATUS_WX86_BREAKPOINT);
>  }
> 

Add

/* See target.h.  */

> +const struct target_desc *
> +x86_windows_nat_target::read_description ()
> +{
> +  if (inferior_ptid == null_ptid)
> +    return this->beneath ()->read_description ();
> +
> +  if (xstate_features == 0)
> +    return nullptr;
> +
> +#ifdef __x86_64__
> +  if (!x86_windows_process.wow64_process)
> +    return amd64_target_description (xstate_features, false);
> +  else
> +#endif
> +    return i386_target_description (xstate_features, false);
> +}
> +
>  /* Hardware watchpoint support, adapted from go32-nat.c code.  */
> 
>  /* Pass the address ADDR to the inferior in the I'th debug register.
> diff --git a/gdbserver/win32-i386-low.cc b/gdbserver/win32-i386-low.cc
> index b77f6adc6ed..1a45b15e0cf 100644
> --- a/gdbserver/win32-i386-low.cc
> +++ b/gdbserver/win32-i386-low.cc
> @@ -253,6 +253,11 @@ i386_get_thread_context (windows_thread_info
> *th)
>  			       | WindowsContext<decltype(context)>::floating
>  			       | WindowsContext<decltype(context)>::debug
>  			       | extended_registers);
> +      if (xstate_features != 0)
> +	{
> +	  context->ContextFlags |= CONTEXT_XSTATE_FLAG;
> +	  set_xstate_features_mask (context, xstate_features);
> +	}
> 
>        BOOL ret = get_thread_context (th->h, context);
>        if (!ret)
> @@ -267,6 +272,8 @@ i386_get_thread_context (windows_thread_info *th)
> 
>  	  error (_("GetThreadContext failure %ld\n"), (long) e);
>  	}
> +
> +      th->zero_xstate_features ();
>      });
>  }
> 
> @@ -292,6 +299,17 @@ i386_prepare_to_resume (windows_thread_info
> *th)
> 
>        th->debug_registers_changed = false;
>      }
> +
> +  windows_process.with_context (th, [&] (auto *context)
> +    {
> +      DWORD debug_registers = WindowsContext<decltype(context)>::debug;
> +      if (xstate_features != 0
> +	  && (context->ContextFlags & ~debug_registers) != 0)
> +	{
> +	  context->ContextFlags |= CONTEXT_XSTATE_FLAG;
> +	  set_xstate_features_mask (context, xstate_features);
> +	}
> +    });
>  }
> 
>  static void
> @@ -477,26 +495,40 @@ is_segment_register (int r)
> 
>  template<typename Context>
>  static char *
> -get_context_reg_ptr (Context *context, int r)
> +get_context_reg_ptr (Context *context, int r, const target_desc *tdesc)
>  {
>    const int *mappings;
>    int mappings_count;
> +  bool amd64;
>  #ifdef __x86_64__
>    if (!windows_process.wow64_process)
>      {
>        mappings = amd64_mappings;
>        mappings_count = sizeof (amd64_mappings) / sizeof
> (amd64_mappings[0]);
> +      amd64 = true;
>      }
>    else
>  #endif
>      {
>        mappings = i386_mappings;
>        mappings_count = sizeof (i386_mappings) / sizeof (i386_mappings[0]);
> +      amd64 = false;
>      }
> 
> +  int ymm0h_regnum;
> +  const int num_xmm_registers = amd64 ? 16 : 8;
> +
>    char *context_offset;
>    if (r < mappings_count)
>      context_offset = (char *) context + mappings[r];
> +  else if ((xstate_features & X86_XSTATE_AVX) != 0
> +	   && r >= (ymm0h_regnum = find_regno (tdesc, "ymm0h"))
> +	   && r < ymm0h_regnum + num_xmm_registers)
> +    {
> +      context_offset = (char *) locate_xstate_feature
> +	(context, X86_XSTATE_AVX_ID, nullptr);

We don't check "context_offset" for nulltpr before adding the offsets. We're
checking for nullptr and size explicitly in "zero_xstate_features".  So, I was
wondering if we should add checks here, too.  IIUC, it should not be possible
to have "locate_xstate_feature () == nullptr" at this point.  Maybe add an
assert for consistency?

> +      context_offset += 16 * (r - ymm0h_regnum);
> +    }
>    else
>      gdb_assert_not_reached ("invalid register number %d", r);
> 
> @@ -510,7 +542,7 @@ i386_fetch_inferior_register (struct regcache
> *regcache,
>  {
>    char *context_offset = windows_process.with_context (th, [&] (auto
> *context)
>      {
> -      return get_context_reg_ptr (context, r);
> +      return get_context_reg_ptr (context, r, regcache->tdesc);
>      });
> 
>    /* GDB treats some registers as 32-bit, where they are in fact only
> @@ -538,7 +570,7 @@ i386_store_inferior_register (struct regcache
> *regcache,
>  {
>    char *context_offset = windows_process.with_context (th, [&] (auto
> *context)
>      {
> -      return get_context_reg_ptr (context, r);
> +      return get_context_reg_ptr (context, r, regcache->tdesc);
>      });
> 
>    /* GDB treats some registers as 32-bit, where they are in fact only
> @@ -571,14 +603,17 @@ i386_arch_setup (void)
>  {
>   target_desc_up tdesc;
> 
> +  DWORD64 xcr0 = xstate_features;
> +  if (xcr0 == 0)
> +    xcr0 = X86_XSTATE_SSE_MASK;
> +
>  #ifdef __x86_64__
> -  tdesc = amd64_create_target_description (X86_XSTATE_SSE_MASK, false,
> -					   false, false);
> +  tdesc = amd64_create_target_description (xcr0, false, false, false);
>    init_target_desc (tdesc.get (), amd64_expedite_regs, WINDOWS_OSABI);
>    win32_tdesc = std::move (tdesc);
>  #endif
> 
> -  tdesc = i386_create_target_description (X86_XSTATE_SSE_MASK, false,
> false);
> +  tdesc = i386_create_target_description (xcr0, false, false);
>    init_target_desc (tdesc.get (), i386_expedite_regs, WINDOWS_OSABI);
>  #ifdef __x86_64__
>    wow64_win32_tdesc = std::move (tdesc);
> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
> index 1103a802db7..3e80405f651 100644
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -33,6 +33,7 @@
>  #include <process.h>
>  #include "gdbsupport/gdb_tilde_expand.h"
>  #include "gdbsupport/common-inferior.h"
> +#include "tdesc.h"
> 
>  using namespace windows_nat;
> 
> @@ -427,8 +428,9 @@ child_fetch_inferior_registers (struct regcache
> *regcache, int r)
>    int regno;
>    windows_thread_info *th = windows_process.find_thread (current_thread-
> >id);
>    win32_require_context (th);
> -  if (r == -1 || r > NUM_REGS)
> -    child_fetch_inferior_registers (regcache, NUM_REGS);

With r != -1 and r > regcache->tdesc->reg_defs.size (), this may fall through into
"get_context_reg_ptr" and eventually into the gdb_assert_not_reached_part.

Maybe add another check, e.g.,

    int num_regs = regcache->tdesc->reg_defs.size ();
    if (r == -1 || r > num_regs)
      r = num_regs;

Same for child_store_inferior_registers.

> +  if (r == -1)
> +    child_fetch_inferior_registers (regcache,
> +				    regcache->tdesc->reg_defs.size ());
>    else
>      for (regno = 0; regno < r; regno++)
>        (*the_low_target.fetch_inferior_register) (regcache, th, regno);
> @@ -442,8 +444,9 @@ child_store_inferior_registers (struct regcache
> *regcache, int r)
>    int regno;
>    windows_thread_info *th = windows_process.find_thread (current_thread-
> >id);
>    win32_require_context (th);
> -  if (r == -1 || r == 0 || r > NUM_REGS)
> -    child_store_inferior_registers (regcache, NUM_REGS);
> +  if (r == -1 || r == 0)
> +    child_store_inferior_registers (regcache,
> +				    regcache->tdesc->reg_defs.size ());
>    else
>      for (regno = 0; regno < r; regno++)
>        (*the_low_target.store_inferior_register) (regcache, th, regno);
> @@ -1347,7 +1350,9 @@ void
>  initialize_low (void)
>  {
>    set_target_ops (&the_win32_target);
> -  the_low_target.arch_setup ();
> 
>    initialize_loadable ();
> +  /* Has to be done after initialize_loadable, because it uses the xstate
> +     functions if available.  */
> +  the_low_target.arch_setup ();
>  }
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.