[PATCH v2] coverity: provide Coverity-friendly MIN_CONST and MAX_CONST

Eric Blake posted 1 patch 1 week ago
Test FreeBSD passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200629162804.1096180-1-eblake@redhat.com
include/qemu/osdep.h | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)

[PATCH v2] coverity: provide Coverity-friendly MIN_CONST and MAX_CONST

Posted by Eric Blake 1 week ago
Coverity has problems seeing through __builtin_choose_expr, which
result in it abandoning analysis of later functions that utilize a
definition that used MIN_CONST or MAX_CONST, such as in qemu-file.c:

 50    DECLARE_BITMAP(may_free, MAX_IOV_SIZE);

CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1.
expr_not_constant: expression must have a constant value

As has been done in the past (see 07d66672), it's okay to dumb things
down when compiling for static analyzers.  (Of course, now the
syntax-checker has a false positive on our reference to
__COVERITY__...)

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: CID 1429992, CID 1429995, CID 1429997, CID 1429999
Signed-off-by: Eric Blake <eblake@redhat.com>
---

Improvements over Paolo's v1:
- proper use of ()
- add comment explaining the COVERITY section
- add indentation for easier read of #if/#else flow

 include/qemu/osdep.h | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 0d26a1b9bd07..0fc206ae6154 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -250,7 +250,8 @@ extern int daemon(int, int);
  * Note that neither form is usable as an #if condition; if you truly
  * need to write conditional code that depends on a minimum or maximum
  * determined by the pre-processor instead of the compiler, you'll
- * have to open-code it.
+ * have to open-code it.  Sadly, Coverity is severely confused by the
+ * constant variants, so we have to dumb things down there.
  */
 #undef MIN
 #define MIN(a, b)                                       \
@@ -258,22 +259,28 @@ extern int daemon(int, int);
         typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
         _a < _b ? _a : _b;                              \
     })
-#define MIN_CONST(a, b)                                         \
-    __builtin_choose_expr(                                      \
-        __builtin_constant_p(a) && __builtin_constant_p(b),     \
-        (a) < (b) ? (a) : (b),                                  \
-        ((void)0))
 #undef MAX
 #define MAX(a, b)                                       \
     ({                                                  \
         typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
         _a > _b ? _a : _b;                              \
     })
-#define MAX_CONST(a, b)                                         \
+
+#ifdef __COVERITY__
+# define MIN_CONST(a, b) ((a) < (b) ? (a) : (b))
+# define MAX_CONST(a, b) ((a) > (b) ? (a) : (b))
+#else
+# define MIN_CONST(a, b)                                        \
+    __builtin_choose_expr(                                      \
+        __builtin_constant_p(a) && __builtin_constant_p(b),     \
+        (a) < (b) ? (a) : (b),                                  \
+        ((void)0))
+# define MAX_CONST(a, b)                                        \
     __builtin_choose_expr(                                      \
         __builtin_constant_p(a) && __builtin_constant_p(b),     \
         (a) > (b) ? (a) : (b),                                  \
         ((void)0))
+#endif

 /*
  * Minimum function that returns zero only if both values are zero.
-- 
2.27.0


Re: [PATCH v2] coverity: provide Coverity-friendly MIN_CONST and MAX_CONST

Posted by Paolo Bonzini 1 week ago
On 29/06/20 18:28, Eric Blake wrote:
> Coverity has problems seeing through __builtin_choose_expr, which
> result in it abandoning analysis of later functions that utilize a
> definition that used MIN_CONST or MAX_CONST, such as in qemu-file.c:
> 
>  50    DECLARE_BITMAP(may_free, MAX_IOV_SIZE);
> 
> CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1.
> expr_not_constant: expression must have a constant value
> 
> As has been done in the past (see 07d66672), it's okay to dumb things
> down when compiling for static analyzers.  (Of course, now the
> syntax-checker has a false positive on our reference to
> __COVERITY__...)
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: CID 1429992, CID 1429995, CID 1429997, CID 1429999
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Improvements over Paolo's v1:
> - proper use of ()
> - add comment explaining the COVERITY section
> - add indentation for easier read of #if/#else flow
> 
>  include/qemu/osdep.h | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 0d26a1b9bd07..0fc206ae6154 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -250,7 +250,8 @@ extern int daemon(int, int);
>   * Note that neither form is usable as an #if condition; if you truly
>   * need to write conditional code that depends on a minimum or maximum
>   * determined by the pre-processor instead of the compiler, you'll
> - * have to open-code it.
> + * have to open-code it.  Sadly, Coverity is severely confused by the
> + * constant variants, so we have to dumb things down there.
>   */
>  #undef MIN
>  #define MIN(a, b)                                       \
> @@ -258,22 +259,28 @@ extern int daemon(int, int);
>          typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>          _a < _b ? _a : _b;                              \
>      })
> -#define MIN_CONST(a, b)                                         \
> -    __builtin_choose_expr(                                      \
> -        __builtin_constant_p(a) && __builtin_constant_p(b),     \
> -        (a) < (b) ? (a) : (b),                                  \
> -        ((void)0))
>  #undef MAX
>  #define MAX(a, b)                                       \
>      ({                                                  \
>          typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>          _a > _b ? _a : _b;                              \
>      })
> -#define MAX_CONST(a, b)                                         \
> +
> +#ifdef __COVERITY__
> +# define MIN_CONST(a, b) ((a) < (b) ? (a) : (b))
> +# define MAX_CONST(a, b) ((a) > (b) ? (a) : (b))
> +#else
> +# define MIN_CONST(a, b)                                        \
> +    __builtin_choose_expr(                                      \
> +        __builtin_constant_p(a) && __builtin_constant_p(b),     \
> +        (a) < (b) ? (a) : (b),                                  \
> +        ((void)0))
> +# define MAX_CONST(a, b)                                        \
>      __builtin_choose_expr(                                      \
>          __builtin_constant_p(a) && __builtin_constant_p(b),     \
>          (a) > (b) ? (a) : (b),                                  \
>          ((void)0))
> +#endif
> 
>  /*
>   * Minimum function that returns zero only if both values are zero.
> 

Queued, thanks.


Re: [PATCH v2] coverity: provide Coverity-friendly MIN_CONST and MAX_CONST

Posted by Philippe Mathieu-Daudé 1 week ago
On 6/29/20 6:28 PM, Eric Blake wrote:
> Coverity has problems seeing through __builtin_choose_expr, which
> result in it abandoning analysis of later functions that utilize a
> definition that used MIN_CONST or MAX_CONST, such as in qemu-file.c:
> 
>  50    DECLARE_BITMAP(may_free, MAX_IOV_SIZE);
> 
> CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1.
> expr_not_constant: expression must have a constant value
> 
> As has been done in the past (see 07d66672), it's okay to dumb things
> down when compiling for static analyzers.  (Of course, now the
> syntax-checker has a false positive on our reference to
> __COVERITY__...)
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: CID 1429992, CID 1429995, CID 1429997, CID 1429999
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
> 
> Improvements over Paolo's v1:
> - proper use of ()
> - add comment explaining the COVERITY section
> - add indentation for easier read of #if/#else flow
> 
>  include/qemu/osdep.h | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 0d26a1b9bd07..0fc206ae6154 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -250,7 +250,8 @@ extern int daemon(int, int);
>   * Note that neither form is usable as an #if condition; if you truly
>   * need to write conditional code that depends on a minimum or maximum
>   * determined by the pre-processor instead of the compiler, you'll
> - * have to open-code it.
> + * have to open-code it.  Sadly, Coverity is severely confused by the
> + * constant variants, so we have to dumb things down there.
>   */
>  #undef MIN
>  #define MIN(a, b)                                       \
> @@ -258,22 +259,28 @@ extern int daemon(int, int);
>          typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>          _a < _b ? _a : _b;                              \
>      })
> -#define MIN_CONST(a, b)                                         \
> -    __builtin_choose_expr(                                      \
> -        __builtin_constant_p(a) && __builtin_constant_p(b),     \
> -        (a) < (b) ? (a) : (b),                                  \
> -        ((void)0))
>  #undef MAX
>  #define MAX(a, b)                                       \
>      ({                                                  \
>          typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>          _a > _b ? _a : _b;                              \
>      })
> -#define MAX_CONST(a, b)                                         \
> +
> +#ifdef __COVERITY__
> +# define MIN_CONST(a, b) ((a) < (b) ? (a) : (b))
> +# define MAX_CONST(a, b) ((a) > (b) ? (a) : (b))
> +#else
> +# define MIN_CONST(a, b)                                        \
> +    __builtin_choose_expr(                                      \
> +        __builtin_constant_p(a) && __builtin_constant_p(b),     \
> +        (a) < (b) ? (a) : (b),                                  \
> +        ((void)0))
> +# define MAX_CONST(a, b)                                        \
>      __builtin_choose_expr(                                      \
>          __builtin_constant_p(a) && __builtin_constant_p(b),     \
>          (a) > (b) ? (a) : (b),                                  \
>          ((void)0))
> +#endif
> 
>  /*
>   * Minimum function that returns zero only if both values are zero.
>