From: Eric Blake <eblake@redhat.com>
I'm not aware of any immediate bugs in qemu where a second runtime
evaluation of the arguments to MIN() or MAX() causes a problem, but
proactively preventing such abuse is easier than falling prey to an
unintended case down the road. At any rate, here's the conversation
that sparked the current patch:
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html
Update the MIN/MAX macros to only evaluate their argument once at
runtime; this uses typeof(1 ? (a) : (b)) to ensure that we are
promoting the temporaries to the same type as the final comparison (we
have to trigger type promotion, as typeof(bitfield) won't compile; and
we can't use typeof((a) + (b)) or even typeof((a) + 0), as some of our
uses of MAX are on void* pointers where such addition is undefined).
However, we are unable to work around gcc refusing to compile ({}) in
a constant context (such as the array length of a static variable),
even when only used in the dead branch of a __builtin_choose_expr(),
so we have to provide a second macro pair MIN_CONST and MAX_CONST for
use when both arguments are known to be compile-time constants and
where the result must also be usable as a constant; this second form
evaluates arguments multiple times but that doesn't matter for
constants. By using a void expression as the expansion if a
non-constant is presented to this second form, we can enlist the
compiler to ensure the double evaluation is not attempted on
non-constants.
Alas, as both macros now rely on compiler intrinsics, they are no
longer usable in preprocessor #if conditions; those will just have to
be open-coded or the logic rewritten into #define or runtime 'if'
conditions (but where the compiler dead-code-elimination will probably
still apply).
I tested that both gcc 10.1.1 and clang 10.0.0 produce errors for all
forms of macro mis-use. As the errors can sometimes be cryptic, I'm
demonstrating the gcc output:
Use of MIN when MIN_CONST is needed:
In file included from /home/eblake/qemu/qemu-img.c:25:
/home/eblake/qemu/include/qemu/osdep.h:249:5: error: braced-group within expression allowed only inside a function
249 | ({ \
| ^
/home/eblake/qemu/qemu-img.c:92:12: note: in expansion of macro ‘MIN’
92 | char array[MIN(1, 2)] = "";
| ^~~
Use of MIN_CONST when MIN is needed:
/home/eblake/qemu/qemu-img.c: In function ‘is_allocated_sectors’:
/home/eblake/qemu/qemu-img.c:1225:15: error: void value not ignored as it ought to be
1225 | i = MIN_CONST(i, n);
| ^
Use of MIN in the preprocessor:
In file included from /home/eblake/qemu/accel/tcg/translate-all.c:20:
/home/eblake/qemu/accel/tcg/translate-all.c: In function ‘page_check_range’:
/home/eblake/qemu/include/qemu/osdep.h:249:6: error: token "{" is not valid in preprocessor expressions
249 | ({ \
| ^
Fix the resulting callsites that used #if or computed a compile-time
constant min or max to use the new macros. cpu-defs.h is interesting,
as CPU_TLB_DYN_MAX_BITS is sometimes used as a constant and sometimes
dynamic.
It may be worth improving glib's MIN/MAX definitions to be saner, but
that is a task for another day.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200625162602.700741-1-eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
accel/tcg/translate-all.c | 6 ++---
hw/usb/hcd-xhci.h | 2 +-
include/block/block.h | 4 +--
include/exec/cpu-all.h | 8 +++---
include/exec/cpu-defs.h | 7 ++++-
include/qemu/osdep.h | 57 ++++++++++++++++++++++++++++++++-------
migration/qemu-file.c | 2 +-
7 files changed, 63 insertions(+), 23 deletions(-)
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index c3d37058a1..2afa46bd2b 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2582,9 +2582,9 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
/* This function should never be called with addresses outside the
guest address space. If this assert fires, it probably indicates
a missing call to h2g_valid. */
-#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
- assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
-#endif
+ if (TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS) {
+ assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
+ }
if (len == 0) {
return 0;
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index 2fad4df2a7..946af51fc2 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -214,7 +214,7 @@ struct XHCIState {
uint32_t dcbaap_high;
uint32_t config;
- USBPort uports[MAX(MAXPORTS_2, MAXPORTS_3)];
+ USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)];
XHCIPort ports[MAXPORTS];
XHCISlot slots[MAXSLOTS];
uint32_t numports;
diff --git a/include/block/block.h b/include/block/block.h
index 25e299605e..e8fc814996 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -133,8 +133,8 @@ typedef struct HDGeometry {
#define BDRV_SECTOR_BITS 9
#define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS)
-#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
- INT_MAX >> BDRV_SECTOR_BITS)
+#define BDRV_REQUEST_MAX_SECTORS MIN_CONST(SIZE_MAX >> BDRV_SECTOR_BITS, \
+ INT_MAX >> BDRV_SECTOR_BITS)
#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
/*
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index fb4e8a8e29..fc403d456b 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -176,11 +176,9 @@ extern unsigned long reserved_va;
* avoid setting bits at the top of guest addresses that might need
* to be used for tags.
*/
-#if MIN(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32
-# define GUEST_ADDR_MAX_ UINT32_MAX
-#else
-# define GUEST_ADDR_MAX_ (~0ul)
-#endif
+#define GUEST_ADDR_MAX_ \
+ ((MIN_CONST(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32) ? \
+ UINT32_MAX : ~0ul)
#define GUEST_ADDR_MAX (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_)
#else
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 8c44abefa2..9185632337 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -102,8 +102,13 @@ typedef uint64_t target_ulong;
* Skylake's Level-2 STLB has 16 1G entries.
* Also, make sure we do not size the TLB past the guest's address space.
*/
-# define CPU_TLB_DYN_MAX_BITS \
+# ifdef TARGET_PAGE_BITS_VARY
+# define CPU_TLB_DYN_MAX_BITS \
MIN(22, TARGET_VIRT_ADDR_SPACE_BITS - TARGET_PAGE_BITS)
+# else
+# define CPU_TLB_DYN_MAX_BITS \
+ MIN_CONST(22, TARGET_VIRT_ADDR_SPACE_BITS - TARGET_PAGE_BITS)
+# endif
# endif
typedef struct CPUTLBEntry {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ff7c17b857..0d26a1b9bd 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -236,18 +236,55 @@ extern int daemon(int, int);
#define SIZE_MAX ((size_t)-1)
#endif
-#ifndef MIN
-#define MIN(a, b) (((a) < (b)) ? (a) : (b))
-#endif
-#ifndef MAX
-#define MAX(a, b) (((a) > (b)) ? (a) : (b))
-#endif
+/*
+ * Two variations of MIN/MAX macros. The first is for runtime use, and
+ * evaluates arguments only once (so it is safe even with side
+ * effects), but will not work in constant contexts (such as array
+ * size declarations) because of the '{}'. The second is for constant
+ * expression use, where evaluating arguments twice is safe because
+ * the result is going to be constant anyway, but will not work in a
+ * runtime context because of a void expression where a value is
+ * expected. Thus, both gcc and clang will fail to compile if you use
+ * the wrong macro (even if the error may seem a bit cryptic).
+ *
+ * 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.
+ */
+#undef MIN
+#define MIN(a, b) \
+ ({ \
+ 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) \
+ __builtin_choose_expr( \
+ __builtin_constant_p(a) && __builtin_constant_p(b), \
+ (a) > (b) ? (a) : (b), \
+ ((void)0))
-/* Minimum function that returns zero only iff both values are zero.
- * Intended for use with unsigned values only. */
+/*
+ * Minimum function that returns zero only if both values are zero.
+ * Intended for use with unsigned values only.
+ */
#ifndef MIN_NON_ZERO
-#define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \
- ((b) == 0 ? (a) : (MIN(a, b))))
+#define MIN_NON_ZERO(a, b) \
+ ({ \
+ typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
+ _a == 0 ? _b : (_b == 0 || _b > _a) ? _a : _b; \
+ })
#endif
/* Round number down to multiple */
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 1c3a358a14..be21518c57 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -31,7 +31,7 @@
#include "qapi/error.h"
#define IO_BUF_SIZE 32768
-#define MAX_IOV_SIZE MIN(IOV_MAX, 64)
+#define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
struct QEMUFile {
const QEMUFileOps *ops;
--
2.26.2
On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Eric Blake <eblake@redhat.com>
>
> I'm not aware of any immediate bugs in qemu where a second runtime
> evaluation of the arguments to MIN() or MAX() causes a problem, but
> proactively preventing such abuse is easier than falling prey to an
> unintended case down the road. At any rate, here's the conversation
> that sparked the current patch:
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html
Hi; the changes in this patch seem to confuse Coverity.
> +#undef MIN
> +#define MIN(a, b) \
> + ({ \
> + 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) \
> + __builtin_choose_expr( \
> + __builtin_constant_p(a) && __builtin_constant_p(b), \
> + (a) > (b) ? (a) : (b), \
> + ((void)0))
In particular, where MIN_CONST or MAX_CONST are used to
define values that must be const, eg in qemu-file.c:
50 DECLARE_BITMAP(may_free, MAX_IOV_SIZE);
or in hcd-xhci.h:
217 USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)];
Coverity reports:
CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1.
expr_not_constant: expression must have a constant value
Can we do something (eg providing fallback less-intelligent
versions of the macro ifdef __COVERITY__) to help it?
(This is the cause of CID 1429992, 1429995, 1429997,
1429999. Parse errors are unfortunate because Coverity
abandons analysis of the affected function entirely,
and analysis of its callers is also limited.)
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> From: Eric Blake <eblake@redhat.com>
>>
>> I'm not aware of any immediate bugs in qemu where a second runtime
>> evaluation of the arguments to MIN() or MAX() causes a problem, but
>> proactively preventing such abuse is easier than falling prey to an
>> unintended case down the road. At any rate, here's the conversation
>> that sparked the current patch:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html
>
> Hi; the changes in this patch seem to confuse Coverity.
>
>> +#undef MIN
>> +#define MIN(a, b) \
>> + ({ \
>> + 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) \
>> + __builtin_choose_expr( \
>> + __builtin_constant_p(a) && __builtin_constant_p(b), \
>> + (a) > (b) ? (a) : (b), \
>> + ((void)0))
>
> In particular, where MIN_CONST or MAX_CONST are used to
> define values that must be const, eg in qemu-file.c:
> 50 DECLARE_BITMAP(may_free, MAX_IOV_SIZE);
> or in hcd-xhci.h:
> 217 USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)];
>
> Coverity reports:
>
> CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1.
> expr_not_constant: expression must have a constant value
>
> Can we do something (eg providing fallback less-intelligent
> versions of the macro ifdef __COVERITY__) to help it?
Perhaps we can solve the issue with scripts/coverity-model.c.
Unfortunately, I can't spare the time to try right now.
> (This is the cause of CID 1429992, 1429995, 1429997,
> 1429999. Parse errors are unfortunate because Coverity
> abandons analysis of the affected function entirely,
> and analysis of its callers is also limited.)
Bummer. I recommend to revert until we figure out how not to break
Coverity.
On 6/27/20 4:35 PM, Peter Maydell wrote: > On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> From: Eric Blake <eblake@redhat.com> >> >> I'm not aware of any immediate bugs in qemu where a second runtime >> evaluation of the arguments to MIN() or MAX() causes a problem, but >> proactively preventing such abuse is easier than falling prey to an >> unintended case down the road. At any rate, here's the conversation >> that sparked the current patch: >> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html > > Hi; the changes in this patch seem to confuse Coverity. Oh dear. > In particular, where MIN_CONST or MAX_CONST are used to > define values that must be const, eg in qemu-file.c: > 50 DECLARE_BITMAP(may_free, MAX_IOV_SIZE); > or in hcd-xhci.h: > 217 USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)]; > > Coverity reports: > > CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1. > expr_not_constant: expression must have a constant value > > Can we do something (eg providing fallback less-intelligent > versions of the macro ifdef __COVERITY__) to help it? Absolutely; I see we've done similar in include/qemu/thread.h. I'll post something later today. > > (This is the cause of CID 1429992, 1429995, 1429997, > 1429999. Parse errors are unfortunate because Coverity > abandons analysis of the affected function entirely, > and analysis of its callers is also limited.) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 29/06/20 17:23, Eric Blake wrote: >> >> >> Can we do something (eg providing fallback less-intelligent >> versions of the macro ifdef __COVERITY__) to help it? > > Absolutely; I see we've done similar in include/qemu/thread.h. I'll > post something later today. Done already, you're in Cc. :) Paolo
© 2016 - 2026 Red Hat, Inc.