RE: [PATCH v5 05/12] gdb, gdbserver: Use xstate_bv for target description creation on x86.

[Andrew Burgess <aburgess@redhat.com>]

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

> Hi Andrew,
>
> Do you have any final comments ?
> My plan would be to fix your comments locally (as described below) and push the series by tomorrow.
> Or should I repost this patch to be sure?
>
> Thanks,
> Christina
>
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Tuesday, July 15, 2025 12:28 PM
> To: Andrew Burgess <aburgess@redhat.com>; gdb-patches@sourceware.org
> Cc: thiago.bauermann@linaro.org; luis.machado@arm.com
> Subject: Re: [PATCH v5 05/12] gdb, gdbserver: Use xstate_bv for target description creation on x86.
>
> Hi Andrew,
>
> Thanks a lot for the review.
>
>>> The XSAVE features set is organized in state components, which are a set
>>> of or parts of registers.  So-called XSAVE-supported features are
>
>> Maybe here you mean: "..., which are a set of, or parts of, registers."
>> If not, then this sentence doesn't make sense to me
>
> I agree, this sentence is a bit confusing. I think I would prefer to write it out actually.
> Is the following more understandable?
> "The XSAVE function set is organized in state components, which are a set of registers
> or parts of registers."
>
>>> The SDM uses the term xstate_bv for a state-component bitmap, which is
>
>> Would it be possible to define what SDM it please.
>
> Do you mean writing it out to "Intel Software Developer's Manual" ?

Yes, just the first time SDM is referenced.  Ideally these commit
messages should be written for a general audience, who might not be
familiar with Intel acronyms.

>> I'd like to ask about the use of 'mask' in this variable name.  We used
>> to pass in an xcr0 value, which was then combined with various masks to
>> extract the state bits we were interested in.
>
>> You've renamed the new variable as a mask, but continue to combine it
>> with the same masks (e.g. X86_XSTATE_AVX), which makes me suspect the
>> variable is not a mask at all.
>
> It's not a mask at all. I honestly cannot tell why I decided to call this xstate_bv
> instead of xstate_bv_mask when I wrote this patch...
>
>> Now, I can see where the confusion might have come from.  Almost every
>> call to amd64_target_description passes in a *_MASK constant.  But
>> that's OK, given these are bit sets, then the MASK constants actually
>> define valid values.
>
>> I am aware that this sounds like a trivial complaint over a variable
>> name, but I think calling this a mask is pretty confusing (at least to
>> me), so unless I'm not understanding this, then I think it is worth
>> renaming this.
>
>> The use of `mask` for the value is present throughout this patch, not
>> just this one function.
>
> I agree and am glad for your feedback here.
> The current naming is confusing and I will rename it to xstate_bv.

Sounds great.

Thanks,
Andrew