Re: [Qemu-trivial] [PATCH] Remove left shifts of negative signed integers

[Paolo Bonzini]

----- Original Message -----
> From: "John Snow" <address@hidden>
> To: address@hidden
> Cc: address@hidden, address@hidden, address@hidden, "peter maydell" 
> <address@hidden>,
> "John Snow" <address@hidden>
> Sent: Thursday, June 30, 2016 9:36:36 PM
> Subject: [PATCH] Remove left shifts of negative signed integers
> 
> Another exercise in placating Clang's increasingly strict -Werror mode.
> Technically, this is undefined behavior. In practice, -N<<M is the same
> as -(N<<M).
> 
> Signed-off-by: John Snow <address@hidden>

There's been discussions on this in the past; sorry but this is a
super-duper NACK.

GCC correctly puts this warning under -Wextra, and promises not to ever
make use of this facet of undefined behavior.  The only correct patch
is the one that disables the warning for clang, and possibly adds
-fwrapv.  In GCC, -fwrapv correctly silences ubsan's left-shift
and signed-overflow warnings.  In Clang, this is reported at
https://llvm.org/bugs/show_bug.cgi?id=25552.  It's a heavy hammer
but it's the safest options as compiler evolve.

Paolo

> ---
>  hw/audio/fmopl.c      | 2 +-
>  target-i386/monitor.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index 731110f..de9338b 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -69,7 +69,7 @@ static int opl_dbg_maxchip,opl_dbg_chip;
>  /* final output shift , limit minimum and maximum */
>  #define OPL_OUTSB   (TL_BITS+3-16)           /* OPL output final shift 16bit 
> */
>  #define OPL_MAXOUT (0x7fff<<OPL_OUTSB)
> -#define OPL_MINOUT (-0x8000<<OPL_OUTSB)
> +#define OPL_MINOUT (-(0x8000<<OPL_OUTSB))
>  
>  /* -------------------- quality selection --------------------- */
>  
> diff --git a/target-i386/monitor.c b/target-i386/monitor.c
> index fccfe40..94e9871 100644
> --- a/target-i386/monitor.c
> +++ b/target-i386/monitor.c
> @@ -36,7 +36,7 @@ static void print_pte(Monitor *mon, hwaddr addr,
>  {
>  #ifdef TARGET_X86_64
>      if (addr & (1ULL << 47)) {
> -        addr |= -1LL << 48;
> +        addr |= -(1LL << 48);
>      }
>  #endif
>      monitor_printf(mon, TARGET_FMT_plx ": " TARGET_FMT_plx
> --
> 2.5.5
> 
>