Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.05.2021 17:44, Tim Deegan wrote:
> Hi,
> 
> At 10:58 +0200 on 25 May (1621940330), Jan Beulich wrote:
>> On 24.05.2021 06:29, Roberto Bagnara wrote:
>>> I stumbled upon parsing errors due to invalid uses of
>>> _Static_assert expanded from HASH_CALLBACKS_CHECK where
>>> the tested expression is not constant, as mandated by
>>> the C standard.
>>>
>>> Judging from the following comment, there is partial awareness
>>> of the fact this is an issue:
>>>
>>> #ifndef __clang__ /* At least some versions dislike some of the uses. */
>>> #define HASH_CALLBACKS_CHECK(mask) \
>>>      BUILD_BUG_ON((mask) > (1U << ARRAY_SIZE(callbacks)) - 1)
>>>
>>> Indeed, this is not a fault of Clang: the point is that some
>>> of the expansions of this macro are not C.  Moreover,
>>> the fact that GCC sometimes accepts them is not
>>> something we can rely upon:
> 
> Well, that is unfortunate - especially since the older ad-hoc
> compile-time assertion macros handled this kind of thing pretty well.
> Why when I were a lad &c &c. :)

So I have to admit I don't understand: The commit introducing
HASH_CALLBACKS_CHECK() (90629587e16e "x86/shadow: replace stale
literal numbers in hash_{vcpu,domain}_foreach()") did not replace
any prior compile-time checking. Hence I wonder what you're
referring to (and hence what alternative ways of dealing with the
situation there might be that I'm presently not seeing).

>>> Finally, I think this can be easily avoided: instead
>>> of initializing a static const with a constant expression
>>> and then static-asserting the static const, just static-assert
>>> the constant initializer.
>>
>> Well, yes, but the whole point of constructs like
>>
>>     HASH_CALLBACKS_CHECK(callback_mask);
>>     hash_domain_foreach(d, callback_mask, callbacks, gmfn);
>>
>> is to make very obvious that the checked mask and the used mask
>> match. Hence if anything I'd see us eliminate the static const
>> callback_mask variables altogether.
> 
> That seems like a good approach.

Okay, I'll make a patch then.

Jan