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

[Julien Grall <julien@xxxxxxx>]

(+ Bertrand)

Hi Jan,

On 27/01/2022 14:34, 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).
So I agree that this construct will always result to a minimum 5 lines. 
Which is not nice. But the proposed change is...
> 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>
> ---
> v2: Rename new macro parameter.
> ---
> RFC: The use of a control flow construct as a macro argument may be
>       controversial.
indeed controversial. I find this quite confusing and not something I 
would request a user to switch to if they use the longer version.
That said, this is mainly a matter of taste. So I am interested to hear 
others view.
I have also CCed Bertrand to have an opinions from the Fusa Group (I 
suspect this will go backward for them).
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -826,11 +826,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;
> @@ -920,11 +916,7 @@ static int xenmem_add_to_physmap_batch(s
>        * call doesn't succumb to dead-code-elimination. Duplicate the 
> short-circut
>        * from xatp_permission_check() to try and help the compiler out.
>        */
> -    if ( !paging_mode_translate(d) )
> -    {
> -        ASSERT_UNREACHABLE();
> -        return -EACCES;
> -    }
> +    ASSERT(paging_mode_translate(d), return -EACCES);
>   
>       if ( unlikely(xatpb->size < extent) )
>           return -EILSEQ;
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -49,11 +49,13 @@
>   #endif
>   
>   #ifndef NDEBUG
> -#define ASSERT(p) \
> +#define ASSERT(p, ...) \
>       do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>   #define ASSERT_UNREACHABLE() assert_failed("unreachable")
>   #else
> -#define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
> +#define ASSERT(p, failsafe...) do { \
> +        if ( !count_args(failsafe) || unlikely(!(p)) ) { failsafe; } \
> +    } while ( 0 )
>   #define ASSERT_UNREACHABLE() do { } while (0)
>   #endif
>   
Cheers,

--
Julien Grall