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

Luis Machado
2025-06-23T12:57:52Z

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.

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