On Thu, Jun 25, 2020 at 6:36 PM Philippe Mathieu-Daudé <phi...@redhat.com> wrote: > On 6/25/20 6:26 PM, Eric Blake wrote: > > 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 <ebl...@redhat.com> > > > > Nice! (btw why extra line after s-o-b?) > > Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com> > > > --- > > > > v2 was: https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00727.html > > v3: avoid __auto_type [Richard], document other approaches that fail > > [Dave], rebase to master > > v4: use ((void)0) instead of __builtin_unreachable [Dave], update comments > > v5: fix typo, comment formatting, one more __builtin_unreachable > > --- > > 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 ++++++++++++++++++++++++++++++++------- > > accel/tcg/translate-all.c | 6 ++--- > > migration/qemu-file.c | 2 +- > > 7 files changed, 63 insertions(+), 23 deletions(-)
