RE: [PATCH v4 04/11] gdb, gdbserver: Use xstate_bv for target description creation on x86.

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Luis, 

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Monday, June 23, 2025 2:56 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: thiago.bauermann@linaro.org; eliz@gnu.org
> Subject: Re: [PATCH v4 04/11] gdb, gdbserver: Use xstate_bv for target
> description creation on x86.
> 
> On 6/23/25 13:46, Schimpe, Christina wrote:
> > Hi Luis,
> >
> > Thanks a lot for the feedback. Please find my comments below.
> >
> >> -----Original Message-----
> >> From: Luis Machado <luis.machado@arm.com>
> >> Sent: Thursday, June 19, 2025 11:24 AM
> >> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> >> patches@sourceware.org
> >> Cc: thiago.bauermann@linaro.org; eliz@gnu.org
> >> Subject: Re: [PATCH v4 04/11] gdb, gdbserver: Use xstate_bv for
> >> target description creation on x86.
> >>
> >> On 6/17/25 13:11, Christina Schimpe wrote:
> >>> diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c index
> >>> 252650b6390..9cac0645cdc 100644
> >>> --- a/gdb/arch/amd64.c
> >>> +++ b/gdb/arch/amd64.c
> >>> @@ -30,14 +30,11 @@
> >>>
> >>>  #include "../features/i386/x32-core.c"
> >>>
> >>> -/* Create amd64 target descriptions according to XCR0.  If IS_X32 is
> >>> -   true, create the x32 ones.  If IS_LINUX is true, create target
> >>> -   descriptions for Linux.  If SEGMENTS is true, then include
> >>> -   the "org.gnu.gdb.i386.segments" feature registers.  */
> >>> +/* See amd64.h.  */
> >>
> >> More appropriate to reference arch/amd64.h.
> >
> > True, will fix.
> >
> >>
> >>>
> >>>  target_desc *
> >>> -amd64_create_target_description (uint64_t xcr0, bool is_x32, bool
> is_linux,
> >>> -				 bool segments)
> >>> +amd64_create_target_description (uint64_t xstate_bv_mask, bool is_x32,
> >>> +				 bool is_linux, bool segments)
> >>>  {
> >>>    target_desc_up tdesc = allocate_target_description ();
> >>>
> >>> @@ -62,13 +59,13 @@ amd64_create_target_description (uint64_t xcr0,
> >> bool is_x32, bool is_linux,
> >>>    if (segments)
> >>>      regnum = create_feature_i386_64bit_segments (tdesc.get (),
> >>> regnum);
> >>>
> >>> -  if (xcr0 & X86_XSTATE_AVX)
> >>> +  if (xstate_bv_mask & X86_XSTATE_AVX)
> >>>      regnum = create_feature_i386_64bit_avx (tdesc.get (), regnum);
> >>>
> >>> -  if (xcr0 & X86_XSTATE_AVX512)
> >>> +  if (xstate_bv_mask & X86_XSTATE_AVX512)
> >>>      regnum = create_feature_i386_64bit_avx512 (tdesc.get (),
> >>> regnum);
> >>>
> >>> -  if (xcr0 & X86_XSTATE_PKRU)
> >>> +  if (xstate_bv_mask & X86_XSTATE_PKRU)
> >>>      regnum = create_feature_i386_pkeys (tdesc.get (), regnum);
> >>>
> >>>    return tdesc.release ();
> >>
> >> <...>
> >>
> >>> diff --git a/gdb/arch/i386.c b/gdb/arch/i386.c index
> >>> 835df53c75d..84f31439e7a 100644
> >>> --- a/gdb/arch/i386.c
> >>> +++ b/gdb/arch/i386.c
> >>> @@ -29,10 +29,11 @@
> >>>  #include "../features/i386/32bit-segments.c"
> >>>  #include "../features/i386/pkeys.c"
> >>>
> >>> -/* Create i386 target descriptions according to XCR0.  */
> >>> +/* See i386.h.  */
> >>
> >> arch/i386.h
> >
> > Will fix.
> >
> >>
> >>>
> >>>  target_desc *
> >>> -i386_create_target_description (uint64_t xcr0, bool is_linux, bool
> >>> segments)
> >>> +i386_create_target_description (uint64_t xstate_bv_mask, bool is_linux,
> >>> +				bool segments)
> >>>  {
> >>>    target_desc_up tdesc = allocate_target_description ();
> >>>
> >>> @@ -44,10 +45,10 @@ i386_create_target_description (uint64_t xcr0,
> >>> bool is_linux, bool segments)
> >>>
> >>>    long regnum = 0;
> >>>
> >>> -  if (xcr0 & X86_XSTATE_X87)
> >>> +  if (xstate_bv_mask & X86_XSTATE_X87)
> >>>      regnum = create_feature_i386_32bit_core (tdesc.get (), regnum);
> >>>
> >>> -  if (xcr0 & X86_XSTATE_SSE)
> >>> +  if (xstate_bv_mask & X86_XSTATE_SSE)
> >>>      regnum = create_feature_i386_32bit_sse (tdesc.get (), regnum);
> >>>
> >>>    if (is_linux)
> >>> @@ -56,13 +57,13 @@ i386_create_target_description (uint64_t xcr0,
> >>> bool
> >> is_linux, bool segments)
> >>>    if (segments)
> >>>      regnum = create_feature_i386_32bit_segments (tdesc.get (),
> >>> regnum);
> >>>
> >>> -  if (xcr0 & X86_XSTATE_AVX)
> >>> +  if (xstate_bv_mask & X86_XSTATE_AVX)
> >>>      regnum = create_feature_i386_32bit_avx (tdesc.get (), regnum);
> >>>
> >>> -  if (xcr0 & X86_XSTATE_AVX512)
> >>> +  if (xstate_bv_mask & X86_XSTATE_AVX512)
> >>>      regnum = create_feature_i386_32bit_avx512 (tdesc.get (),
> >>> regnum);
> >>>
> >>> -  if (xcr0 & X86_XSTATE_PKRU)
> >>> +  if (xstate_bv_mask & X86_XSTATE_PKRU)
> >>>      regnum = create_feature_i386_pkeys (tdesc.get (), regnum);
> >>>
> >>>    return tdesc.release ();
> >>
> >> <...>
> >>
> >>> diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c index
> >>> fc7c5f642ed..a82ad21da27 100644
> >>> --- a/gdb/x86-linux-nat.c
> >>> +++ b/gdb/x86-linux-nat.c
> >>> @@ -97,15 +97,20 @@ const struct target_desc *
> >>> x86_linux_nat_target::read_description ()  {
> >>>    /* The x86_linux_tdesc_for_tid call only reads xcr0 the first time it is
> >>> -     called, the xcr0 value is stored here and reused on subsequent calls.  */
> >>> -  static uint64_t xcr0_storage;
> >>> +     called.  The mask is stored in XSTATE_BV_STORAGE and reused on
> >>> +     subsequent calls.  Note that GDB currently supports features for user
> >>> +     state components only.  However, once supervisor state components
> are
> >>> +     supported in GDB XSTATE_BV_STORAGE will not be configured based
> on
> >>> +     xcr0 only.  */
> >>
> >> Something about the comment above reads off in the last phrase. Is
> >> there a missing connection word?
> >
> > I agree, would this be a bit better?
> >
> > "However, once supervisor state components are supported in GDB, the
> > value XSTATE_BV_STORAGE will not be configured based on xcr0 only."
> >
> 
> Yes, that reads better, thanks.
> 
> I don't think you need a new iteration for these fixes though. They could be
> local.

Just to be sure, you mean for this patch or your comments on the series in general?

> >>
> >>> +  static uint64_t xstate_bv_storage;
> >>>
> >>>    if (inferior_ptid == null_ptid)
> >>>      return this->beneath ()->read_description ();
> >>>
> >>>    int tid = inferior_ptid.pid ();
> >>>
> >>> -  return x86_linux_tdesc_for_tid (tid, &xcr0_storage,
> >>> &this->m_xsave_layout);
> >>> +  return x86_linux_tdesc_for_tid (tid, &xstate_bv_storage,
> >>> +				  &this->m_xsave_layout);
> >>>  }
> >>>
> >
> >> Otherwise this looks OK to me. I suppose this was regression tested?
> >> I'd be a bit more comfortable if someone more knowledgeable could
> >> chime in before this gets approved.
> >>
> >> But if nobody shows up, we can get it in if others have no concerns.
> >>
> >> Reviewed-By: Luis Machado <luis.machado@arm.com>
> >
> > Yes, this has been regression tested (recent testing for v4 on ubuntu 24.04).
> > I am currently working on the setup for testing x32 again. This configuration
> has been tested for v1 initially only.
> 
> Alright. Thanks for confirming.

Thanks,
Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928