[Qemu-devel] [PATCH 00/12] Avoid shifting left into sign bit

[Qemu-devel] [PATCH 00/12] Avoid shifting left into sign bit

[Peter Maydell]

This is a set of patches which silence clang -fsanitize=undefined
warnings about shifting left into the sign bit of a signed value.
Typically this is the result of "1 << 31" and similar constructs;
the fix is to add a "U" suffix to the 1 so that we do unsigned
arithmetic rather than signed arithmetic.

Since these patches are very minor changes to a fairly
wide ranging set of files, it seems easiest to send these
through the -trivial queue. Happy to split the series up
if people disagree.

Mostly I think these warnings are not particularly exciting
(even the adversarial optimizations preferred by modern
compilers will probably not break "1 << 31") but there are
a lot of them, the fix is pretty trivial, and getting rid of
them allows us to see the interesting sanitizer warnings more
clearly.

My method here has been just to look at the warnings produced
during a 'make check' run; no doubt actually running a guest
for various platforms would identify more of these.

thanks
-- PMM

Peter Maydell (12):
  target-i386: Avoid shifting left into sign bit
  hw/intc/apic.c: Use uint32_t for mask word in foreach_apic
  hw/pci/pci_host.c: Avoid shifting left into sign bit
  hw/i386/acpi_build.c: Avoid shifting left into sign bit
  target-mips: Avoid shifting left into sign bit
  hw/usb/hcd-ohci.c: Avoid shifting left into sign bit
  hw/intc/openpic: Avoid shifting left into sign bit
  hw/ppc: Avoid shifting left into sign bit
  tests/libqos/pci-pc: Avoid shifting left into sign bit
  hw/intc/slavio_intctl: Avoid shifting left into sign bit
  hw/intc/xilinx_intc: Avoid shifting left into sign bit
  hw/pci-host/apb.c: Avoid shifting left into sign bit

 hw/i386/acpi-build.c         |  2 +-
 hw/intc/apic.c               |  5 +++--
 hw/intc/openpic.c            |  4 ++--
 hw/intc/slavio_intctl.c      |  2 +-
 hw/intc/xilinx_intc.c        |  3 ++-
 hw/pci-host/apb.c            |  2 +-
 hw/pci/pci_host.c            |  3 ++-
 hw/ppc/ppc.c                 |  2 +-
 hw/ppc/ppc440_bamboo.c       |  4 ++--
 hw/ppc/ppc4xx_devs.c         |  2 +-
 hw/ppc/ppc_booke.c           |  4 ++--
 hw/ppc/virtex_ml507.c        |  4 ++--
 hw/usb/hcd-ohci.c            |  6 +++---
 target-i386/cpu.h            |  8 ++++----
 target-mips/cpu.h            |  2 +-
 target-mips/helper.c         |  8 ++++----
 target-mips/op_helper.c      |  2 +-
 target-mips/translate_init.c | 22 +++++++++++-----------
 tests/libqos/pci-pc.c        | 12 ++++++------
 19 files changed, 50 insertions(+), 47 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH 01/12] target-i386: Avoid shifting left into sign bit

[Peter Maydell]

Add 'U' suffixes where necessary to avoid (1 << 31) which
shifts left into the sign bit, which is undefined behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-i386/cpu.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0014acc..064f987 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -202,7 +202,7 @@
 #define CR0_NE_MASK  (1 << 5)
 #define CR0_WP_MASK  (1 << 16)
 #define CR0_AM_MASK  (1 << 18)
-#define CR0_PG_MASK  (1 << 31)
+#define CR0_PG_MASK  (1U << 31)
 
 #define CR4_VME_MASK  (1 << 0)
 #define CR4_PVI_MASK  (1 << 1)
@@ -436,7 +436,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_HT (1 << 28)
 #define CPUID_TM (1 << 29)
 #define CPUID_IA64 (1 << 30)
-#define CPUID_PBE (1 << 31)
+#define CPUID_PBE (1U << 31)
 
 #define CPUID_EXT_SSE3     (1 << 0)
 #define CPUID_EXT_PCLMULQDQ (1 << 1)
@@ -467,7 +467,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_EXT_AVX      (1 << 28)
 #define CPUID_EXT_F16C     (1 << 29)
 #define CPUID_EXT_RDRAND   (1 << 30)
-#define CPUID_EXT_HYPERVISOR  (1 << 31)
+#define CPUID_EXT_HYPERVISOR  (1U << 31)
 
 #define CPUID_EXT2_FPU     (1 << 0)
 #define CPUID_EXT2_VME     (1 << 1)
@@ -496,7 +496,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_EXT2_RDTSCP  (1 << 27)
 #define CPUID_EXT2_LM      (1 << 29)
 #define CPUID_EXT2_3DNOWEXT (1 << 30)
-#define CPUID_EXT2_3DNOW   (1 << 31)
+#define CPUID_EXT2_3DNOW   (1U << 31)
 
 /* CPUID[8000_0001].EDX bits that are aliase of CPUID[1].EDX bits on AMD CPUs */
 #define CPUID_EXT2_AMD_ALIASES (CPUID_EXT2_FPU | CPUID_EXT2_VME | \
-- 
1.9.0

Re: [Qemu-devel] [PATCH 01/12] target-i386: Avoid shifting left into sign bit

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 07:10:37PM +0000, Peter Maydell wrote:
> Add 'U' suffixes where necessary to avoid (1 << 31) which
> shifts left into the sign bit, which is undefined behaviour.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

While not required for correctness,
I think it would be cleaner to change them all to 1U, for consistency.


> ---
>  target-i386/cpu.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 0014acc..064f987 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -202,7 +202,7 @@
>  #define CR0_NE_MASK  (1 << 5)
>  #define CR0_WP_MASK  (1 << 16)
>  #define CR0_AM_MASK  (1 << 18)
> -#define CR0_PG_MASK  (1 << 31)
> +#define CR0_PG_MASK  (1U << 31)
>  
>  #define CR4_VME_MASK  (1 << 0)
>  #define CR4_PVI_MASK  (1 << 1)
> @@ -436,7 +436,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_HT (1 << 28)
>  #define CPUID_TM (1 << 29)
>  #define CPUID_IA64 (1 << 30)
> -#define CPUID_PBE (1 << 31)
> +#define CPUID_PBE (1U << 31)
>  
>  #define CPUID_EXT_SSE3     (1 << 0)
>  #define CPUID_EXT_PCLMULQDQ (1 << 1)
> @@ -467,7 +467,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_EXT_AVX      (1 << 28)
>  #define CPUID_EXT_F16C     (1 << 29)
>  #define CPUID_EXT_RDRAND   (1 << 30)
> -#define CPUID_EXT_HYPERVISOR  (1 << 31)
> +#define CPUID_EXT_HYPERVISOR  (1U << 31)
>  
>  #define CPUID_EXT2_FPU     (1 << 0)
>  #define CPUID_EXT2_VME     (1 << 1)
> @@ -496,7 +496,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_EXT2_RDTSCP  (1 << 27)
>  #define CPUID_EXT2_LM      (1 << 29)
>  #define CPUID_EXT2_3DNOWEXT (1 << 30)
> -#define CPUID_EXT2_3DNOW   (1 << 31)
> +#define CPUID_EXT2_3DNOW   (1U << 31)
>  
>  /* CPUID[8000_0001].EDX bits that are aliase of CPUID[1].EDX bits on AMD CPUs */
>  #define CPUID_EXT2_AMD_ALIASES (CPUID_EXT2_FPU | CPUID_EXT2_VME | \
> -- 
> 1.9.0
> 

[Qemu-devel] [PATCH 02/12] hw/intc/apic.c: Use uint32_t for mask word in foreach_apic

[Peter Maydell]

Use unsigned arithmetic for operations on the mask word
in the foreach_apic() macro, to avoid relying on undefined
behaviour when shifting into the sign bit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/apic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 361ae90..e137882 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -201,12 +201,13 @@ static void apic_external_nmi(APICCommonState *s)
 
 #define foreach_apic(apic, deliver_bitmask, code) \
 {\
-    int __i, __j, __mask;\
+    int __i, __j;\
+    uint32_t __mask;\
     for(__i = 0; __i < MAX_APIC_WORDS; __i++) {\
         __mask = deliver_bitmask[__i];\
         if (__mask) {\
             for(__j = 0; __j < 32; __j++) {\
-                if (__mask & (1 << __j)) {\
+                if (__mask & (1U << __j)) {\
                     apic = local_apics[__i * 32 + __j];\
                     if (apic) {\
                         code;\
-- 
1.9.0

Re: [Qemu-devel] [PATCH 02/12] hw/intc/apic.c: Use uint32_t for mask word in foreach_apic

[Stefan Weil]

Am 10.03.2014 20:10, schrieb Peter Maydell:
> Use unsigned arithmetic for operations on the mask word
> in the foreach_apic() macro, to avoid relying on undefined
> behaviour when shifting into the sign bit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/apic.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 361ae90..e137882 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -201,12 +201,13 @@ static void apic_external_nmi(APICCommonState *s)
>  
>  #define foreach_apic(apic, deliver_bitmask, code) \
>  {\
> -    int __i, __j, __mask;\
> +    int __i, __j;\
> +    uint32_t __mask;\
>      for(__i = 0; __i < MAX_APIC_WORDS; __i++) {\
>          __mask = deliver_bitmask[__i];\
>          if (__mask) {\
>              for(__j = 0; __j < 32; __j++) {\
> -                if (__mask & (1 << __j)) {\
> +                if (__mask & (1U << __j)) {\
>                      apic = local_apics[__i * 32 + __j];\
>                      if (apic) {\
>                          code;\
> 


The declaration of __mask could be moved inside the for block and be
combined with the assignment, but that's not strictly necessary.

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Re: [Qemu-devel] [PATCH 02/12] hw/intc/apic.c: Use uint32_t for mask word in foreach_apic

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 07:10:38PM +0000, Peter Maydell wrote:
> Use unsigned arithmetic for operations on the mask word
> in the foreach_apic() macro, to avoid relying on undefined
> behaviour when shifting into the sign bit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/intc/apic.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 361ae90..e137882 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -201,12 +201,13 @@ static void apic_external_nmi(APICCommonState *s)
>  
>  #define foreach_apic(apic, deliver_bitmask, code) \
>  {\
> -    int __i, __j, __mask;\
> +    int __i, __j;\
> +    uint32_t __mask;\
>      for(__i = 0; __i < MAX_APIC_WORDS; __i++) {\
>          __mask = deliver_bitmask[__i];\
>          if (__mask) {\
>              for(__j = 0; __j < 32; __j++) {\
> -                if (__mask & (1 << __j)) {\
> +                if (__mask & (1U << __j)) {\
>                      apic = local_apics[__i * 32 + __j];\
>                      if (apic) {\
>                          code;\
> -- 
> 1.9.0
> 

[Qemu-devel] [PATCH 03/12] hw/pci/pci_host.c: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix to avoid undefined behaviour.

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

diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 77c7d1f..2c17916 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -142,8 +142,9 @@ static uint64_t pci_host_data_read(void *opaque,
 {
     PCIHostState *s = opaque;
     uint32_t val;
-    if (!(s->config_reg & (1 << 31)))
+    if (!(s->config_reg & (1u << 31))) {
         return 0xffffffff;
+    }
     val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
     PCI_DPRINTF("read addr " TARGET_FMT_plx " len %d val %x\n",
                 addr, len, val);
-- 
1.9.0

Re: [Qemu-devel] [PATCH 03/12] hw/pci/pci_host.c: Avoid shifting left into sign bit

[Stefan Weil]

Am 10.03.2014 20:10, schrieb Peter Maydell:
> Add U suffix to avoid undefined behaviour.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/pci/pci_host.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 77c7d1f..2c17916 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -142,8 +142,9 @@ static uint64_t pci_host_data_read(void *opaque,
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> +    if (!(s->config_reg & (1u << 31))) {
>          return 0xffffffff;

I suggest fixing that 0xffffffff, too. Do we expect here a 32 bit value
(-1) which is expanded to a 64 bit value? Then 0xffffffffffffffffULL
would be correct. Otherwise 0xffffffffU is clearer.

Michael, are 8 byte reads possible here? If yes, the current code is wrong.

> +    }
>      val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
>      PCI_DPRINTF("read addr " TARGET_FMT_plx " len %d val %x\n",
>                  addr, len, val);
> 

What about using the BIT macro from qemu/bitops.h? I think that BIT(31)
looks better than (1u << 31).

Stefan

Re: [Qemu-devel] [PATCH 03/12] hw/pci/pci_host.c: Avoid shifting left into sign bit

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 09:03:08PM +0100, Stefan Weil wrote:
> Am 10.03.2014 20:10, schrieb Peter Maydell:
> > Add U suffix to avoid undefined behaviour.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/pci/pci_host.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index 77c7d1f..2c17916 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -142,8 +142,9 @@ static uint64_t pci_host_data_read(void *opaque,
> >  {
> >      PCIHostState *s = opaque;
> >      uint32_t val;
> > -    if (!(s->config_reg & (1 << 31)))
> > +    if (!(s->config_reg & (1u << 31))) {
> >          return 0xffffffff;
> 
> I suggest fixing that 0xffffffff, too. Do we expect here a 32 bit value
> (-1) which is expanded to a 64 bit value? Then 0xffffffffffffffffULL
> would be correct. Otherwise 0xffffffffU is clearer.

Hmm why is this clearer?  The spec says:

	The type of an integer constant is the first of the corresponding list
	in which its value can be represented.

Basically 0x<anything> is always a positive value and when cast to
uint64_t is not sign extended.


> Michael, are 8 byte reads possible here? If yes, the current code is wrong.

AFAIK they aren't possible on any platform we support.
So since we discard high bits 0 seems cleaner.

> > +    }
> >      val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
> >      PCI_DPRINTF("read addr " TARGET_FMT_plx " len %d val %x\n",
> >                  addr, len, val);
> > 
> 
> What about using the BIT macro from qemu/bitops.h? I think that BIT(31)
> looks better than (1u << 31).
> 
> Stefan

I slightly prefer 1u << 31 personally, standard C as opposed to qemu macros.


-- 
MST

Re: [Qemu-devel] [PATCH 03/12] hw/pci/pci_host.c: Avoid shifting left into sign bit

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 07:10:39PM +0000, Peter Maydell wrote:
> Add U suffix to avoid undefined behaviour.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci/pci_host.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 77c7d1f..2c17916 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -142,8 +142,9 @@ static uint64_t pci_host_data_read(void *opaque,
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> +    if (!(s->config_reg & (1u << 31))) {
>          return 0xffffffff;
> +    }
>      val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
>      PCI_DPRINTF("read addr " TARGET_FMT_plx " len %d val %x\n",
>                  addr, len, val);
> -- 
> 1.9.0
> 

[Qemu-devel] [PATCH 04/12] hw/i386/acpi_build.c: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix to avoid undefined behaviour.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b1a7ebb..46d4e60 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 
             build_append_byte(notify, 0x7B); /* AndOp */
             build_append_byte(notify, 0x68); /* Arg0Op */
-            build_append_int(notify, 0x1 << i);
+            build_append_int(notify, 0x1U << i);
             build_append_byte(notify, 0x00); /* NullName */
             build_append_byte(notify, 0x86); /* NotifyOp */
             build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
-- 
1.9.0

Re: [Qemu-devel] [PATCH 04/12] hw/i386/acpi_build.c: Avoid shifting left into sign bit

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 07:10:40PM +0000, Peter Maydell wrote:
> Add U suffix to avoid undefined behaviour.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/i386/acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b1a7ebb..46d4e60 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>  
>              build_append_byte(notify, 0x7B); /* AndOp */
>              build_append_byte(notify, 0x68); /* Arg0Op */
> -            build_append_int(notify, 0x1 << i);
> +            build_append_int(notify, 0x1U << i);
>              build_append_byte(notify, 0x00); /* NullName */
>              build_append_byte(notify, 0x86); /* NotifyOp */
>              build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> -- 
> 1.9.0
> 

[Qemu-devel] [PATCH 05/12] target-mips: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix to various places where we shift a 1 left by 31,
to avoid undefined behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-mips/cpu.h            |  2 +-
 target-mips/helper.c         |  8 ++++----
 target-mips/op_helper.c      |  2 +-
 target-mips/translate_init.c | 22 +++++++++++-----------
 4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 60c8061..25e97be 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -803,7 +803,7 @@ static inline void compute_hflags(CPUMIPSState *env)
            and disable the MIPS IV extensions to the MIPS III ISA.
            Some other MIPS IV CPUs ignore the bit, so the check here
            would be too restrictive for them.  */
-        if (env->CP0_Status & (1 << CP0St_CU3)) {
+        if (env->CP0_Status & (1U << CP0St_CU3)) {
             env->hflags |= MIPS_HFLAG_COP1X;
         }
     }
diff --git a/target-mips/helper.c b/target-mips/helper.c
index 33e0e88..68eb9f6 100644
--- a/target-mips/helper.c
+++ b/target-mips/helper.c
@@ -452,7 +452,7 @@ void mips_cpu_do_interrupt(CPUState *cs)
         env->hflags &= ~(MIPS_HFLAG_KSU);
         /* EJTAG probe trap enable is not implemented... */
         if (!(env->CP0_Status & (1 << CP0St_EXL)))
-            env->CP0_Cause &= ~(1 << CP0Ca_BD);
+            env->CP0_Cause &= ~(1U << CP0Ca_BD);
         env->active_tc.PC = (int32_t)0xBFC00480;
         set_hflags_for_handler(env);
         break;
@@ -472,7 +472,7 @@ void mips_cpu_do_interrupt(CPUState *cs)
         env->hflags |= MIPS_HFLAG_64 | MIPS_HFLAG_CP0;
         env->hflags &= ~(MIPS_HFLAG_KSU);
         if (!(env->CP0_Status & (1 << CP0St_EXL)))
-            env->CP0_Cause &= ~(1 << CP0Ca_BD);
+            env->CP0_Cause &= ~(1U << CP0Ca_BD);
         env->active_tc.PC = (int32_t)0xBFC00000;
         set_hflags_for_handler(env);
         break;
@@ -610,9 +610,9 @@ void mips_cpu_do_interrupt(CPUState *cs)
         if (!(env->CP0_Status & (1 << CP0St_EXL))) {
             env->CP0_EPC = exception_resume_pc(env);
             if (env->hflags & MIPS_HFLAG_BMASK) {
-                env->CP0_Cause |= (1 << CP0Ca_BD);
+                env->CP0_Cause |= (1U << CP0Ca_BD);
             } else {
-                env->CP0_Cause &= ~(1 << CP0Ca_BD);
+                env->CP0_Cause &= ~(1U << CP0Ca_BD);
             }
             env->CP0_Status |= (1 << CP0St_EXL);
             env->hflags |= MIPS_HFLAG_64 | MIPS_HFLAG_CP0;
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 2ef6633..256c3c9 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -646,7 +646,7 @@ static void sync_c0_tcstatus(CPUMIPSState *cpu, int tc,
 {
     uint32_t status;
     uint32_t tcu, tmx, tasid, tksu;
-    uint32_t mask = ((1 << CP0St_CU3)
+    uint32_t mask = ((1U << CP0St_CU3)
                        | (1 << CP0St_CU2)
                        | (1 << CP0St_CU1)
                        | (1 << CP0St_CU0)
diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c
index 29d39e2..c765825 100644
--- a/target-mips/translate_init.c
+++ b/target-mips/translate_init.c
@@ -22,20 +22,20 @@
 
 /* Have config1, uncached coherency */
 #define MIPS_CONFIG0                                              \
-  ((1 << CP0C0_M) | (0x2 << CP0C0_K0))
+  ((1U << CP0C0_M) | (0x2 << CP0C0_K0))
 
 /* Have config2, no coprocessor2 attached, no MDMX support attached,
    no performance counters, watch registers present,
    no code compression, EJTAG present, no FPU */
 #define MIPS_CONFIG1                                              \
-((1 << CP0C1_M) |                                                 \
+((1U << CP0C1_M) |                                                \
  (0 << CP0C1_C2) | (0 << CP0C1_MD) | (0 << CP0C1_PC) |            \
  (1 << CP0C1_WR) | (0 << CP0C1_CA) | (1 << CP0C1_EP) |            \
  (0 << CP0C1_FP))
 
 /* Have config3, no tertiary/secondary caches implemented */
 #define MIPS_CONFIG2                                              \
-((1 << CP0C2_M))
+((1U << CP0C2_M))
 
 /* No config4, no DSP ASE, no large physaddr (PABITS),
    no external interrupt controller, no vectored interrupts,
@@ -301,16 +301,16 @@ static const mips_def_t mips_defs[] =
                     (1 << FCR0_D) | (1 << FCR0_S) | (0x95 << FCR0_PRID),
         .CP0_SRSCtl = (0xf << CP0SRSCtl_HSS),
         .CP0_SRSConf0_rw_bitmask = 0x3fffffff,
-        .CP0_SRSConf0 = (1 << CP0SRSC0_M) | (0x3fe << CP0SRSC0_SRS3) |
+        .CP0_SRSConf0 = (1U << CP0SRSC0_M) | (0x3fe << CP0SRSC0_SRS3) |
                     (0x3fe << CP0SRSC0_SRS2) | (0x3fe << CP0SRSC0_SRS1),
         .CP0_SRSConf1_rw_bitmask = 0x3fffffff,
-        .CP0_SRSConf1 = (1 << CP0SRSC1_M) | (0x3fe << CP0SRSC1_SRS6) |
+        .CP0_SRSConf1 = (1U << CP0SRSC1_M) | (0x3fe << CP0SRSC1_SRS6) |
                     (0x3fe << CP0SRSC1_SRS5) | (0x3fe << CP0SRSC1_SRS4),
         .CP0_SRSConf2_rw_bitmask = 0x3fffffff,
-        .CP0_SRSConf2 = (1 << CP0SRSC2_M) | (0x3fe << CP0SRSC2_SRS9) |
+        .CP0_SRSConf2 = (1U << CP0SRSC2_M) | (0x3fe << CP0SRSC2_SRS9) |
                     (0x3fe << CP0SRSC2_SRS8) | (0x3fe << CP0SRSC2_SRS7),
         .CP0_SRSConf3_rw_bitmask = 0x3fffffff,
-        .CP0_SRSConf3 = (1 << CP0SRSC3_M) | (0x3fe << CP0SRSC3_SRS12) |
+        .CP0_SRSConf3 = (1U << CP0SRSC3_M) | (0x3fe << CP0SRSC3_SRS12) |
                     (0x3fe << CP0SRSC3_SRS11) | (0x3fe << CP0SRSC3_SRS10),
         .CP0_SRSConf4_rw_bitmask = 0x3fffffff,
         .CP0_SRSConf4 = (0x3fe << CP0SRSC4_SRS15) |
@@ -355,8 +355,8 @@ static const mips_def_t mips_defs[] =
                        (0 << CP0C1_DS) | (3 << CP0C1_DL) | (1 << CP0C1_DA) |
                        (1 << CP0C1_CA),
         .CP0_Config2 = MIPS_CONFIG2,
-        .CP0_Config3 = MIPS_CONFIG3 | (1 << CP0C3_M),
-        .CP0_Config4 = MIPS_CONFIG4 | (1 << CP0C4_M),
+        .CP0_Config3 = MIPS_CONFIG3 | (1U << CP0C3_M),
+        .CP0_Config4 = MIPS_CONFIG4 | (1U << CP0C4_M),
         .CP0_Config4_rw_bitmask = 0,
         .CP0_Config5 = MIPS_CONFIG5 | (1 << CP0C5_UFR),
         .CP0_Config5_rw_bitmask = (0 << CP0C5_M) | (1 << CP0C5_K) |
@@ -668,7 +668,7 @@ static void mvp_init (CPUMIPSState *env, const mips_def_t *def)
        programmable cache partitioning implemented, number of allocatable
        and sharable TLB entries, MVP has allocatable TCs, 2 VPEs
        implemented, 5 TCs implemented. */
-    env->mvp->CP0_MVPConf0 = (1 << CP0MVPC0_M) | (1 << CP0MVPC0_TLBS) |
+    env->mvp->CP0_MVPConf0 = (1U << CP0MVPC0_M) | (1 << CP0MVPC0_TLBS) |
                              (0 << CP0MVPC0_GS) | (1 << CP0MVPC0_PCP) |
 // TODO: actually do 2 VPEs.
 //                             (1 << CP0MVPC0_TCA) | (0x1 << CP0MVPC0_PVPE) |
@@ -682,7 +682,7 @@ static void mvp_init (CPUMIPSState *env, const mips_def_t *def)
 
     /* Allocatable CP1 have media extensions, allocatable CP1 have FP support,
        no UDI implemented, no CP2 implemented, 1 CP1 implemented. */
-    env->mvp->CP0_MVPConf1 = (1 << CP0MVPC1_CIM) | (1 << CP0MVPC1_CIF) |
+    env->mvp->CP0_MVPConf1 = (1U << CP0MVPC1_CIM) | (1 << CP0MVPC1_CIF) |
                              (0x0 << CP0MVPC1_PCX) | (0x0 << CP0MVPC1_PCP2) |
                              (0x1 << CP0MVPC1_PCP1);
 }
-- 
1.9.0

[Qemu-devel] [PATCH 06/12] hw/usb/hcd-ohci.c: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix to avoid undefined behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/usb/hcd-ohci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 3d35058b..9dcfb99 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -242,7 +242,7 @@ struct ohci_iso_td {
 #define OHCI_INTR_FNO         (1<<5) /* Frame number overflow */
 #define OHCI_INTR_RHSC        (1<<6) /* Root hub status change */
 #define OHCI_INTR_OC          (1<<30) /* Ownership change */
-#define OHCI_INTR_MIE         (1<<31) /* Master Interrupt Enable */
+#define OHCI_INTR_MIE         (1U<<31) /* Master Interrupt Enable */
 
 #define OHCI_HCCA_SIZE        0x100
 #define OHCI_HCCA_MASK        0xffffff00
@@ -253,7 +253,7 @@ struct ohci_iso_td {
 #define OHCI_FMI_FSMPS        0xffff0000
 #define OHCI_FMI_FIT          0x80000000
 
-#define OHCI_FR_RT            (1<<31)
+#define OHCI_FR_RT            (1U<<31)
 
 #define OHCI_LS_THRESH        0x628
 
@@ -270,7 +270,7 @@ struct ohci_iso_td {
 #define OHCI_RHS_DRWE         (1<<15)
 #define OHCI_RHS_LPSC         (1<<16)
 #define OHCI_RHS_OCIC         (1<<17)
-#define OHCI_RHS_CRWE         (1<<31)
+#define OHCI_RHS_CRWE         (1U<<31)
 
 #define OHCI_PORT_CCS         (1<<0)
 #define OHCI_PORT_PES         (1<<1)
-- 
1.9.0

[Qemu-devel] [PATCH 07/12] hw/intc/openpic: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix to avoid undefined behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/openpic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 7df72f4..cfd7f06 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -123,7 +123,7 @@ static FslMpicInfo fsl_mpic_42 = {
 #define TCCR_TOG          0x80000000 /* toggles when decrement to zero */
 
 #define IDR_EP_SHIFT      31
-#define IDR_EP_MASK       (1 << IDR_EP_SHIFT)
+#define IDR_EP_MASK       (1U << IDR_EP_SHIFT)
 #define IDR_CI0_SHIFT     30
 #define IDR_CI1_SHIFT     29
 #define IDR_P1_SHIFT      1
@@ -220,7 +220,7 @@ typedef struct IRQSource {
 } IRQSource;
 
 #define IVPR_MASK_SHIFT       31
-#define IVPR_MASK_MASK        (1 << IVPR_MASK_SHIFT)
+#define IVPR_MASK_MASK        (1U << IVPR_MASK_SHIFT)
 #define IVPR_ACTIVITY_SHIFT   30
 #define IVPR_ACTIVITY_MASK    (1 << IVPR_ACTIVITY_SHIFT)
 #define IVPR_MODE_SHIFT       29
-- 
1.9.0

[Qemu-devel] [PATCH 08/12] hw/ppc: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix to various places where we were doing "1 << 31",
which is undefined behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/ppc.c           | 2 +-
 hw/ppc/ppc440_bamboo.c | 4 ++--
 hw/ppc/ppc4xx_devs.c   | 2 +-
 hw/ppc/ppc_booke.c     | 4 ++--
 hw/ppc/virtex_ml507.c  | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 0e82719..9c2a132 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1002,7 +1002,7 @@ static void cpu_4xx_wdt_cb (void *opaque)
     case 0x1:
         timer_mod(ppc40x_timer->wdt_timer, next);
         ppc40x_timer->wdt_next = next;
-        env->spr[SPR_40x_TSR] |= 1 << 31;
+        env->spr[SPR_40x_TSR] |= 1U << 31;
         break;
     case 0x2:
         timer_mod(ppc40x_timer->wdt_timer, next);
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index ec15bab..2ddc2ed 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -128,7 +128,7 @@ static void mmubooke_create_initial_mapping(CPUPPCState *env,
 
     tlb->attr = 0;
     tlb->prot = PAGE_VALID | ((PAGE_READ | PAGE_WRITE | PAGE_EXEC) << 4);
-    tlb->size = 1 << 31; /* up to 0x80000000  */
+    tlb->size = 1U << 31; /* up to 0x80000000  */
     tlb->EPN = va & TARGET_PAGE_MASK;
     tlb->RPN = pa & TARGET_PAGE_MASK;
     tlb->PID = 0;
@@ -136,7 +136,7 @@ static void mmubooke_create_initial_mapping(CPUPPCState *env,
     tlb = &env->tlb.tlbe[1];
     tlb->attr = 0;
     tlb->prot = PAGE_VALID | ((PAGE_READ | PAGE_WRITE | PAGE_EXEC) << 4);
-    tlb->size = 1 << 31; /* up to 0xffffffff  */
+    tlb->size = 1U << 31; /* up to 0xffffffff  */
     tlb->EPN = 0x80000000 & TARGET_PAGE_MASK;
     tlb->RPN = 0x80000000 & TARGET_PAGE_MASK;
     tlb->PID = 0;
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 9160ee7..8a43111 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -161,7 +161,7 @@ static void ppcuic_set_irq (void *opaque, int irq_num, int level)
     uint32_t mask, sr;
 
     uic = opaque;
-    mask = 1 << (31-irq_num);
+    mask = 1U << (31-irq_num);
     LOG_UIC("%s: irq %d level %d uicsr %08" PRIx32
                 " mask %08" PRIx32 " => %08" PRIx32 " %08" PRIx32 "\n",
                 __func__, irq_num, level,
diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
index d839960..773c4ea 100644
--- a/hw/ppc/ppc_booke.c
+++ b/hw/ppc/ppc_booke.c
@@ -34,7 +34,7 @@
 /* Timer Control Register */
 
 #define TCR_WP_SHIFT  30        /* Watchdog Timer Period */
-#define TCR_WP_MASK   (0x3 << TCR_WP_SHIFT)
+#define TCR_WP_MASK   (0x3U << TCR_WP_SHIFT)
 #define TCR_WRC_SHIFT 28        /* Watchdog Timer Reset Control */
 #define TCR_WRC_MASK  (0x3 << TCR_WRC_SHIFT)
 #define TCR_WIE       (1 << 27) /* Watchdog Timer Interrupt Enable */
@@ -58,7 +58,7 @@
 #define TSR_WRS_SHIFT 28        /* Watchdog Timer Reset Status */
 #define TSR_WRS_MASK  (0x3 << TSR_WRS_SHIFT)
 #define TSR_WIS       (1 << 30) /* Watchdog Timer Interrupt Status */
-#define TSR_ENW       (1 << 31) /* Enable Next Watchdog Timer */
+#define TSR_ENW       (1U << 31) /* Enable Next Watchdog Timer */
 
 typedef struct booke_timer_t booke_timer_t;
 struct booke_timer_t {
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index ce8ea91..3e3569d 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -71,7 +71,7 @@ static void mmubooke_create_initial_mapping(CPUPPCState *env,
 
     tlb->attr = 0;
     tlb->prot = PAGE_VALID | ((PAGE_READ | PAGE_WRITE | PAGE_EXEC) << 4);
-    tlb->size = 1 << 31; /* up to 0x80000000  */
+    tlb->size = 1U << 31; /* up to 0x80000000  */
     tlb->EPN = va & TARGET_PAGE_MASK;
     tlb->RPN = pa & TARGET_PAGE_MASK;
     tlb->PID = 0;
@@ -79,7 +79,7 @@ static void mmubooke_create_initial_mapping(CPUPPCState *env,
     tlb = &env->tlb.tlbe[1];
     tlb->attr = 0;
     tlb->prot = PAGE_VALID | ((PAGE_READ | PAGE_WRITE | PAGE_EXEC) << 4);
-    tlb->size = 1 << 31; /* up to 0xffffffff  */
+    tlb->size = 1U << 31; /* up to 0xffffffff  */
     tlb->EPN = 0x80000000 & TARGET_PAGE_MASK;
     tlb->RPN = 0x80000000 & TARGET_PAGE_MASK;
     tlb->PID = 0;
-- 
1.9.0

[Qemu-devel] [PATCH 09/12] tests/libqos/pci-pc: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix when doing "1 << 31" to avoid undefined behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/libqos/pci-pc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index 3bde8ab..bf741a4 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -110,37 +110,37 @@ static void qpci_pc_io_writel(QPCIBus *bus, void *addr, uint32_t value)
 
 static uint8_t qpci_pc_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1 << 31) | (devfn << 8) | offset);
+    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
     return inb(0xcfc);
 }
 
 static uint16_t qpci_pc_config_readw(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1 << 31) | (devfn << 8) | offset);
+    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
     return inw(0xcfc);
 }
 
 static uint32_t qpci_pc_config_readl(QPCIBus *bus, int devfn, uint8_t offset)
 {
-    outl(0xcf8, (1 << 31) | (devfn << 8) | offset);
+    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
     return inl(0xcfc);
 }
 
 static void qpci_pc_config_writeb(QPCIBus *bus, int devfn, uint8_t offset, uint8_t value)
 {
-    outl(0xcf8, (1 << 31) | (devfn << 8) | offset);
+    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
     outb(0xcfc, value);
 }
 
 static void qpci_pc_config_writew(QPCIBus *bus, int devfn, uint8_t offset, uint16_t value)
 {
-    outl(0xcf8, (1 << 31) | (devfn << 8) | offset);
+    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
     outw(0xcfc, value);
 }
 
 static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint32_t value)
 {
-    outl(0xcf8, (1 << 31) | (devfn << 8) | offset);
+    outl(0xcf8, (1U << 31) | (devfn << 8) | offset);
     outl(0xcfc, value);
 }
 
-- 
1.9.0

[Qemu-devel] [PATCH 10/12] hw/intc/slavio_intctl: Avoid shifting left into sign bit

[Peter Maydell]

Add 'U' suffix to avoid undefined behaviour.

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

diff --git a/hw/intc/slavio_intctl.c b/hw/intc/slavio_intctl.c
index 41a1672..b10fb66 100644
--- a/hw/intc/slavio_intctl.c
+++ b/hw/intc/slavio_intctl.c
@@ -272,7 +272,7 @@ static void slavio_check_interrupts(SLAVIO_INTCTLState *s, int set_irqs)
             CPU_IRQ_TIMER_IN;
         if (i == s->target_cpu) {
             for (j = 0; j < 32; j++) {
-                if ((s->intregm_pending & (1 << j)) && intbit_to_level[j]) {
+                if ((s->intregm_pending & (1U << j)) && intbit_to_level[j]) {
                     s->slaves[i].intreg_pending |= 1 << intbit_to_level[j];
                 }
             }
-- 
1.9.0

[Qemu-devel] [PATCH 11/12] hw/intc/xilinx_intc: Avoid shifting left into sign bit

[Peter Maydell]

Avoid undefined behaviour shifting left into the sign bit.

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

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index 4a10398..1b228ff 100644
--- a/hw/intc/xilinx_intc.c
+++ b/hw/intc/xilinx_intc.c
@@ -71,8 +71,9 @@ static void update_irq(struct xlx_pic *p)
 
     /* Update the vector register.  */
     for (i = 0; i < 32; i++) {
-        if (p->regs[R_IPR] & (1 << i))
+        if (p->regs[R_IPR] & (1U << i)) {
             break;
+        }
     }
     if (i == 32)
         i = ~0;
-- 
1.9.0

[Qemu-devel] [PATCH 12/12] hw/pci-host/apb.c: Avoid shifting left into sign bit

[Peter Maydell]

Add U suffix to avoid undefined behaviour.

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

diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 1b399dd..a6869b8 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -58,7 +58,7 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
 #define PBM_PCI_IMR_MASK    0x7fffffff
 #define PBM_PCI_IMR_ENABLED 0x80000000
 
-#define POR          (1 << 31)
+#define POR          (1U << 31)
 #define SOFT_POR     (1 << 30)
 #define SOFT_XIR     (1 << 29)
 #define BTN_POR      (1 << 28)
-- 
1.9.0

Re: [Qemu-devel] [PATCH 12/12] hw/pci-host/apb.c: Avoid shifting left into sign bit

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 07:10:48PM +0000, Peter Maydell wrote:
> Add U suffix to avoid undefined behaviour.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

While not required for correctness, it would be cleaner
to change all constants around this line to 1U <<, for consistency.

> ---
>  hw/pci-host/apb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 1b399dd..a6869b8 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -58,7 +58,7 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
>  #define PBM_PCI_IMR_MASK    0x7fffffff
>  #define PBM_PCI_IMR_ENABLED 0x80000000
>  
> -#define POR          (1 << 31)
> +#define POR          (1U << 31)
>  #define SOFT_POR     (1 << 30)
>  #define SOFT_XIR     (1 << 29)
>  #define BTN_POR      (1 << 28)
> -- 
> 1.9.0
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH 12/12] hw/pci-host/apb.c: Avoid shifting left into sign bit

[Michael Tokarev]

11.03.2014 01:59, Michael S. Tsirkin wrote:
> On Mon, Mar 10, 2014 at 07:10:48PM +0000, Peter Maydell wrote:
>> Add U suffix to avoid undefined behaviour.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> While not required for correctness, it would be cleaner
> to change all constants around this line to 1U <<, for consistency.

I agree, this is what I thought as well when looking at the result.
I can fix when applying if you like.

Thanks,

/mjt


>> ---
>>  hw/pci-host/apb.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
>> index 1b399dd..a6869b8 100644
>> --- a/hw/pci-host/apb.c
>> +++ b/hw/pci-host/apb.c
>> @@ -58,7 +58,7 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
>>  #define PBM_PCI_IMR_MASK    0x7fffffff
>>  #define PBM_PCI_IMR_ENABLED 0x80000000
>>  
>> -#define POR          (1 << 31)
>> +#define POR          (1U << 31)
>>  #define SOFT_POR     (1 << 30)
>>  #define SOFT_XIR     (1 << 29)
>>  #define BTN_POR      (1 << 28)


>> 1.9.0
>>
> 

Re: [Qemu-devel] [PATCH 00/12] Avoid shifting left into sign bit

[Michael S. Tsirkin]

On Mon, Mar 10, 2014 at 07:10:36PM +0000, Peter Maydell wrote:
> This is a set of patches which silence clang -fsanitize=undefined
> warnings about shifting left into the sign bit of a signed value.
> Typically this is the result of "1 << 31" and similar constructs;
> the fix is to add a "U" suffix to the 1 so that we do unsigned
> arithmetic rather than signed arithmetic.
> 
> Since these patches are very minor changes to a fairly
> wide ranging set of files, it seems easiest to send these
> through the -trivial queue. Happy to split the series up
> if people disagree.

I'm fine with this for pci and pc bits.

> Mostly I think these warnings are not particularly exciting
> (even the adversarial optimizations preferred by modern
> compilers will probably not break "1 << 31") but there are
> a lot of them, the fix is pretty trivial, and getting rid of
> them allows us to see the interesting sanitizer warnings more
> clearly.
> 
> My method here has been just to look at the warnings produced
> during a 'make check' run; no doubt actually running a guest
> for various platforms would identify more of these.
> 
> thanks
> -- PMM
> 
> Peter Maydell (12):
>   target-i386: Avoid shifting left into sign bit
>   hw/intc/apic.c: Use uint32_t for mask word in foreach_apic
>   hw/pci/pci_host.c: Avoid shifting left into sign bit
>   hw/i386/acpi_build.c: Avoid shifting left into sign bit
>   target-mips: Avoid shifting left into sign bit
>   hw/usb/hcd-ohci.c: Avoid shifting left into sign bit
>   hw/intc/openpic: Avoid shifting left into sign bit
>   hw/ppc: Avoid shifting left into sign bit
>   tests/libqos/pci-pc: Avoid shifting left into sign bit
>   hw/intc/slavio_intctl: Avoid shifting left into sign bit
>   hw/intc/xilinx_intc: Avoid shifting left into sign bit
>   hw/pci-host/apb.c: Avoid shifting left into sign bit
> 
>  hw/i386/acpi-build.c         |  2 +-
>  hw/intc/apic.c               |  5 +++--
>  hw/intc/openpic.c            |  4 ++--
>  hw/intc/slavio_intctl.c      |  2 +-
>  hw/intc/xilinx_intc.c        |  3 ++-
>  hw/pci-host/apb.c            |  2 +-
>  hw/pci/pci_host.c            |  3 ++-
>  hw/ppc/ppc.c                 |  2 +-
>  hw/ppc/ppc440_bamboo.c       |  4 ++--
>  hw/ppc/ppc4xx_devs.c         |  2 +-
>  hw/ppc/ppc_booke.c           |  4 ++--
>  hw/ppc/virtex_ml507.c        |  4 ++--
>  hw/usb/hcd-ohci.c            |  6 +++---
>  target-i386/cpu.h            |  8 ++++----
>  target-mips/cpu.h            |  2 +-
>  target-mips/helper.c         |  8 ++++----
>  target-mips/op_helper.c      |  2 +-
>  target-mips/translate_init.c | 22 +++++++++++-----------
>  tests/libqos/pci-pc.c        | 12 ++++++------
>  19 files changed, 50 insertions(+), 47 deletions(-)
> 
> -- 
> 1.9.0
>