[Qemu-devel] [PATCH 0/3] fix clang negative signed bit shift warning

[Qemu-devel] [PATCH 0/3] fix clang negative signed bit shift warning

[Stefan Hajnoczi]

LLVM's clang 3.7.0 compile warns about bit shifting negative numbers because
the result is undefined.  This series includes 3 small fixes to appease clang.

Stefan Hajnoczi (3):
  monitor: avoid clang shifting negative signed warning
  tpm: avoid clang shifting negative signed warning
  disas/arm: avoid clang shifting negative signed warning

 disas/arm.c           | 2 +-
 hw/tpm/tpm_tis.c      | 2 +-
 target-i386/monitor.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/3] monitor: avoid clang shifting negative signed warning

[Stefan Hajnoczi]

clang 3.7.0 on x86_64 warns about the following:

  target-i386/monitor.c:38:22: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
        addr |= -1LL << 48;
                ~~~~ ^

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 target-i386/monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/monitor.c b/target-i386/monitor.c
index aac6b1b..6f5c280 100644
--- a/target-i386/monitor.c
+++ b/target-i386/monitor.c
@@ -35,7 +35,7 @@ static void print_pte(Monitor *mon, hwaddr addr,
 {
 #ifdef TARGET_X86_64
     if (addr & (1ULL << 47)) {
-        addr |= -1LL << 48;
+        addr |= ~0ULL << 48;
     }
 #endif
     monitor_printf(mon, TARGET_FMT_plx ": " TARGET_FMT_plx
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/3] monitor: avoid clang shifting negative signed warning

[Eduardo Habkost]

On Tue, Nov 10, 2015 at 03:57:33PM +0000, Stefan Hajnoczi wrote:
> clang 3.7.0 on x86_64 warns about the following:
> 
>   target-i386/monitor.c:38:22: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>         addr |= -1LL << 48;
>                 ~~~~ ^
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Applied to x86 tree. Thanks!

-- 
Eduardo

[Qemu-devel] [PATCH 2/3] tpm: avoid clang shifting negative signed warning

[Stefan Hajnoczi]

clang 3.7.0 on x86_64 warns about the following:

  hw/tpm/tpm_tis.c:1000:36: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
            tis->loc[c].iface_id = TPM_TIS_IFACE_ID_SUPPORTED_FLAGS1_3;
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  hw/tpm/tpm_tis.c:144:10: note: expanded from macro 'TPM_TIS_IFACE_ID_SUPPORTED_FLAGS1_3'
     (~0 << 4)/* all of it is don't care */)
      ~~ ^

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/tpm/tpm_tis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 0806b5f..ff073d5 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -141,7 +141,7 @@
 
 #define TPM_TIS_IFACE_ID_SUPPORTED_FLAGS1_3 \
     (TPM_TIS_IFACE_ID_INTERFACE_TIS1_3 | \
-     (~0 << 4)/* all of it is don't care */)
+     (~0u << 4)/* all of it is don't care */)
 
 /* if backend was a TPM 2.0: */
 #define TPM_TIS_IFACE_ID_SUPPORTED_FLAGS2_0 \
-- 
2.5.0

[Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Stefan Hajnoczi]

clang 3.7.0 on x86_64 warns about the following:

  disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
    imm |= (-1 << 7);
            ~~ ^

Note that this patch preserves the tab indent in this source file
because the surrounding code still uses tabs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 disas/arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disas/arm.c b/disas/arm.c
index 6165246..7a7354b 100644
--- a/disas/arm.c
+++ b/disas/arm.c
@@ -1779,7 +1779,7 @@ print_insn_coprocessor (bfd_vma pc, struct disassemble_info *info, long given,
 
 			/* Is ``imm'' a negative number?  */
 			if (imm & 0x40)
-			  imm |= (-1 << 7);
+			  imm |= (~0u << 7);
 
 			func (stream, "%d", imm);
 		      }
-- 
2.5.0

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Paolo Bonzini]

On 10/11/2015 16:57, Stefan Hajnoczi wrote:
> clang 3.7.0 on x86_64 warns about the following:
> 
>   disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>     imm |= (-1 << 7);
>             ~~ ^
> 
> Note that this patch preserves the tab indent in this source file
> because the surrounding code still uses tabs.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I would like to know a case where (except with ubsan) clang actually
uses the optimization.

If not, this is just error message theatre (which is not news for clang)
and shouldn't have been part of -Wall.

Paolo

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Peter Maydell]

On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 10/11/2015 16:57, Stefan Hajnoczi wrote:
>> clang 3.7.0 on x86_64 warns about the following:
>>
>>   disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>>     imm |= (-1 << 7);
>>             ~~ ^
>>
>> Note that this patch preserves the tab indent in this source file
>> because the surrounding code still uses tabs.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> I would like to know a case where (except with ubsan) clang actually
> uses the optimization.
>
> If not, this is just error message theatre (which is not news for clang)
> and shouldn't have been part of -Wall.

It could be they're attempting to warn us now about the possibility
that in a future version of clang they will start using this UB
to optimize with.

http://stackoverflow.com/questions/22883790/left-shift-of-negative-values
reports that Intel's ICC will use this in dead-code-elimination
optimization. One day clang might do that too.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Paolo Bonzini]

On 10/11/2015 18:48, Peter Maydell wrote:
> It could be they're attempting to warn us now about the possibility
> that in a future version of clang they will start using this UB
> to optimize with.

Good luck to them.

Paolo

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 10/11/2015 16:57, Stefan Hajnoczi wrote:
>>> clang 3.7.0 on x86_64 warns about the following:
>>>
>>>   disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>>>     imm |= (-1 << 7);
>>>             ~~ ^
>>>
>>> Note that this patch preserves the tab indent in this source file
>>> because the surrounding code still uses tabs.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> I would like to know a case where (except with ubsan) clang actually
>> uses the optimization.
>>
>> If not, this is just error message theatre (which is not news for clang)
>> and shouldn't have been part of -Wall.
>
> It could be they're attempting to warn us now about the possibility
> that in a future version of clang they will start using this UB
> to optimize with.
>
> http://stackoverflow.com/questions/22883790/left-shift-of-negative-values
> reports that Intel's ICC will use this in dead-code-elimination
> optimization. One day clang might do that too.

Nice example of a compiler being gratuitously nasty.

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Steven Noonan]

On Tue, Nov 10, 2015 at 10:52 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 10/11/2015 16:57, Stefan Hajnoczi wrote:
>>>> clang 3.7.0 on x86_64 warns about the following:
>>>>
>>>>   disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>>>>     imm |= (-1 << 7);
>>>>             ~~ ^
>>>>
>>>> Note that this patch preserves the tab indent in this source file
>>>> because the surrounding code still uses tabs.
>>>>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>
>>> I would like to know a case where (except with ubsan) clang actually
>>> uses the optimization.
>>>
>>> If not, this is just error message theatre (which is not news for clang)
>>> and shouldn't have been part of -Wall.
>>
>> It could be they're attempting to warn us now about the possibility
>> that in a future version of clang they will start using this UB
>> to optimize with.
>>
>> http://stackoverflow.com/questions/22883790/left-shift-of-negative-values
>> reports that Intel's ICC will use this in dead-code-elimination
>> optimization. One day clang might do that too.
>
> Nice example of a compiler being gratuitously nasty.
>

I don't read this warning as "clang will do crazy things with your
code eventually". Clang has always been very verbose when it comes to
undefined behavior, and I don't think that's really a bad thing to do.
Even if clang does emit sane code for it, all bets are off for other
compilers -- so it's more of a portability warning. And I know some
other compilers *won't* warn before doing crazy things in the name of
undefined behavior. The ICC example is a fine one...

In my experience fixing the warnings produced by clang has actually
eliminated bugs that were present but undiscovered on other platforms.

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Markus Armbruster]

Steven Noonan <steven@uplinklabs.net> writes:

> On Tue, Nov 10, 2015 at 10:52 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>
>>>> On 10/11/2015 16:57, Stefan Hajnoczi wrote:
>>>>> clang 3.7.0 on x86_64 warns about the following:
>>>>>
>>>>>   disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>>>>>     imm |= (-1 << 7);
>>>>>             ~~ ^
>>>>>
>>>>> Note that this patch preserves the tab indent in this source file
>>>>> because the surrounding code still uses tabs.
>>>>>
>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>
>>>> I would like to know a case where (except with ubsan) clang actually
>>>> uses the optimization.
>>>>
>>>> If not, this is just error message theatre (which is not news for clang)
>>>> and shouldn't have been part of -Wall.
>>>
>>> It could be they're attempting to warn us now about the possibility
>>> that in a future version of clang they will start using this UB
>>> to optimize with.
>>>
>>> http://stackoverflow.com/questions/22883790/left-shift-of-negative-values
>>> reports that Intel's ICC will use this in dead-code-elimination
>>> optimization. One day clang might do that too.
>>
>> Nice example of a compiler being gratuitously nasty.
>>
>
> I don't read this warning as "clang will do crazy things with your
> code eventually". Clang has always been very verbose when it comes to
> undefined behavior, and I don't think that's really a bad thing to do.
[...]

Misunderstanding?

Clang's warning is at worst annoying, but nowhere near nasty.

ICC concluding that code executing a left shift of a negative value must
be unreachable is gratuitously nasty.

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Steven Noonan]

On Tue, Nov 10, 2015 at 12:06 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Steven Noonan <steven@uplinklabs.net> writes:
>
>> On Tue, Nov 10, 2015 at 10:52 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>
>>>>>
>>>>> On 10/11/2015 16:57, Stefan Hajnoczi wrote:
>>>>>> clang 3.7.0 on x86_64 warns about the following:
>>>>>>
>>>>>>   disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
>>>>>>     imm |= (-1 << 7);
>>>>>>             ~~ ^
>>>>>>
>>>>>> Note that this patch preserves the tab indent in this source file
>>>>>> because the surrounding code still uses tabs.
>>>>>>
>>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>
>>>>> I would like to know a case where (except with ubsan) clang actually
>>>>> uses the optimization.
>>>>>
>>>>> If not, this is just error message theatre (which is not news for clang)
>>>>> and shouldn't have been part of -Wall.
>>>>
>>>> It could be they're attempting to warn us now about the possibility
>>>> that in a future version of clang they will start using this UB
>>>> to optimize with.
>>>>
>>>> http://stackoverflow.com/questions/22883790/left-shift-of-negative-values
>>>> reports that Intel's ICC will use this in dead-code-elimination
>>>> optimization. One day clang might do that too.
>>>
>>> Nice example of a compiler being gratuitously nasty.
>>>
>>
>> I don't read this warning as "clang will do crazy things with your
>> code eventually". Clang has always been very verbose when it comes to
>> undefined behavior, and I don't think that's really a bad thing to do.
> [...]
>
> Misunderstanding?
>
> Clang's warning is at worst annoying, but nowhere near nasty.

Sure, it's merely annoying if you only compile your code with Clang.
But my point is that a lot of Clang's aggressive undefined behavior
warnings are there to get people to write portable code so it doesn't
break on other compilers where the "undefined behavior" depends on the
compiler implementation.

> ICC concluding that code executing a left shift of a negative value must
> be unreachable is gratuitously nasty.

I agree, that's completely insane.

Re: [Qemu-devel] [PATCH 3/3] disas/arm: avoid clang shifting negative signed warning

[Paolo Bonzini]

On 10/11/2015 20:51, Steven Noonan wrote:
> I don't read this warning as "clang will do crazy things with your
> code eventually". Clang has always been very verbose when it comes to
> undefined behavior, and I don't think that's really a bad thing to do.

Sure, but it doesn't belong in -Wall.  It's what -Wextra is for.

Paolo

Re: [Qemu-devel] [PATCH 0/3] fix clang negative signed bit shift warning

[Peter Maydell]

On 10 November 2015 at 15:57, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> LLVM's clang 3.7.0 compile warns about bit shifting negative numbers because
> the result is undefined.  This series includes 3 small fixes to appease clang.
>
> Stefan Hajnoczi (3):
>   monitor: avoid clang shifting negative signed warning
>   tpm: avoid clang shifting negative signed warning
>   disas/arm: avoid clang shifting negative signed warning
>
>  disas/arm.c           | 2 +-
>  hw/tpm/tpm_tis.c      | 2 +-
>  target-i386/monitor.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

Whole series
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] fix clang negative signed bit shift warning

[Peter Maydell]

On 10 November 2015 at 15:57, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> LLVM's clang 3.7.0 compile warns about bit shifting negative numbers because
> the result is undefined.  This series includes 3 small fixes to appease clang.
>
> Stefan Hajnoczi (3):
>   monitor: avoid clang shifting negative signed warning
>   tpm: avoid clang shifting negative signed warning
>   disas/arm: avoid clang shifting negative signed warning
>
>  disas/arm.c           | 2 +-
>  hw/tpm/tpm_tis.c      | 2 +-
>  target-i386/monitor.c | 2 +-

My clang-3.7 build (it's part of fbinfer) also complains about:

/Users/pm215/src/qemu/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 );
                                                    ^~~~~~~~~~
/Users/pm215/src/qemu/hw/audio/fmopl.c:75:28: note: expanded from
macro 'OPL_MINOUT'
#define OPL_MINOUT (-0x8000<<OPL_OUTSB)
                    ~~~~~~~^

and a lot of shift-of-negative uses in target-mips code.
Did you also have patches planned for those or are these three
the only ones you have in progress?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] fix clang negative signed bit shift warning

[Peter Maydell]

On 16 November 2015 at 14:23, Peter Maydell <peter.maydell@linaro.org> wrote:
> My clang-3.7 build (it's part of fbinfer) also complains about:
>
> /Users/pm215/src/qemu/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 );
>                                                     ^~~~~~~~~~
> /Users/pm215/src/qemu/hw/audio/fmopl.c:75:28: note: expanded from
> macro 'OPL_MINOUT'
> #define OPL_MINOUT (-0x8000<<OPL_OUTSB)
>                     ~~~~~~~^

I sent a patch for that one. The MIPS stuff I'll leave for somebody
else at least for now...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] fix clang negative signed bit shift warning

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Mon, Nov 16, 2015 at 03:20:12PM +0000, Peter Maydell wrote:
> On 16 November 2015 at 14:23, Peter Maydell <peter.maydell@linaro.org> wrote:
> > My clang-3.7 build (it's part of fbinfer) also complains about:
> >
> > /Users/pm215/src/qemu/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 );
> >                                                     ^~~~~~~~~~
> > /Users/pm215/src/qemu/hw/audio/fmopl.c:75:28: note: expanded from
> > macro 'OPL_MINOUT'
> > #define OPL_MINOUT (-0x8000<<OPL_OUTSB)
> >                     ~~~~~~~^
> 
> I sent a patch for that one. The MIPS stuff I'll leave for somebody
> else at least for now...

Thanks.  I don't plan to investigate the audio one soon.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/3] fix clang negative signed bit shift warning

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

On Tue, Nov 10, 2015 at 03:57:32PM +0000, Stefan Hajnoczi wrote:
> LLVM's clang 3.7.0 compile warns about bit shifting negative numbers because
> the result is undefined.  This series includes 3 small fixes to appease clang.
> 
> Stefan Hajnoczi (3):
>   monitor: avoid clang shifting negative signed warning
>   tpm: avoid clang shifting negative signed warning
>   disas/arm: avoid clang shifting negative signed warning
> 
>  disas/arm.c           | 2 +-
>  hw/tpm/tpm_tis.c      | 2 +-
>  target-i386/monitor.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> -- 
> 2.5.0
> 

Thanks, applied Patches 2 & 3 to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]