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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/05/2023 7:43 am, Jan Beulich wrote:
> On 10.05.2023 17:06, Andrew Cooper wrote:
>> 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.
> Where? Quoting cpu-policy.c:
>
> const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>
> static const uint32_t __initconst pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
> static const uint32_t hvm_shadow_max_featuremask[] = 
> INIT_HVM_SHADOW_MAX_FEATURES;
> static const uint32_t __initconst hvm_hap_max_featuremask[] =
>     INIT_HVM_HAP_MAX_FEATURES;
> static const uint32_t __initconst pv_def_featuremask[] = INIT_PV_DEF_FEATURES;
> static const uint32_t __initconst hvm_shadow_def_featuremask[] =
>     INIT_HVM_SHADOW_DEF_FEATURES;
> static const uint32_t __initconst hvm_hap_def_featuremask[] =
>     INIT_HVM_HAP_DEF_FEATURES;
> static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
>
> I notice that known_features[], as an exception, has its dimension declared
> in cpuid.h.

Ah.  I had indeed not spotted that.  Sorry.

It explains why all of my test builds (checking known_features[])
appeared to work.  I will rework these to have dimensions, because it
will remove some reasonably complex logic in gen-cpuid.py.

>
>> 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.
> As per above the very minimum would be to accompany their dropping with
> adding of explicitly specified dimensions for all the static arrays. I'm
> not entirely certain about the other side (the zero-extension), but I'd
> likely end up simply trusting you on that.

https://godbolt.org/z/c13Kxcdsh

GCC (on both extremes that godbolt supports) zero extends to the
declaration dimension size.

~Andrew