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

Luis Machado
2025-06-26T16:04:43Z

On 6/24/25 14:46, Schimpe, Christina wrote:
> 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?
>

I meant on this patch. Sorry, missed this reply.