[Qemu-devel] [PATCH] hw/audio/fmopl.c: Avoid clang warning about shifting negative number

[Qemu-devel] [PATCH] hw/audio/fmopl.c: Avoid clang warning about shifting negative number

[Peter Maydell]

Newer versions of clang warn:

hw/audio/fmopl.c:1085:39: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
                data = Limit( outd[0] , OPL_MAXOUT, OPL_MINOUT );
                                                    ^~~~~~~~~~
hw/audio/fmopl.c:75:28: note: expanded from macro 'OPL_MINOUT'
#define OPL_MINOUT (-0x8000<<OPL_OUTSB)
                    ~~~~~~~^

Rephrase the definition of OPL_MINOUT to avoid the undefined behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/audio/fmopl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index 81c0c1b..807b29c 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -72,7 +72,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 --------------------- */
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH] hw/audio/fmopl.c: Avoid clang warning about shifting negative number

[Paolo Bonzini]

On 16/11/2015 16:16, Peter Maydell wrote:
> Newer versions of clang warn:
> 
> hw/audio/fmopl.c:1085:39: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>                 data = Limit( outd[0] , OPL_MAXOUT, OPL_MINOUT );
>                                                     ^~~~~~~~~~
> hw/audio/fmopl.c:75:28: note: expanded from macro 'OPL_MINOUT'
> #define OPL_MINOUT (-0x8000<<OPL_OUTSB)
>                     ~~~~~~~^
> 
> Rephrase the definition of OPL_MINOUT to avoid the undefined behaviour.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/audio/fmopl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index 81c0c1b..807b29c 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -72,7 +72,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))

Again: let's stop this madness!!!!!!!!!!!!!!!!!!!!!!

(Yes, so many exclamation marks).

This is clearly computing -32768 * 2^N, not -(32768 * 2^N).  The latter
is totally, utterly wrong, because 32768 is _not even expressible_ as a
16-bit fixed point value, which OPL_MINOUT/OPL_MAXOUT obviously are.

I'll shortly post a patch to disable this obnoxious warning.

Paolo

Re: [Qemu-devel] [PATCH] hw/audio/fmopl.c: Avoid clang warning about shifting negative number

[Peter Maydell]

On 17 November 2015 at 09:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Again: let's stop this madness!!!!!!!!!!!!!!!!!!!!!!
>
> (Yes, so many exclamation marks).
>
> This is clearly computing -32768 * 2^N, not -(32768 * 2^N).  The latter
> is totally, utterly wrong, because 32768 is _not even expressible_ as a
> 16-bit fixed point value, which OPL_MINOUT/OPL_MAXOUT obviously are.
>
> I'll shortly post a patch to disable this obnoxious warning.

This is undefined behaviour by the language spec. If clang is
warning about it they obviously don't want to guarantee that they
aren't ever going to rely on this UB for optimisation. I agree
that it's a silly thing to have be UB, but that's what C is.
I don't think we should be silencing this warning (or the
runtime ubsan one) and I do think we should be fixing our UBs.

I agree that the rephrasing isn't great; if you have a
preferred non-UB way to write it I'm open to suggestions.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/audio/fmopl.c: Avoid clang warning about shifting negative number

[Paolo Bonzini]

On 17/11/2015 11:17, Peter Maydell wrote:
> If clang is
> warning about it they obviously don't want to guarantee that they
> aren't ever going to rely on this UB for optimisation.

My interpretation is just that clang's diagnostics are not that much
better than GCC anymore, and they feel the need to pump their release
notes with oh so many new -W flags.

Just look at the 3.5 release notes:

   New warning -Wabsolute-value: Clang warns about incorrect or useless
   usage of the absolute functions (abs, fabsf, etc).

   #include <stdlib.h>
   void foo() {
     unsigned int i=0;
     abs(i);
   }

   returns warning: taking the absolute value of unsigned type
   ‘unsigned int’ has no effect [-Wabsolute-value]

Wow, haven't you wanted that since kindergarten? :-D

There's no reason not to put this under the existing -Wunused-value,
other than politics.

> I agree that the rephrasing isn't great; if you have a
> preferred non-UB way to write it I'm open to suggestions.

I don't think there's a good way to express it.  I guess -0x8000u would
look mildly better, but it is more dangerous so I think overall it's
even worse.

The point is that left shifting of a negative number _is not overflow_
unless that number is less than ("has fewer leading ones than") INT_MIN
>> n.  It makes no sense to forbid anything else, if at the same time
you allow signed right shift to be arithmetic shift.

I'm all okay with compilers treating shifts of negative numbers as
undefined behavior if a 0 is shifted into the sign bit.

Paolo