Re: [PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/05/2024 3:27 pm, Roger Pau Monné wrote:
> On Tue, May 14, 2024 at 02:05:10PM +0100, Andrew Cooper wrote:
>> On 14/05/2024 8:53 am, Roger Pau Monné wrote:
>>> On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote:
>>>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>>>> index 6ee835b22949..2f34694e9c57 100644
>>>> --- a/tools/misc/xen-cpuid.c
>>>> +++ b/tools/misc/xen-cpuid.c
>>>> @@ -291,6 +292,9 @@ static const struct {
>>>>  
>>>>  #define COL_ALIGN "24"
>>>>  
>>>> +static const char *const feature_names[(FEATURESET_NR_ENTRIES + 1) << 5] =
>>>> +    INIT_FEATURE_VAL_TO_NAME;
>>> I've also considered this when doing the original patch, but it seemed
>>> worse to force each user of INIT_FEATURE_VAL_TO_NAME to have to
>>> correctly size the array.  I would also use '* 32', as it's IMO
>>> clearer and already used below when accessing the array.  I'm fine
>>> if we want to go this way, but the extra Python code to add a last
>>> array entry if required didn't seem that much TBH.
>> I was looking to avoid the other BUILD_BUG_ON()'s, and in particular
>> bringing in known_features just for a build time check.
>>
>> Given that there's only one instance right now, and no obvious other
>> usecase, I'd say this is better.  In terms of just xen-cpuid.c, it's
>> clearly correct whereas leaving it implicitly to
>> INIT_FEATURE_VAL_TO_NAME is not.
> If you dislike my original attempt at doing this, what about casting
> the literal array initializer created by gen-cpuid.py, so that the
> result ends up looking like:
>
> #define INIT_FEATURE_NAME_ARRAY (const char *[(FEATURESET_NR_ENTRIES + 1) * 
> 32]) { \
> ...
>
> Would that be better?

That will trigger -Wvla, I think.

~Andrew