Re: [PATCH RFC] lib: extend ASSERT()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/01/2021 12:45, Jan Beulich wrote:
> The increasing amount of constructs along the lines of
>
>     if ( !condition )
>     {
>         ASSERT_UNREACHABLE();
>         return;
>     }
>
> is not only longer than necessary, but also doesn't produce incident
> specific console output (except for file name and line number). Allow
> the intended effect to be achieved with ASSERT(), by giving it a second
> parameter allowing specification of the action to take in release builds
> in case an assertion would have triggered in a debug one. The example
> above then becomes
>
>     ASSERT(condition, return);
>
> Make sure the condition will continue to not get evaluated when just a
> single argument gets passed to ASSERT().
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> RFC: The use of a control flow construct as a macro argument may be
>      controversial.

So I had been putting some consideration towards this.  I agree that the
current use of ASSERT_UNREACHABLE() isn't great, and that we ought to do
something to improve the status quo.

However, the more interesting constructs to consider are the ones with
printk()'s and/or domain_crash().  While a straight return or goto in
alt... is perhaps acceptable, anything more complicated probably isn't.

I also found, with my still-pending domain_crash() cleanup series, that
domain_crash()/ASSERT_UNREACHABE()/return/goto became an increasingly
common combination.

>
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -818,11 +818,7 @@ int xenmem_add_to_physmap(struct domain
>      union add_to_physmap_extra extra = {};
>      struct page_info *pages[16];
>  
> -    if ( !paging_mode_translate(d) )
> -    {
> -        ASSERT_UNREACHABLE();
> -        return -EACCES;
> -    }
> +    ASSERT(paging_mode_translate(d), return -EACCES);
>  
>      if ( xatp->space == XENMAPSPACE_gmfn_foreign )
>          extra.foreign_domid = DOMID_INVALID;
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -55,12 +55,14 @@
>  #endif
>  
>  #ifndef NDEBUG
> -#define ASSERT(p) \
> +#define ASSERT(p, ...) \
>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>  #define ASSERT_UNREACHABLE() assert_failed("unreachable")
>  #define debug_build() 1
>  #else
> -#define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
> +#define ASSERT(p, alt...) do { \
> +        if ( !count_args(alt) || unlikely(!(p)) ) { alt; } \

I'd strongly recommend naming this failsafe... rather than alt, to make
it clear what its purpose is.

Also, we really can't have (p) conditionally evaluated depending on
whether there are any failsafe arguments or not.  It is already bad
enough that its state of evaluation differs between debug and release
builds.

~Andrew

> +    } while ( 0 )
>  #define ASSERT_UNREACHABLE() do { } while (0)
>  #define debug_build() 0
>  #endif