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

[Hannes Domani <ssbssa@yahoo.de>]

 I'm not sure what happened, but I replied earlier to 8/8 already, but
with the contents to my reply for 7/8...


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

> Hi Hannes,
> 
> Thank you for this patch and your work on this.
> 
> Please find my comments on top of Stephan's feedback.
> 
> > -----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
> > Cc: Joos, Christina <christina.joos@intel.com>; Tom Tromey
> > <tom@tromey.com>
> > Subject: RE: [PATCH v3 8/8] Windows gdb: Implement AVX-512 register support
> >
> > Hi Hannes,
> >
> > please see some inline feedback below.
> > Some of the feedback provided for
> >
> >  [PATCH v3 7/8] Windows gdb: Implement AVX register support
> >
> > applies here as well.
> >
> > 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 8/8] Windows gdb: Implement AVX-512 register
> > > support
> > >
> > > This adds support for the Intel AVX-512 registers on Windows.
> > > It enables accessing registers $ymm0 - $ymm31, $zmm0 - $zmm31, and
> > > $k0 - $k7 where they are available.
> 
> Suggestion:
> For the linux side we have this commit to enable AMX-512:
> " Add AVX512 registers support to GDB and GDBserver."
> 
> It includes a description which registers are added, extended etc:
> 
> " Intel(R) AVX-512 is an extension to AVX to support 512-bit wide
> SIMD registers in 64-bit mode (XMM0-XMM31, YMM0-YMM31, ZMM0-ZMM31).
> The number of available registers in 32-bit mode is still 8
> (XMM0-7, YMM0-7, ZMM0-7).  The lower 256-bits of the ZMM registers
> are aliased to the respective 256-bit YMM registers.  The lower
> 128-bits are aliased to the respective 128-bit XMM registers.
> 
> There are also 8 new, dedicated mask registers (K0-K7) in both 32-bit
> mode and 64-bit mode."
> 
> Maybe you could reference it, something like
> "similar to commit xyz for linux this patch adds [...] for 32 and 64-bit mode in windows." ?
> 
> Then you have all the details described, if one would like to look it up to understand your
> patch in detail. But you don't have to repeat it in your own commit message.
> 
> What do you think?

Sounds good.


> > > After this patch gdb.arch/i386-avx512.exp passes on windows.
> > > ---
> > > v3:
> > >  - merged gdb+gdbserver parts, and split again AVX/AVX-512 parts
> > > ---
> > >  gdb/NEWS                    |  2 ++
> > >  gdb/nat/windows-nat.c      |  2 +-
> > >  gdb/x86-windows-nat.c      | 35 +++++++++++++++++++++++++++
> > >  gdbserver/win32-i386-low.cc | 48
> > > ++++++++++++++++++++++++++++++++++++-
> > >  4 files changed, 85 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > index f7effc822e9..d3db6dd167e 100644
> > > --- a/gdb/NEWS
> > > +++ b/gdb/NEWS
> > > @@ -120,6 +120,8 @@
> > >
> > >  * Support for Intel AVX registers on Windows.
> > >    Support displaying and modifying Intel AVX registers $ymm0 - $ymm31.
> >
> > Following the previous patch, this should update again to registers $ymm0 -
> > $ymm31?

Right.


> > > +  Support displaying and modifying Intel AVX-512 registers $zmm0 -
> > > + $zmm31  and $k0 - $k7.
> > >
> > >  * Configure changes
> > >
> > > diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c index
> > > c9a21d7c41f..30d49c07332 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_AVX_MASK);
> > > +            & X86_XSTATE_AVX_AVX512_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
> > > 1cefe6171be..425e343deca 100644
> > > --- a/gdb/x86-windows-nat.c
> > > +++ b/gdb/x86-windows-nat.c
> > > @@ -291,6 +291,41 @@ get_context_reg_ptr (Context *context, int r,
> > > i386_gdbarch_tdep *tdep)
> > >      (context, X86_XSTATE_AVX_ID, NULL);
> > >        context_offset += 16 * (r - I387_YMM0H_REGNUM (tdep));
> > >      }
> > > +  else if (I387_ZMM0H_REGNUM (tdep) > 0 && r >=
> > I387_ZMM0H_REGNUM
> > > (tdep)
> > > +      && r < I387_ZMM16H_REGNUM (tdep) && r <
> > > I387_ZMMENDH_REGNUM (tdep))
> > > +    {
> >
> > I basically have the same concern as in patch 7/8 regarding the guards on
> > gdbserver side but missing here.
> >
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_H_ID, NULL);
> > > +      context_offset += 32 * (r - I387_ZMM0H_REGNUM (tdep));
> > > +    }
> > > +  else if (I387_ZMM0H_REGNUM (tdep) > 0 && r >=
> > > I387_ZMM16H_REGNUM (tdep)
> > > +      && r < I387_ZMMENDH_REGNUM (tdep))
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_ID, NULL);
> > > +      context_offset += 32 + 64 * (r - I387_ZMM16H_REGNUM (tdep));
> > > +    }
> > > +  else if (I387_K0_REGNUM (tdep) > 0 && r >= I387_K0_REGNUM (tdep)
> > > +      && r < I387_KEND_REGNUM (tdep))
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_K_ID, NULL);
> > > +      context_offset += 8 * (r - I387_K0_REGNUM (tdep));
> > > +    }
> > > +  else if (I387_YMM16H_REGNUM (tdep) > 0 && r >=
> > > I387_YMM16H_REGNUM (tdep)
> > > +      && r < I387_YMMH_AVX512_END_REGNUM (tdep))
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_ID, NULL);
> > > +      context_offset += 16 + 64 * (r - I387_YMM16H_REGNUM (tdep));
> > > +    }
> > > +  else if (I387_XMM16_REGNUM (tdep) > 0 && r >= I387_XMM16_REGNUM
> > > (tdep)
> > > +      && r < I387_XMM_AVX512_END_REGNUM (tdep))
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_ID, NULL);
> > > +      context_offset += 64 * (r - I387_XMM16_REGNUM (tdep));
> > > +    }
> > >    else
> > >      gdb_assert_not_reached ("invalid register number %d", r);
> > >
> > > diff --git a/gdbserver/win32-i386-low.cc b/gdbserver/win32-i386-low.cc
> > > index a7e83c0239c..6911587f977 100644
> > > --- a/gdbserver/win32-i386-low.cc
> > > +++ b/gdbserver/win32-i386-low.cc
> > > @@ -529,8 +529,11 @@ get_context_reg_ptr (Context *context, int r,
> > > const target_desc *tdesc)
> > >      }
> > >
> > >    bool amd64 = register_size (tdesc, 0) == 8;
> > > -  int ymm0h_regnum;
> > > +  int ymm0h_regnum, zmm0h_regnum, k0_regnum;  int xmm16_regnum,
> > > + ymm16h_regnum, zmm16h_regnum;
> > >    const int num_xmm_registers = amd64 ? 16 : 8;
> > > +  const int num_zmm_high_registers = amd64 ? 16 : 0;  const int
> > > + num_avx512_k_registers = 8;
> > >
> > >    char *context_offset;
> > >    if (r < mappings_count)
> > > @@ -543,6 +546,49 @@ get_context_reg_ptr (Context *context, int r,
> > > const target_desc *tdesc)
> > >      (context, X86_XSTATE_AVX_ID, NULL);
> > >        context_offset += 16 * (r - ymm0h_regnum);
> > >      }
> > > +  else if ((xstate_features & X86_XSTATE_ZMM_H) != 0
> > > +      && r >= (zmm0h_regnum = find_regno (tdesc, "zmm0h"))
> > > +      && r < zmm0h_regnum + num_xmm_registers)
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_H_ID, NULL);
> > > +      context_offset += 32 * (r - zmm0h_regnum);
> > > +    }
> 
> Similar comment  to the previous patch for nullptr here and below.
> 
> > > +  else if ((xstate_features & X86_XSTATE_ZMM) != 0
> > > +      && num_zmm_high_registers != 0
> > > +      && r >= (zmm16h_regnum = find_regno (tdesc, "zmm16h"))
> > > +      && r < zmm16h_regnum + num_zmm_high_registers)
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_ID, NULL);
> > > +      context_offset += 32 + 64 * (r - zmm16h_regnum);
> > > +    }
> > > +  else if ((xstate_features & X86_XSTATE_K) != 0
> > > +      && r >= (k0_regnum = find_regno (tdesc, "k0"))
> > > +      && r < k0_regnum + num_avx512_k_registers)
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_K_ID, NULL);
> > > +      context_offset += 8 * (r - k0_regnum);
> > > +    }
> > > +  else if ((xstate_features & X86_XSTATE_ZMM) != 0
> > > +      && num_zmm_high_registers != 0
> > > +      && r >= (ymm16h_regnum = find_regno (tdesc, "ymm16h"))
> > > +      && r < ymm16h_regnum + num_zmm_high_registers)
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_ID, NULL);
> > > +      context_offset += 16 + 64 * (r - ymm16h_regnum);
> > > +    }
> > > +  else if ((xstate_features & X86_XSTATE_ZMM) != 0
> > > +      && num_zmm_high_registers != 0
> > > +      && r >= (xmm16_regnum = find_regno (tdesc, "xmm16"))
> > > +      && r < xmm16_regnum + num_zmm_high_registers)
> > > +    {
> > > +      context_offset = (char *) locate_xstate_feature
> > > +    (context, X86_XSTATE_ZMM_ID, NULL);
> > > +      context_offset += 64 * (r - xmm16_regnum);
> > > +    }
> > >    else
> > >      gdb_assert_not_reached ("invalid register number %d", r);
> > >
> >
> > These are a lot of look-ups on gdbserver side.  Each "find_regno" iterates over
> > the complete set of registers and does string comparison.  It would be nice to
> > implement this like the GDB side and cache the register numbers.
> >
> > I think it is not mandatory to implement this but would be nice to have.
> 
> I have a similar comment on this, but for the offsets:
> 
> Those offsets are similar in gdbserver/i387-fp.cc and gdb/i387-tdep.c, but we
> cannot use/share the complete logic for windows gdb & gdbserver I believe.
> 
> However, the offsets inside one feature (I don't mean the offsets between two
> consecutive features) should be the same for any OS and independent of AMD or Intel.
> I believe it would be nice if we could at least share the offset calculations at some point
> maybe in gdbsupport/* sometime in future...
> But this is nothing that you must do as part of this patch of course.
> 
> For this patch:
> I don't think this is strictly necessary, but we could make this code a bit more generic inside each individual file on gdb & gdbserver side.
> For example, we could introduce some static helpers or similar, that are called in each else if.
> What do you think?
> 
> Christina

I was also thinking that these look-ups are not ideal.
As I understand it, the offsets for each feature are not fixed values on
windows, but they should be the same for all threads on a system.
In that case, maybe we could calculate these offsets for all registers
once, put them in a vector, and use that in get_context_reg_ptr?
I'm also assuming that the register numbers always stay the same.
I'm just not sure at which point the offset calculation should be done.
Any suggestions?


Hannes