[v2,1/3] x86/mm: do not attempt to convert _PAGE_GNTTAB to a boolean
diff mbox series

Message ID 20200528144023.10814-2-roger.pau@citrix.com
State New
Headers show
Series
  • Clang/LLVM build fixes
Related show

Commit Message

Roger Pau Monné May 28, 2020, 2:40 p.m. UTC
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(-)

Comments

Jan Beulich May 29, 2020, 3:10 p.m. UTC | #1
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

Patch
diff mbox series

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