| Message ID | 20200505092454.9161-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | build: fixes for clang 10 | expand |
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. > 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. Or a more simplistic one, like using "#if _PAGE_GNTTAB" around the construct. Jan
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.
On 05.05.2020 16:11, Roger Pau Monné wrote: > On Tue, May 05, 2020 at 03:47:43PM +0200, Jan Beulich wrote: >> On 05.05.2020 11:24, Roger Pau Monne wrote: >>> 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? I'd be okay with it, but then I guess I'd prefer ... >> Or a >> more simplistic one, like using "#if _PAGE_GNTTAB" around the >> construct. > > Yes, that's the other solution I had in mind. .... this one. Let's see if Andrew has a clear opinion either way - it was him to address the original Coverity issue after all. Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 355c50ff91..27069d2451 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -1236,7 +1236,7 @@ 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 ( (l1e_get_flags(l1e) & _PAGE_GNTTAB) && !l1e_owner->is_shutting_down && !l1e_owner->is_dying ) { gdprintk(XENLOG_WARNING,
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) ^ 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. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)