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

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

> -----Original Message-----
> From: Hannes Domani <ssbssa@yahoo.de>
> Sent: Freitag, 9. Oktober 2026 16:37
> To: gdb-patches@sourceware.org; Rohr, Stephan <stephan.rohr@intel.com>
> Cc: Joos, Christina <christina.joos@intel.com>
> Subject: Re: [PATCH v4 8/9] Windows gdb: Implement AVX-512 register support
> 
>  Am Montag, 5. Oktober 2026 um 16:09:19 MESZ hat Rohr, Stephan
> <stephan.rohr@intel.com> Folgendes geschrieben:
> 
> > Hi Hanns,
> >
> > also here, some minor findings below.
> >
> > 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 8/9] Windows gdb: Implement AVX-512 register
> > > support
> > >
> > > Similar to commit 01f9f808e2e86187c95e7cff4aeb014a421a53ce for
> > > Linux,
> >
> > Maybe name the commit title instead of the hash. Not sure what is best
> > practice for this.
> 
> This part was suggested to me in another review, so maybe I should just add
> both commit hash and title.
> 
> 
> > > this patch adds support for the Intel AVX-512 registers for 32 and
> > > 64-bit mode on Windows.
> > > It enables accessing registers $ymm0 - $ymm31, $zmm0 - $zmm31, and
> > > $k0 - $k7 where they are available.
> > >
> >
> > Wouldn't we want to mention "It enables accessing registers
> > $ymm16 - $ymm31 [..]" instead?
> 
> Ok.
> 
> 
> > > After this patch gdb.arch/i386-avx512.exp passes on windows.
> > > ---
> > > v3:
> > > - merged gdb+gdbserver parts, and split again AVX/AVX-512 parts
> > > v4:
> > > - Add reference to equivalent Linux commit in commit message
> > > - Add check for enabled xstate_features in get_context_reg_ptr of
> > > x86-windows-nat.c
> > > - Changed all NULL to nullptr
> > > ---
> > > gdb/NEWS | 4 +++-
> > > gdb/nat/windows-nat.c | 4 ++--
> > > gdb/x86-windows-nat.c | 40 +++++++++++++++++++++++++++++++
> > > gdbserver/win32-i386-low.cc | 48
> > > ++++++++++++++++++++++++++++++++++++-
> > > 4 files changed, 92 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > index da4bc811c3a..704e6a7c8ba 100644
> > > --- a/gdb/NEWS
> > > +++ b/gdb/NEWS
> > > @@ -195,7 +195,9 @@ info locals
> > > The s390 64-bit target (s390x-*) remains supported.
> > >
> > > * Support for Intel AVX registers on Windows.
> > > - Support displaying and modifying Intel AVX registers $ymm0 - $ymm15.
> > > + Support displaying and modifying Intel AVX registers $ymm0 - $ymm31.
> >
> > AVX is limited to $ymm0 - $ymm15. Maybe add another bullet "Support
> > for Intel AVX-512 registers on Windows." and list what's introduced in
> > this patch.
> 
> Ok.
> 
> 
> > > + 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
> > > cd3c827a5d5..3940465d06b 100644
> > > --- a/gdb/nat/windows-nat.c
> > > +++ b/gdb/nat/windows-nat.c
> > > @@ -290,7 +290,7 @@ windows_thread_info::zero_xstate_features () if
> > > (zeroed_features == 0) return;
> > >
> > > - for (int f = X86_XSTATE_AVX_ID; f <= X86_XSTATE_AVX_ID; f++)
> > > + for (int f = X86_XSTATE_AVX_ID; f <= X86_XSTATE_ZMM_ID; f++)
> > > {
> > > DWORD64 flag = 1ULL << f;
> > > if ((zeroed_features & flag) != 0)
> > > @@ -1373,7 +1373,7 @@ initialize_loadable () {
> > > /* Available XState features masked with implemented features. */
> > > xstate_features = (GetEnabledXStateFeatures ()
> > > - & X86_XSTATE_AVX_MASK);
> > > + & X86_XSTATE_AVX_AVX512_MASK);
> >
> > I think it makes sense to add a check that bits X86_XSTATE_K,
> X86_XSTATE_ZMM_H
> > and X86_XSTATE_ZMM are all set. x86-xstate.h explicitly mentions
> > "AVX 512 adds three feature bits. All three must be enabled.". I know this is a
> > theoretical example but it doesn't hurt, e.g.,
> >
> > if ((xstate_features & X86_XSTATE_AVX512) != X86_XSTATE_AVX512)
> > xstate_features &= ~X86_XSTATE_AVX512;
> 
> I don't really get the why, but ok.
> 
> 
> > > /* 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 1e44a45171c..afc71b4ef61 100644
> > > --- a/gdb/x86-windows-nat.c
> > > +++ b/gdb/x86-windows-nat.c
> > > @@ -275,6 +275,46 @@ get_context_reg_ptr (Context *context, int r,
> > > i386_gdbarch_tdep *tdep)
> > > (context, X86_XSTATE_AVX_ID, nullptr);
> > > context_offset += 16 * (r - I387_YMM0H_REGNUM (tdep));
> > > }
> > > + else if ((xstate_features & X86_XSTATE_ZMM_H) != 0
> > > + && I387_ZMM0H_REGNUM (tdep) > 0 && r >=
> > > I387_ZMM0H_REGNUM (tdep)
> > > + && r < I387_ZMM16H_REGNUM (tdep) && r <
> > > I387_ZMMENDH_REGNUM (tdep))
> > > + {
> > > + context_offset = (char *) locate_xstate_feature
> > > + (context, X86_XSTATE_ZMM_H_ID, nullptr);
> > > + context_offset += 32 * (r - I387_ZMM0H_REGNUM (tdep));
> > > + }
> > > + else if ((xstate_features & X86_XSTATE_ZMM) != 0
> > > + && 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, nullptr);
> > > + context_offset += 32 + 64 * (r - I387_ZMM16H_REGNUM (tdep));
> > > + }
> > > + else if ((xstate_features & X86_XSTATE_K) != 0
> > > + && 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, nullptr);
> > > + context_offset += 8 * (r - I387_K0_REGNUM (tdep));
> > > + }
> > > + else if ((xstate_features & X86_XSTATE_ZMM) != 0
> > > + && 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, nullptr);
> > > + context_offset += 16 + 64 * (r - I387_YMM16H_REGNUM (tdep));
> > > + }
> > > + else if ((xstate_features & X86_XSTATE_ZMM) != 0
> > > + && 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, nullptr);
> > > + 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 1a45b15e0cf..8c62787a31b 100644
> > > --- a/gdbserver/win32-i386-low.cc
> > > +++ b/gdbserver/win32-i386-low.cc
> > > @@ -515,8 +515,11 @@ get_context_reg_ptr (Context *context, int r,
> const
> > > target_desc *tdesc)
> > > amd64 = false;
> > > }
> > >
> > > - 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)
> > > @@ -529,6 +532,49 @@ get_context_reg_ptr (Context *context, int r,
> const
> > > target_desc *tdesc)
> > > (context, X86_XSTATE_AVX_ID, nullptr);
> > > 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, nullptr);
> > > + context_offset += 32 * (r - zmm0h_regnum);
> > > + }
> > > + 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, nullptr);
> > > + 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, nullptr);
> > > + 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, nullptr);
> > > + 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, nullptr);
> > > + context_offset += 64 * (r - xmm16_regnum);
> > > + }
> > > else
> > > gdb_assert_not_reached ("invalid register number %d", r);
> > >
> > > --
> > > 2.55.0

I agree with all the comments Stephan has made here. I don't have any additional remarks,
except for the point about locate_xstate_feature, which is relevant to the AVX patch as well
and handled in a different thread.

Regards,
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.