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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 05, 2020 at 03:47:43PM +0200, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> unless you have verified the sender and know the content is safe.
> 
> On 05.05.2020 11:24, 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) &&
> >          ^
> > xen/include/asm/x86_64/page.h:161:25: note: expanded from macro 
> > '_PAGE_GNTTAB'
> > #define _PAGE_GNTTAB (1U<<22)
> >                         ^
> 
> This is a rather odd warning. Do they also warn for "if ( 0 )"
> or "do { } while ( 0 )", as we use in various places? There's
> no difference to me between a plain number and a constant
> composed via an expression.

Using plain 0 is fine, they just seem to dislike using << for some
reason that escapes me. Seems like it might be useful to catch bugs
where || is wrongly used instead of | when setting flags, ie:

https://github.com/haproxy/haproxy/issues/588

> 
> > Remove the conversion of _PAGE_GNTTAB to a boolean, since the and
> > operation performed afterwards will already return false if the value
> > of the macro is 0.
> 
> I'm sorry, but no. The expression was put there on purpose by
> 0932210ac095 ("x86: Address "Bitwise-and with zero
> CONSTANT_EXPRESSION_RESULT" Coverity issues"), and the
> description there is clearly telling us that this wants to stay
> unless Coverity changed in the meantime. Otherwise I'm afraid
> a more elaborate solution will be needed to please both.

Clang is fine with changing this to _PAGE_GNTTAB != 0. Would you be
OK with this approach?

> Or a
> more simplistic one, like using "#if _PAGE_GNTTAB" around the
> construct.

Yes, that's the other solution I had in mind.

Thanks, Roger.