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

Paolo Bonzini posted 1 patch 1 week ago
Test FreeBSD passed
Test docker-quick@centos7 passed
Test checkpatch failed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200629151642.11974-1-pbonzini@redhat.com
include/qemu/osdep.h | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)

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

Posted by Paolo Bonzini 1 week ago
Coverity's parser chokes on __builtin_choose_expr inside a constant
expression.  Since it is used only to raise compilation errors for
non-constant arguments, we can just assume there are no such errors
in the Coverity runs, and define MIN_CONST and MAX_CONST to the
"classic" ternary-operator-based definitions of minimum and maximum.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/osdep.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 0d26a1b9bd..4f4d4a3060 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -258,22 +258,29 @@ 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;                              \
     })
+
+#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.26.2


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

Posted by no-reply@patchew.org 1 week ago
Patchew URL: https://patchew.org/QEMU/20200629151642.11974-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH] coverity: provide Coverity-friendly MIN_CONST and MAX_CONST
Type: series
Message-id: 20200629151642.11974-1-pbonzini@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200629140938.17566-1-drjones@redhat.com -> patchew/20200629140938.17566-1-drjones@redhat.com
 - [tag update]      patchew/20200629151642.11974-1-pbonzini@redhat.com -> patchew/20200629151642.11974-1-pbonzini@redhat.com
Switched to a new branch 'test'
b4e52a9 coverity: provide Coverity-friendly MIN_CONST and MAX_CONST

=== OUTPUT BEGIN ===
WARNING: architecture specific defines should be avoided
#39: FILE: include/qemu/osdep.h:269:
+#ifdef __COVERITY__

ERROR: Macros with complex values should be enclosed in parenthesis
#40: FILE: include/qemu/osdep.h:270:
+#define MIN_CONST(a, b) (a) < (b) ? (a) : (b)

ERROR: Macros with complex values should be enclosed in parenthesis
#41: FILE: include/qemu/osdep.h:271:
+#define MAX_CONST(a, b) (a) > (b) ? (a) : (b)

total: 2 errors, 1 warnings, 34 lines checked

Commit b4e52a97baed (coverity: provide Coverity-friendly MIN_CONST and MAX_CONST) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200629151642.11974-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

Posted by Eric Blake 1 week ago
On 6/29/20 10:16 AM, Paolo Bonzini wrote:
> Coverity's parser chokes on __builtin_choose_expr inside a constant
> expression.  Since it is used only to raise compilation errors for
> non-constant arguments, we can just assume there are no such errors
> in the Coverity runs, and define MIN_CONST and MAX_CONST to the
> "classic" ternary-operator-based definitions of minimum and maximum.
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/qemu/osdep.h | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

I wrote a variant in the meantime, and in comparing the two, the only 
major difference was that I added a line:

     Fixes: CID 1429992, CID 1429995, CID 1429997, CID 1429999

in the commit message, as well as a comment in osdep.h:

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 0d26a1b9bd07..98bc7156fa9b 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)                                       \


I'm fine whether or not we include that.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Posted by Eric Blake 1 week ago
On 6/29/20 10:34 AM, Eric Blake wrote:
> On 6/29/20 10:16 AM, Paolo Bonzini wrote:
>> Coverity's parser chokes on __builtin_choose_expr inside a constant
>> expression.  Since it is used only to raise compilation errors for
>> non-constant arguments, we can just assume there are no such errors
>> in the Coverity runs, and define MIN_CONST and MAX_CONST to the
>> "classic" ternary-operator-based definitions of minimum and maximum.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   include/qemu/osdep.h | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I wrote a variant in the meantime, and in comparing the two, the only 
> major difference was that I added a line:
> 
>      Fixes: CID 1429992, CID 1429995, CID 1429997, CID 1429999
> 
> in the commit message, as well as a comment in osdep.h:
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 0d26a1b9bd07..98bc7156fa9b 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)                                       \
> 
> 

Oh, and the more significant difference that the syntax checker is 
right: we want the fallback definition to be enclosed in an outer (). 
Looks like we need v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Posted by Peter Maydell 1 week ago
On Mon, 29 Jun 2020 at 16:34, Eric Blake <eblake@redhat.com> wrote:
>
> On 6/29/20 10:16 AM, Paolo Bonzini wrote:
> > Coverity's parser chokes on __builtin_choose_expr inside a constant
> > expression.  Since it is used only to raise compilation errors for
> > non-constant arguments, we can just assume there are no such errors
> > in the Coverity runs, and define MIN_CONST and MAX_CONST to the
> > "classic" ternary-operator-based definitions of minimum and maximum.
> >
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   include/qemu/osdep.h | 17 ++++++++++++-----
> >   1 file changed, 12 insertions(+), 5 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I wrote a variant in the meantime, and in comparing the two, the only
> major difference was that I added a line:
>
>      Fixes: CID 1429992, CID 1429995, CID 1429997, CID 1429999
>
> in the commit message, as well as a comment in osdep.h:
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 0d26a1b9bd07..98bc7156fa9b 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)                                       \
>
>
> I'm fine whether or not we include that.

I would vote for including the comment improvement, please.

-- PMM

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

Posted by no-reply@patchew.org 1 week ago
Patchew URL: https://patchew.org/QEMU/20200629151642.11974-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH] coverity: provide Coverity-friendly MIN_CONST and MAX_CONST
Type: series
Message-id: 20200629151642.11974-1-pbonzini@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200629151642.11974-1-pbonzini@redhat.com -> patchew/20200629151642.11974-1-pbonzini@redhat.com
Switched to a new branch 'test'
3aee0de coverity: provide Coverity-friendly MIN_CONST and MAX_CONST

=== OUTPUT BEGIN ===
WARNING: architecture specific defines should be avoided
#38: FILE: include/qemu/osdep.h:269:
+#ifdef __COVERITY__

ERROR: Macros with complex values should be enclosed in parenthesis
#39: FILE: include/qemu/osdep.h:270:
+#define MIN_CONST(a, b) (a) < (b) ? (a) : (b)

ERROR: Macros with complex values should be enclosed in parenthesis
#40: FILE: include/qemu/osdep.h:271:
+#define MAX_CONST(a, b) (a) > (b) ? (a) : (b)

total: 2 errors, 1 warnings, 34 lines checked

Commit 3aee0de30ad2 (coverity: provide Coverity-friendly MIN_CONST and MAX_CONST) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200629151642.11974-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

Posted by Eric Blake 1 week ago
On 6/29/20 10:21 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200629151642.11974-1-pbonzini@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [PATCH] coverity: provide Coverity-friendly MIN_CONST and MAX_CONST
> Type: series
> Message-id: 20200629151642.11974-1-pbonzini@redhat.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
>  From https://github.com/patchew-project/qemu
>   * [new tag]         patchew/20200629151642.11974-1-pbonzini@redhat.com -> patchew/20200629151642.11974-1-pbonzini@redhat.com
> Switched to a new branch 'test'
> 3aee0de coverity: provide Coverity-friendly MIN_CONST and MAX_CONST
> 
> === OUTPUT BEGIN ===
> WARNING: architecture specific defines should be avoided
> #38: FILE: include/qemu/osdep.h:269:
> +#ifdef __COVERITY__
> 
> ERROR: Macros with complex values should be enclosed in parenthesis

Obvious false positive.

> #39: FILE: include/qemu/osdep.h:270:
> +#define MIN_CONST(a, b) (a) < (b) ? (a) : (b)
> 
> ERROR: Macros with complex values should be enclosed in parenthesis

But this one is a real complaint.  We really do want:

#define MIN_CONST(a, b) ((a) < (b) ? (a) : (b))

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org