Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.05.2023 17:59, Andrew Cooper wrote:
> On 09/05/2023 3:28 pm, Jan Beulich wrote:
>> On 09.05.2023 15:04, Andrew Cooper wrote:
>>> On 08/05/2023 7:47 am, Jan Beulich wrote:
>>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>>> These BUILD_BUG_ON()s exist to cover the curious absence of a diagnostic 
>>>>> for
>>>>> code which looks like:
>>>>>
>>>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>>>
>>>>> However, GCC 12 at least does now warn for this:
>>>>>
>>>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>>>         |                        ^
>>>>>   foo.c:1:24: note: (near initialization for 'foo')
>>>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>>>> the arrays in question don't have explicit dimensions at their
>>>> definition sites, and hence they derive their dimensions from their
>>>> initializers. So the build-time-checks are about the arrays in fact
>>>> obtaining the right dimensions, i.e. the initializers being suitable.
>>>>
>>>> With the core part of the reasoning not being applicable, I'm afraid I
>>>> can't even say "okay with an adjusted description".
>>> Now I'm extra confused.
>>>
>>> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
>>> when I was expecting one, and there was a bug in the original featureset
>>> work caused by this going wrong.
>>>
>>> But godbolt seems to agree that even GCC 4.1 notices.
>>>
>>> Maybe it was some other error (C file not seeing the header properly?)
>>> which disappeared across the upstream review?
>> Or maybe, by mistake, too few initializer fields? But what exactly it
>> was probably doesn't matter. If this patch is to stay (see below), some
>> different description will be needed anyway (or the change be folded
>> into the one actually invalidating those BUILD_BUG_ON()s).
>>
>>> Either way, these aren't appropriate, and need deleting before patch 5,
>>> because the check is no longer valid when a featureset can be longer
>>> than the autogen length.
>> Well, they need deleting if we stick to the approach chosen there right
>> now. If we switched to my proposed alternative, they better would stay.
> 
> Given that all versions of GCC do warn, I don't see any justification
> for them to stay.

All versions warn when the variable declarations / definitions have a
dimension specified, and then there are excess initializers. Yet none
of the five affected arrays have a dimension specified in their
definitions.

Even if dimensions were added, we'd then have only covered half of
what the BUILD_BUG_ON()s cover right now: There could then be fewer
than intended initializer fields, and things may still be screwed. I
think it was for this very reason why BUILD_BUG_ON() was chosen.

Jan

> i.e. this should be committed, even if the commit message says "no idea
> what they were taken originally, but they're superfluous in the logic as
> it exists today".
> 
> ~Andrew