[Qemu-devel] [PATCH] Remove left shifts of negative signed integers

[Qemu-devel] [PATCH] Remove left shifts of negative signed integers

[John Snow]

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 <jsnow@redhat.com>
---
 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

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

[Peter Maydell]

On 30 June 2016 at 20:36, John Snow <jsnow@redhat.com> wrote:
> 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 <jsnow@redhat.com>
> ---
>  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))

We have been down this path before:
http://patchwork.ozlabs.org/patch/545238/

Paolo will doubtless be along with the rant shortly.

-- PMM

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

[John Snow]

On 06/30/2016 04:11 PM, Peter Maydell wrote:
> On 30 June 2016 at 20:36, John Snow <jsnow@redhat.com> wrote:
>> 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 <jsnow@redhat.com>
>> ---
>>  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))
> 
> We have been down this path before:
> http://patchwork.ozlabs.org/patch/545238/
> 
> Paolo will doubtless be along with the rant shortly.
> 
> -- PMM
> 

I figured, so I CC'd him. Nobody can say I didn't try.

--js

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

[Paolo Bonzini]

> > We have been down this path before:
> > http://patchwork.ozlabs.org/patch/545238/
> > 
> > Paolo will doubtless be along with the rant shortly.

Here I am!

Paolo

> I figured, so I CC'd him. Nobody can say I didn't try.
> 
> --js
> 

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

[Paolo Bonzini]

----- Original Message -----
> From: "John Snow" <jsnow@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: qemu-trivial@nongnu.org, pbonzini@redhat.com, kraxel@redhat.com, "peter maydell" <peter.maydell@linaro.org>,
> "John Snow" <jsnow@redhat.com>
> 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 <jsnow@redhat.com>

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
> 
> 

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

[John Snow]

On 06/30/2016 04:15 PM, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
>> From: "John Snow" <jsnow@redhat.com>
>> To: qemu-devel@nongnu.org
>> Cc: qemu-trivial@nongnu.org, pbonzini@redhat.com, kraxel@redhat.com, "peter maydell" <peter.maydell@linaro.org>,
>> "John Snow" <jsnow@redhat.com>
>> 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 <jsnow@redhat.com>
> 
> 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
> 

Where /exactly/ do you propose we canonicalize the clang invocation you
would like to support?

--js

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

[Paolo Bonzini]

> > 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.
> 
> Where /exactly/ do you propose we canonicalize the clang invocation you
> would like to support?

In configure where we look for other warning flags.  Add -fwrapv and
-Wno-shift-negative-value so that they're enabled if the compilers
supports them.

Paolo

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

[Peter Maydell]

On 30 June 2016 at 21:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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.

James Molloy kindly wrote a patch for clang which fixes PR25552
(so -fwrapv silences the compiler warning and the sanitizer
warning), and that has now been committed to clang. So I'm now
happy that both clang and gcc agree that -fwrapv is the way to
give you a well-defined C dialect with the shift semantics we
want. Post-2.7 we should add -fwrapv to our compiler flags
(we'll want to keep -Wno-shift-negative-value for the benefit
of clang versions without the 25552 bugfix).

thanks
-- PMM