Re: [PATCH v3 8/8] Windows gdb: Implement AVX-512 register support

[Hannes Domani <ssbssa@yahoo.de>]

 Am Donnerstag, 10. September 2026 um 13:33:20 MESZ hat Joos, Christina <christina.joos@intel.com> Folgendes geschrieben:

> Hi Hannes,
> 
> I saw that Stephan already reviewed this (thanks!).
> I added my remarks on top, see below.
> 
> > -----Original Message-----
> > From: Rohr, Stephan <stephan.rohr@intel.com>
> > Sent: Dienstag, 8. September 2026 15:05
> > To: Hannes Domani <ssbssa@yahoo.de>; gdb-patches@sourceware.org; gdb-
> > patches@sourceware.org
> > Cc: Joos, Christina <christina.joos@intel.com>; Tom Tromey
> > <tom@tromey.com>
> > Subject: RE: [PATCH v3 7/8] Windows gdb: Implement AVX register support
> >
> > Hi Hannes,
> >
> > please see some feedback inlined below.
> >
> > Let me know if you have any questions.
> >
> > Thanks
> > Stephan
> >
> > > -----Original Message-----
> > > From: Hannes Domani <ssbssa@yahoo.de>
> > > Sent: Saturday, 29 August 2026 16:49
> > > To: gdb-patches@sourceware.org
> > > Subject: [PATCH v3 7/8] 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
> > > ---
> > >  gdb/NEWS                    |  3 ++
> > >  gdb/nat/windows-nat.c      |  2 +-
> > >  gdb/x86-windows-nat.c      | 67
> > > +++++++++++++++++++++++++++++++++++--
> > >  gdbserver/win32-i386-low.cc | 61 +++++++++++++++++++++++++++++----
> > >  gdbserver/win32-low.cc      | 15 ++++++---
> > >  5 files changed, 133 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > index 10c182067f9..f7effc822e9 100644
> > > --- a/gdb/NEWS
> > > +++ b/gdb/NEWS
> > > @@ -118,6 +118,9 @@
> > >    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 - $ymm31.
> > > +
> >
> > I think this should be registers $ymm0 - $ymm15 ?
> 
> Yes, I agree with Stephan's feedback here.
> The AVX state only comprises only YMM0–YMM15 for 64 bit.
> For 32-bit mode, it's YMM0- YMM7 only.
> See the docs added in commit "Add org.gnu.gdb.i386.avx."

Yes, I missed this when I split AVX/AVX-512.


> > >  * 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
> > > 8930536f3ba..c9a21d7c41f 100644
> > > --- a/gdb/nat/windows-nat.c
> > > +++ b/gdb/nat/windows-nat.c
> > > @@ -1339,7 +1339,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/x86-windows-nat.c b/gdb/x86-windows-nat.c index
> > > 3af5ef4dae0..1cefe6171be 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,31 @@ 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);
> > > +        }
> >
> > We have the same code in "i386_get_thread_context" in "win32-i386-low.cc".
> > Make a shared function in gdb/nat/windows-nat.h?
> >
> > >        CHECK (get_thread_context (th->h, context));
> > > +
> > > +      if (xstate_features != 0)
> > > +        {
> > > +          DWORD64 features = 0;
> > > +          CHECK (get_xstate_features_mask (context, &features));
> >
> > Should this be changed to sth. like
> >
> >  if (!get_xstate_features_mask (context, &features))
> >    {
> >      warning (..)
> >      return;
> >    }
> >
> > The call of "CHECK" only prints a message but doesn't error out.  If this call fails
> > we may still have features == 0.  This implies "zeroed_features ==
> > xstate_features".  With this, the loop clears all features.
> > IIUC, this would clear the AVX registers on the next call of "SetThreadContext".
> >
> > Also refer to the implementation in gdbserver/win32-i386-low.cc:
> >
> >      DWORD64 features = 0;
> >      if (xstate_features != 0
> >       && get_xstate_features_mask (context, &features))
> >     {
> >
> > I think it makes sense to unify those as the rest of the code is basically identical.
> > Put shared function into gdb/nat/windows-nat.h?  This keeps the code
> > consistent.

I will try that.


> > > +          DWORD64 zeroed_features = xstate_features & ~features;
> > > +
> > > +          for (int f = X86_XSTATE_AVX_ID; f <= X86_XSTATE_CET_U_ID; f++)
> 
> In addition to Stephan's feedback for this code area:
> Since we decided to not add CET, and this patch is even for AVX only for now only, do we need a loop at this point already?
> 
> For the follow up AVX-512 patch I think we can stop at the highest supported feature (AVX-512) in windows, can't we?

I would prefer it if we could keep it a loop, even if it's only X86_XSTATE_AVX_ID in the AVX patch.
And yes, I forgot to change it to X86_XSTATE_ZMM_ID when I removed the CET stuff.


> > > +          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);
> > > +            }
> > > +        }
> > > +        }
> > >      }
> > >      });
> > >  }
> > > @@ -198,6 +227,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 +264,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 +284,13 @@ get_context_reg_ptr (Context *context, int r)
> > >    char *context_offset;
> > >    if (r < mappings_count)
> > >      context_offset = (char *) context + mappings[r];
> > > +  else if (I387_YMM0H_REGNUM (tdep) > 0 && r >=
> > I387_YMM0H_REGNUM
> > > (tdep)
> > > +      && r < I387_YMMENDH_REGNUM (tdep))
> >
> > The implementation on gdbserver side guards against
> >
> >  xstate_features & X86_XSTATE_AVX) != 0
> >
> > I wonder if the same guard would be helpful here, too.  I understand the
> > register number is initialized to -1, so this should not fire.  I'm not sure if it is
> > possible to have $ymm0 register number > 0 w/o xstate support, e.g., if the
> > target description is read from file, see "target_find_description"?

On gdbserver side I needed the guard because there is no register number
I could check.
I didn't think it was necessary on gdb side as well, but if you prefer it
like this, I will add it.


> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_AVX_ID, NULL);
> 
> We prefer to use nullptr.

Right.


> > > +      context_offset += 16 * (r - I387_YMM0H_REGNUM (tdep));
> > > +    }
> > >    else
> > >      gdb_assert_not_reached ("invalid register number %d", r);
> > >
> > > @@ -267,7 +311,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 +377,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 +412,23 @@ x86_windows_nat_target::is_sw_breakpoint (const
> > > EXCEPTION_RECORD *er) const
> > >        || er->ExceptionCode == STATUS_WX86_BREAKPOINT);  }
> > >
> > > +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..a7e83c0239c 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,24 @@ i386_get_thread_context (windows_thread_info
> > > *th)
> > >
> > >        error (_("GetThreadContext failure %ld\n"), (long) e);
> > >      }
> > > +
> > > +      DWORD64 features = 0;
> > > +      if (xstate_features != 0
> > > +      && get_xstate_features_mask (context, &features))
> > > +    {
> > > +      DWORD64 zeroed_features = xstate_features & ~features;
> > > +      for (int f = X86_XSTATE_AVX_ID; f <= X86_XSTATE_CET_U_ID; f++)
> > > +        {
> > > +          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);
> > > +        }
> > > +        }
> > > +    }
> > >      });
> > >  }
> > >
> > > @@ -292,6 +315,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,7 +511,7 @@ 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;
> > > @@ -494,9 +528,21 @@ get_context_reg_ptr (Context *context, int r)
> > >        mappings_count = sizeof (i386_mappings) / sizeof (i386_mappings[0]);
> > >      }
> > >
> > > +  bool amd64 = register_size (tdesc, 0) == 8;
> >
> > There is already an " if (!windows_process.wow64_process)" a few lines above.
> > Wouldn't it make sense to move the "bool amd64" in the ifdef blocks and assign
> > accordingly?

Yes, I agree.


> > > +  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, NULL);
> > > +      context_offset += 16 * (r - ymm0h_regnum);
> > > +    }
> > >    else
> > >      gdb_assert_not_reached ("invalid register number %d", r);
> > >
> > > @@ -510,7 +556,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 +584,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 +617,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
> > > 7629beca213..5ccdc89a7ef 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;
> > >
> > > @@ -426,8 +427,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);
> >
> > IIUC this was the only use of the NUM_REGS define.  We can remove it.
> 
> Yes, I agree. Would you mind explaining why we don't need this check anymore, too ? :)
> I don't understand it yet, unfortunately. Was it necessary before or is this due to AVX?

I don't think the check was necessary before AVX.
But with AVX it doesn't work, because NUM_REGS is the number of fix registers,
so for any AVX register it would go to the 'else' part.


> > Same for " i386_win32_num_regs (void)" and "aarch64_win32_num_regs ()".
> > This allows removing the num_regs hook in win32_target_ops.
> 
> > > +  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); @@ -441,8 +443,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)
> > > +    child_store_inferior_registers (regcache,
> > > +                    regcache->tdesc->reg_defs.size ());
> 
> I assume removing r == 0 is not related to AVX register support and just a cleanup, right?
> Could we make it a separate cleanup patch (with reason)?
> 
> I know it seems like a super tiny nit, but it would help to understand why that kind of
> refactoring/cleanup is necessary (e.g. due to AVX or not).

I removed r == 0 because it wasn't there in child_fetch_inferior_registers
either, and I didn't like the this inconsistency.


> > >    else
> > >      for (regno = 0; regno < r; regno++)
> > >        (*the_low_target.store_inferior_register) (regcache, th,
> > > regno); @@ -1349,7 +1352,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.54.0
> 
> Christina


Thanks
Hannes