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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>

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

    BUILD_ERROR_ON(!__builtin_constant_p(real),
                   "'" STR(fn(val)) "' not compile-time constant");

It then becomes questionable whether a string literal needs passing,
or whether instead the condition couldn't just be stringified while
passing to the asm():

    BUILD_ERROR_ON(!__builtin_constant_p(real));

The thing could even "return" the predicate, permitting

    if ( !BUILD_ERROR_ON(!__builtin_constant_p(real) )
        BUILD_ERROR_ON(real != res);

I realize though that there may be issues from this with unused values
being diagnosed by compiler and/or Eclair, when the "return value" is
not of interest.

I'd be fine with the respective transformation to be left for 4.20,
though. Yet of course churn-wise it would be better to get into final
shape right away.

Jan