Re: [PATCH for-4.19? 3/6] xen/macros: Introduce BUILD_ERROR()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/06/2024 10:23 am, Jan Beulich wrote:
> On 25.06.2024 21:07, Andrew Cooper wrote:
>> --- a/xen/include/xen/macros.h
>> +++ b/xen/include/xen/macros.h
>> @@ -59,6 +59,8 @@
>>  #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond))
>>  #endif
>>  
>> +#define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" )
> I think this wants a comment, and one even beyond what is said for
> BUILD_BUG_ON(). This is primarily to make clear to people when to use
> which, i.e. I consider it important to mention here that it is intended
> for code which, in the normal case, we expect to be DCE-d. The nature
> of the condition used is also a relevant factor, as in some cases
> BUILD_BUG_ON() simply cannot be used because something that really is
> compile time constant isn't an integer constant expression. With
> something to this effect:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Done, thanks.

>
> I have another question / suggestion, though.
>
>> --- a/xen/include/xen/self-tests.h
>> +++ b/xen/include/xen/self-tests.h
>> @@ -22,9 +22,9 @@
>>          typeof(fn(val)) real = fn(val);                                 \
>>                                                                          \
>>          if ( !__builtin_constant_p(real) )                              \
>> -            asm ( ".error \"'" STR(fn(val)) "' not compile-time constant\"" 
>> ); \
>> +            BUILD_ERROR("'" STR(fn(val)) "' not compile-time constant"); \
>>          else if ( real != res )                                         \
>> -            asm ( ".error \"Compile time check '" STR(fn(val) == res) "' 
>> failed\"" ); \
>> +            BUILD_ERROR("Compile time check '" STR(fn(val) == res) "' 
>> failed"); \
>>      } while ( 0 )
> While right here leaving the condition outside of the macro is
> perhaps more natural, considering a case where there's just an if()
> I wonder whether we shouldn't also (only?) have BUILD_ERROR_ON(),
> better paralleling BUILD_BUG_ON():

Right now none of the uses in this series, nor any of the MISRA
conversions away __bad_*() want an _ON() form.

Nothing stops us adding an _ON() form in the future if a reasonable
usecase appears, but right now there's no need.

~Andrew