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

[Hannes Domani <ssbssa@yahoo.de>]

 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.


> > + {
> > + 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)
> > {
> > 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)
> > {
> > 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?


> > + 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;
> > +
> > #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.


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