RE: [PATCH v4 7/9] Windows gdb: Implement AVX register support

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

Hi Hannes, 

Please find my feedback below.

The comments in this email are mostly minor nits, and some can be considered optional.

The one topic I'd like to discuss in a bit more detail is the per-register use of locate_xstate_feature.
I believe we already touched on this in the v3 review, but the discussion did not fully conclude, and I initially misunderstood your latest comment regarding the offsets.
I added my feedback now in the separate thread which also describes the background a bit:
https://sourceware.org/pipermail/gdb-patches/2026-October/231057.html

> -----Original Message-----
> From: Hannes Domani <ssbssa@yahoo.de>
> Sent: Freitag, 9. Oktober 2026 16:30
> To: gdb-patches@sourceware.org; Rohr, Stephan <stephan.rohr@intel.com>
> Cc: Joos, Christina <christina.joos@intel.com>
> Subject: Re: [PATCH v4 7/9] Windows gdb: Implement AVX register support
> 
>  Am Montag, 5. Oktober 2026 um 16:09:22 MESZ hat Rohr, Stephan
> <stephan.rohr@intel.com> Folgendes geschrieben:
> 
> > Hi Hannes,
> >
> > some minor findings below.
> >
> > With those fixed.
> >
> > Reviewed-By: Rohr, Stephan <stephan.rohr@intel.com>
> >
> > 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 7/9] 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
> > > v4:
> > > - fixed $ymm15 in NEWS
> > > - Add common function windows_thread_info::zero_xstate_features,
> > > and fix maximum supported feature in for-loop
> > > - Add check for enabled xstate_features in get_context_reg_ptr of
> > > x86-windows-nat.c
> > > - Move amd64 variable in get_context_reg_ptr of win32-i386-low.cc
> > > - Changed all NULL to nullptr
> > > - Don't remove 'r == 0' from child_store_inferior_registers
> > > ---
> > > gdb/NEWS | 3 +++
> > > gdb/nat/windows-nat.c | 36 +++++++++++++++++++++++++-
> > > gdb/nat/windows-nat.h | 6 +++++ gdb/x86-windows-nat.c | 51
> > > ++++++++++++++++++++++++++++++++++---
> > > gdbserver/win32-i386-low.cc | 47 +++++++++++++++++++++++++++++-----
> > > gdbserver/win32-low.cc | 15 +++++++----
> > > 6 files changed, 143 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > index 5d0c4745653..da4bc811c3a 100644
> > > --- a/gdb/NEWS
> > > +++ b/gdb/NEWS
> > > @@ -194,6 +194,9 @@ info locals
> > > 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 - $ymm15.
> > > +
> > > * 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
> > > 58be5368a79..cd3c827a5d5 100644
> > > --- a/gdb/nat/windows-nat.c
> > > +++ b/gdb/nat/windows-nat.c
> > > @@ -271,6 +271,40 @@ windows_thread_info::xfer_siginfo (gdb_byte
> > > *readbuf, return true; }
> > >
> > > +#if defined __i386__ || defined __x86_64__
> > > +/* See nat/windows-nat.h. */
> > > +
> > > +void
> > > +windows_thread_info::zero_xstate_features () {  if (xstate_features
> > > +== 0)  return;
> > > +
> > > + proc->with_context (this, [] (auto *context)
> > > + {
> > > + DWORD64 features = 0;
> > > + if (!get_xstate_features_mask (context, &features)) return;
> > > +
> > > + DWORD64 zeroed_features = xstate_features & ~features; if
> > > + (zeroed_features == 0) return;
> > > +
> > > + for (int f = X86_XSTATE_AVX_ID; f <= X86_XSTATE_AVX_ID; f++)
> >
> > Start == End.
> 
> That's intentional, it changes with the next patch anyways.

I stumbled over this as well. However, I understand why you chose to keep it this way,
given that the next patch changes it, and I'm fine with that.

> 
> > > + {
> > > + 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);  }  }  }); } #endif
> > > +
> > > /* Try to determine the executable filename.
> > >
> > > EXE_NAME_RET is a pointer to a buffer whose size is EXE_NAME_MAX_LEN.
> > > @@ -1339,7 +1373,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/nat/windows-nat.h b/gdb/nat/windows-nat.h
> > > index a7d4af84e13..4cd75cb6a0e 100644
> > > --- a/gdb/nat/windows-nat.h
> > > +++ b/gdb/nat/windows-nat.h
> > > @@ -90,6 +90,12 @@ struct windows_thread_info ULONGEST offset,
> > > ULONGEST len, ULONGEST *xfered_len);
> > >
> > > +#if defined __i386__ || defined __x86_64__
> > > + /* Zero all XState feature areas that were not provided by
> > > +GetThreadContext. */  void zero_xstate_features (); #endif
> > > +
> > > /* The process this thread belongs to. */ windows_process_info
> > > *const proc;
> > >
> > > diff --git a/gdb/x86-windows-nat.c b/gdb/x86-windows-nat.c index
> > > 3af5ef4dae0..1e44a45171c 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,14 @@ 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); }
> > > CHECK (get_thread_context (th->h, context));
> > > +
> > > + th->zero_xstate_features ();
> > > }
> > > });
> > > }
> > > @@ -198,6 +210,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 +247,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)
> > > {

Nit: explain the new parameter in the function comment and in which cases this is required.

> > > const int *mappings;
> > > int mappings_count;
> > > @@ -247,6 +267,14 @@ get_context_reg_ptr (Context *context, int r)
> > > char *context_offset;
> > > if (r < mappings_count)
> > > context_offset = (char *) context + mappings[r];
> > > + else if ((xstate_features & X86_XSTATE_AVX) != 0
> > > + && I387_YMM0H_REGNUM (tdep) > 0 && r >=
> > > I387_YMM0H_REGNUM (tdep)
> > > + && r < I387_YMMENDH_REGNUM (tdep))
> > > + {
> > > + context_offset = (char *) locate_xstate_feature
> > > + (context, X86_XSTATE_AVX_ID, nullptr);
> > > + context_offset += 16 * (r - I387_YMM0H_REGNUM (tdep));
> > > + }
> > > else
> > > gdb_assert_not_reached ("invalid register number %d", r);
> > >
> > > @@ -267,7 +295,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 +361,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 +396,23 @@ x86_windows_nat_target::is_sw_breakpoint (const
> > > EXCEPTION_RECORD *er) const
> > > || er->ExceptionCode == STATUS_WX86_BREAKPOINT);
> > > }
> > >
> >
> > Add
> >
> > /* See target.h. */
> 
> Ok.
> 
> 
> > > +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..1a45b15e0cf 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,8 @@ i386_get_thread_context (windows_thread_info *th)
> > >
> > > error (_("GetThreadContext failure %ld\n"), (long) e);
> > > }
> > > +
> > > + th->zero_xstate_features ();
> > > });
> > > }
> > >
> > > @@ -292,6 +299,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,26 +495,40 @@ 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)

Similar feedback for the tdesc parameter for the function comment.

> > > {
> > > const int *mappings;
> > > int mappings_count;
> > > + bool amd64;
> > > #ifdef __x86_64__
> > > if (!windows_process.wow64_process)
> > > {
> > > mappings = amd64_mappings;
> > > mappings_count = sizeof (amd64_mappings) / sizeof
> > > (amd64_mappings[0]);
> > > + amd64 = true;
> > > }
> > > else
> > > #endif
> > > {
> > > mappings = i386_mappings;
> > > mappings_count = sizeof (i386_mappings) / sizeof (i386_mappings[0]);
> > > + amd64 = false;
> > > }
> > >
> > > + 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, nullptr);
> >
> > We don't check "context_offset" for nulltpr before adding the offsets. We're
> > checking for nullptr and size explicitly in "zero_xstate_features". So, I was
> > wondering if we should add checks here, too. IIUC, it should not be possible
> > to have "locate_xstate_feature () == nullptr" at this point. Maybe add an
> > assert for consistency?
> 
> Same for x86-windows-nat.c?

If we keep the code as is, it might be a good idea. But this might also change with the
result of our general discussion for the use of locate_xstate_feature.

> > > + context_offset += 16 * (r - ymm0h_regnum);
> > > + }
> > > else
> > > gdb_assert_not_reached ("invalid register number %d", r);
> > >
> > > @@ -510,7 +542,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 +570,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 +603,17 @@ i386_arch_setup (void)
> > > {
> > > target_desc_up tdesc;
> > >
> > > + DWORD64 xcr0 = xstate_features;
> > > + if (xcr0 == 0)
> > > + xcr0 = X86_XSTATE_SSE_MASK;

You could call this xstate_bv instead even though you shadow stacks are not supported yet.
This has changed in commit: " gdb, gdbserver: Use xstate_bv for target description creation on x86."

> > > +
> > > #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 1103a802db7..3e80405f651 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;
> > >
> > > @@ -427,8 +428,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);
> > With r != -1 and r > regcache->tdesc->reg_defs.size (), this may fall through into
> > "get_context_reg_ptr" and eventually into the gdb_assert_not_reached_part.
> >
> > Maybe add another check, e.g.,
> >
> > int num_regs = regcache->tdesc->reg_defs.size ();
> > if (r == -1 || r > num_regs)
> > r = num_regs;
> >
> > Same for child_store_inferior_registers.
> 
> Adding this is fine with me, but I just wonder why r > reg_defs.size()
> shouldn't get to gdb_assert_not_reached_part.

I am +1 for just having the gdb_assert_not_reached deeper down in the stack - to avoid duplication.
But Stephan might have a different opinion on that.

I'd rather improve the function description for the parameter R, but this should be
be an additional patch (since you don't touch the function description).
And you of course don't have to do this. If you want to do this, it could also be independent of this series.

Similar suggestion for the removal of "r==0" in  child_store_inferior_registers below
(which you had in v3). I think it's a good idea.
But same as above: if you want to do this, it could also be independent of this series and should be a separate patch.

> > > + 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);
> > > @@ -442,8 +444,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 || r == 0)
> > > + child_store_inferior_registers (regcache,
> > > + regcache->tdesc->reg_defs.size ());
> > > else
> > > for (regno = 0; regno < r; regno++)
> > > (*the_low_target.store_inferior_register) (regcache, th, regno);
> > > @@ -1347,7 +1350,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.55.0
> 
> 
> Hannes

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.