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

Luis Machado
2025-06-19T09:25:20Z

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.

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

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

> + 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);
> }
> \f
>

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>