Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Roberto Bagnara]

Hi there.

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:

$ cat p.c
void f() {
static const int x = 3;
_Static_assert(x < 4, "");
}
$ gcc -c -O p.c
$ gcc -c p.c
p.c: In function ‘f’:
p.c:3:20: error: expression in static assertion is not constant
3 | _Static_assert(x < 4, "");
| ~^~
$

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.

Kind regards,

    Roberto Bagnara

Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Jan Beulich]

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:
> 
> $ cat p.c
> void f() {
> static const int x = 3;
> _Static_assert(x < 4, "");
> }
> $ gcc -c -O p.c
> $ gcc -c p.c
> p.c: In function ‘f’:
> p.c:3:20: error: expression in static assertion is not constant
> 3 | _Static_assert(x < 4, "");
> | ~^~
> $

I'd nevertheless like to stick to this as long as not proven
otherwise by future gcc.

> 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. I did avoid doing so in the
earlier change, following the assumption that the choice of
using a static const there was for a reason originally (my guess:
a combination of not wanting to use a #define and of having the
mask values live next to their corresponding arrays).

Cc-ing Tim as the maintainer, to possibly override my views.

Jan

Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Roberto Bagnara]

Hi Jan.

Please see below.

On 25/05/21 10:58, 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:
>>
>> $ cat p.c
>> void f() {
>> static const int x = 3;
>> _Static_assert(x < 4, "");
>> }
>> $ gcc -c -O p.c
>> $ gcc -c p.c
>> p.c: In function ‘f’:
>> p.c:3:20: error: expression in static assertion is not constant
>> 3 | _Static_assert(x < 4, "");
>> | ~^~
>> $
> 
> I'd nevertheless like to stick to this as long as not proven
> otherwise by future gcc.

Just two observations:

1) Violating the C standard makes MISRA complicance significantly
    more difficult.  In addition, it complicates also compiler
    qualification, for those who are required to do it.

2) GCC is already proving otherwise: if you try compiling
    without optimization, compilation fails.

Kind regards,

    Roberto

Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Jan Beulich]

On 28.05.2021 11:59, Roberto Bagnara wrote:
> On 25/05/21 10:58, 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:
>>>
>>> $ cat p.c
>>> void f() {
>>> static const int x = 3;
>>> _Static_assert(x < 4, "");
>>> }
>>> $ gcc -c -O p.c
>>> $ gcc -c p.c
>>> p.c: In function ‘f’:
>>> p.c:3:20: error: expression in static assertion is not constant
>>> 3 | _Static_assert(x < 4, "");
>>> | ~^~
>>> $
>>
>> I'd nevertheless like to stick to this as long as not proven
>> otherwise by future gcc.
> 
> Just two observations:
> 
> 1) Violating the C standard makes MISRA complicance significantly
>     more difficult.  In addition, it complicates also compiler
>     qualification, for those who are required to do it.
> 
> 2) GCC is already proving otherwise: if you try compiling
>     without optimization, compilation fails.

I'm afraid we have other issues when building without optimization.

In any event - feel free to contribute a patch. As said, I'm not
the maintainer of that piece of code, and you may well find him
agreeing with such a change. He didn't reply yet on the earlier
mail, which would be a prereq to me possibly making a patch
myself.

Jan

Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Tim Deegan]

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. :)

> > 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.

Cheers,

Tim.

Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Jan Beulich]

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

Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Tim Deegan]

Hi,

At 08:45 +0200 on 31 May (1622450756), Jan Beulich wrote:
> 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).

Sorry, I wasn't clear.  Before there was compiler support for
compile-time assertions, people used horrible macros that expanded to
things like int x[(p)?0:-1].  (I don't remember which exact flavour we
had in Xen.)  Those worked fine with static consts because the
predicates only had to be compile-time constant in practice, but now
they have to be constant in principle too.

So I don't think there was a better way of adding these assertions in
90629587e16e, I'm just generally grumbling that the official
compile-time assertions are not quite as useful as the hacks they
replaced.

And I am definitely *not* suggesting that we go back to those kind of
hacks just to get around the compiler's insistence on the letter of
the law. :)

Cheers,

Tim.