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 18:54
> 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
> 
>  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.

I don't understand which offsets you mean here. 
The ones you hardcode (1) or the context_offset returned by locate_xstate_feature (2)?
My feedback in this email is only for the hardcoded ones.

For (2) I replied in a different email thread:
https://sourceware.org/pipermail/gdb-patches/2026-September/230194.html

> 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?

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.