| Message ID | 20200528144023.10814-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clang/LLVM build fixes | expand |
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@suse.com> Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 54980b4eb1..7bcac17e2e 100644 --- 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 ) { gdprintk(XENLOG_WARNING, @@ -1244,6 +1245,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner) l1e_get_intpte(l1e)); domain_crash(l1e_owner); } +#endif /* * Remember we didn't take a type-count of foreign writable mappings
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 and instead use a preprocessor conditional to check if _PAGE_GNTTAB is defined. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Use a preprocessor conditional. --- xen/arch/x86/mm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)