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

["Joos, Christina" <christina.joos@intel.com>]

> -----Original Message-----
> From: Hannes Domani <ssbssa@yahoo.de>
> Sent: Donnerstag, 10. September 2026 16:39
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org;
> Joos, Christina <christina.joos@intel.com>
> Cc: Tom Tromey <tom@tromey.com>
> Subject: Re: [PATCH v3 8/8] Windows gdb: Implement AVX-512 register
> support
> 
>  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.

Ok, any change which is not related to the AVX patch should be in a separate
cleanup patch.

Thanks,
Christina
________________________________________
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.