Re: [PATCH v2 1/3] x86/mm: do not attempt to convert _PAGE_GNTTAB to a boolean

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.05.2020 16:40, Roger Pau Monne wrote:
> Clang 10 complains with:
> 
> mm.c:1239:10: error: converting the result of '<<' to a boolean always 
> evaluates to true
>       [-Werror,-Wtautological-constant-compare]
>     if ( _PAGE_GNTTAB && (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&

And taking the wording of the message exactly as it is, it is wrong
(if the shifted value is zero, or if all set bits get shifted out).
But the warning is bogus imo anyway - we have it like this for a
reason. I'd then wonder whether -Wtautological-constant-compare
actually does any good, or whether as an alternative we don't want
to disable it.

I further wonder whether they might not warn about the same in #if
down the road.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1236,7 +1236,8 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain 
> *l1e_owner)
>       * (Note that the undestroyable active grants are not a security hole in
>       * Xen. All active grants can safely be cleaned up when the domain dies.)
>       */
> -    if ( _PAGE_GNTTAB && (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
> +#if _PAGE_GNTTAB
> +    if ( (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
>           !l1e_owner->is_shutting_down && !l1e_owner->is_dying )

Us moving in general(?) away from #if / #ifdef to constructs including
the condition in a suitable form in a non-preprocessor expression, I
think we want a comment here clarifying that this shouldn't be
converted back to how it is right now. With that added, for the
immediate purpose:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan