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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/05/2023 5:15 pm, Jan Beulich wrote:
> 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.

???

The dimensions already exist, as proved by the fact GCC can spot the
violation.

On the other hand, zero extending a featureset is explicitly how they're
supposed to work.  How do you think Xapi has coped with migration
compatibility over the years, not to mention the microcode changes which
lengthen a featureset?

So no, there was never any problem with constructs of the form uint32_t
foo[10] = { 1, } in the first place.

The BUILD_BUG_ON()s therefore serve no purpose at all.

~Andrew