| Message ID | 23216f8d6e3e00bdf667e1c7f942500d3c892a18.1348417996.git.blauwirbel@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 23 September 2012 17:33, Blue Swirl <blauwirbel@gmail.com> wrote: > Avoid this warning: > CC arm-softmmu/hw/arm/../arm_gic.o > /src/qemu/hw/arm/../arm_gic.c:432:17: error: implicit truncation from 'unsigned int' to bitfield changes value from 4294967040 to 0 [-Werror,-Wconstant-conversion] > GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > /src/qemu/hw/arm/../arm_gic_internal.h:43:62: note: expanded from: > #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) > ^ ~~~~~ > > 4294967040 is 0xffffff00 and field 'pending' is effectively 8 bits > wide, so the masking has no effect except for avoiding the warning. foo &= ~SOME_FLAGS; is an entirely legitimate and common C idiom, and I think clang is being overexuberant in warning here: we should disable this warning instead of working around it in the code. > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > hw/arm_gic_internal.h | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/hw/arm_gic_internal.h b/hw/arm_gic_internal.h > index db4fad5..219aef3 100644 > --- a/hw/arm_gic_internal.h > +++ b/hw/arm_gic_internal.h > @@ -40,7 +40,8 @@ > #define GIC_CLEAR_ENABLED(irq, cm) s->irq_state[irq].enabled &= ~(cm) > #define GIC_TEST_ENABLED(irq, cm) ((s->irq_state[irq].enabled & (cm)) != 0) > #define GIC_SET_PENDING(irq, cm) s->irq_state[irq].pending |= (cm) > -#define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) > +#define GIC_CLEAR_PENDING(irq, cm) \ > + s->irq_state[irq].pending &= ~(cm) & ALL_CPU_MASK > #define GIC_TEST_PENDING(irq, cm) ((s->irq_state[irq].pending & (cm)) != 0) > #define GIC_SET_ACTIVE(irq, cm) s->irq_state[irq].active |= (cm) > #define GIC_CLEAR_ACTIVE(irq, cm) s->irq_state[irq].active &= ~(cm) This patch makes GIC_CLEAR_PENDING different from GIC_CLEAR_ACTIVE, GIC_CLEAR_LEVEL and GIC_CLEAR_ENABLED, which is odd since the operation should be identical for all those cases. -- PMM
On 24 September 2012 14:22, Peter Maydell <peter.maydell@linaro.org> wrote: > On 23 September 2012 17:33, Blue Swirl <blauwirbel@gmail.com> wrote: >> Avoid this warning: >> CC arm-softmmu/hw/arm/../arm_gic.o >> /src/qemu/hw/arm/../arm_gic.c:432:17: error: implicit truncation from 'unsigned int' to bitfield changes value from 4294967040 to 0 [-Werror,-Wconstant-conversion] >> GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> /src/qemu/hw/arm/../arm_gic_internal.h:43:62: note: expanded from: >> #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) >> ^ ~~~~~ >> >> 4294967040 is 0xffffff00 and field 'pending' is effectively 8 bits >> wide, so the masking has no effect except for avoiding the warning. > > foo &= ~SOME_FLAGS; is an entirely legitimate and common C idiom, > and I think clang is being overexuberant in warning here: we should > disable this warning instead of working around it in the code. Also, what version of clang are you using? I don't see this warning either with MacOS X "Apple clang version 4.0 (tags/Apple/clang-421.0.60) (based on LLVM 3.1svn)" or with "Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final) (based on LLVM 3.0)". -- PMM
On Mon, Sep 24, 2012 at 2:56 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 September 2012 14:22, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 23 September 2012 17:33, Blue Swirl <blauwirbel@gmail.com> wrote: >>> Avoid this warning: >>> CC arm-softmmu/hw/arm/../arm_gic.o >>> /src/qemu/hw/arm/../arm_gic.c:432:17: error: implicit truncation from 'unsigned int' to bitfield changes value from 4294967040 to 0 [-Werror,-Wconstant-conversion] >>> GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> /src/qemu/hw/arm/../arm_gic_internal.h:43:62: note: expanded from: >>> #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) >>> ^ ~~~~~ >>> >>> 4294967040 is 0xffffff00 and field 'pending' is effectively 8 bits >>> wide, so the masking has no effect except for avoiding the warning. >> >> foo &= ~SOME_FLAGS; is an entirely legitimate and common C idiom, >> and I think clang is being overexuberant in warning here: we should >> disable this warning instead of working around it in the code. This is the only warning generated from QEMU sources, related to -Wconstant-conversion (enabled by -Wall). It would be nice to work around it. How about changing the macros to functions? The use of 's' in the macros look bad and there's no do {} while(0) either to protect the assignment. Similar warning problems exist with -Winitializer-overrides and -Wunused-value (also enabled by -Wall) though and there would be a lot more fixing to do to avoid those. > Also, what version of clang are you using? I don't see this warning > either with MacOS X "Apple clang version 4.0 (tags/Apple/clang-421.0.60) > (based on LLVM 3.1svn)" or with "Ubuntu clang version 3.0-6ubuntu3 > (tags/RELEASE_30/final) (based on LLVM 3.0)". $ clang -v Debian clang version 3.0-6 (tags/RELEASE_30/final) (based on LLVM 3.0) Target: x86_64-pc-linux-gnu Thread model: posix I think this should be the same as Ubuntu. > > -- PMM
On 29 September 2012 12:25, Blue Swirl <blauwirbel@gmail.com> wrote: > On Mon, Sep 24, 2012 at 2:56 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 24 September 2012 14:22, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 23 September 2012 17:33, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> Avoid this warning: >>>> CC arm-softmmu/hw/arm/../arm_gic.o >>>> /src/qemu/hw/arm/../arm_gic.c:432:17: error: implicit truncation from 'unsigned int' to bitfield changes value from 4294967040 to 0 [-Werror,-Wconstant-conversion] >>>> GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> /src/qemu/hw/arm/../arm_gic_internal.h:43:62: note: expanded from: >>>> #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) >>>> ^ ~~~~~ >>>> >>>> 4294967040 is 0xffffff00 and field 'pending' is effectively 8 bits >>>> wide, so the masking has no effect except for avoiding the warning. >>> >>> foo &= ~SOME_FLAGS; is an entirely legitimate and common C idiom, >>> and I think clang is being overexuberant in warning here: we should >>> disable this warning instead of working around it in the code. > > This is the only warning generated from QEMU sources, related to > -Wconstant-conversion (enabled by -Wall). It would be nice to work > around it. How about changing the macros to functions? The use of 's' > in the macros look bad and there's no do {} while(0) either to protect > the assignment. Using inline functions would be cleaner code, I think, so if that happens to fix the warning that's OK I guess. I still think clang is actively wrong here, though: foo.bar &= ~(cm); where foo.bar is an 8 bit unsigned bitfield means that we do the standard integer promotions, so we do the & on 'unsigned int' types. So we're not truncating 4294967040 at all, and provably the value that goes back into bar has the top 24 bits clear anyway. Clang appears to think it's doing the & operation on an 8-bit-wide type? -- PMM
On Sat, Sep 29, 2012 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 29 September 2012 12:25, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Mon, Sep 24, 2012 at 2:56 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 24 September 2012 14:22, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> On 23 September 2012 17:33, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>> Avoid this warning: >>>>> CC arm-softmmu/hw/arm/../arm_gic.o >>>>> /src/qemu/hw/arm/../arm_gic.c:432:17: error: implicit truncation from 'unsigned int' to bitfield changes value from 4294967040 to 0 [-Werror,-Wconstant-conversion] >>>>> GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); >>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> /src/qemu/hw/arm/../arm_gic_internal.h:43:62: note: expanded from: >>>>> #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) >>>>> ^ ~~~~~ >>>>> >>>>> 4294967040 is 0xffffff00 and field 'pending' is effectively 8 bits >>>>> wide, so the masking has no effect except for avoiding the warning. >>>> >>>> foo &= ~SOME_FLAGS; is an entirely legitimate and common C idiom, >>>> and I think clang is being overexuberant in warning here: we should >>>> disable this warning instead of working around it in the code. >> >> This is the only warning generated from QEMU sources, related to >> -Wconstant-conversion (enabled by -Wall). It would be nice to work >> around it. How about changing the macros to functions? The use of 's' >> in the macros look bad and there's no do {} while(0) either to protect >> the assignment. > > Using inline functions would be cleaner code, I think, so if that > happens to fix the warning that's OK I guess. > > I still think clang is actively wrong here, though: > foo.bar &= ~(cm); > where foo.bar is an 8 bit unsigned bitfield means that we do the > standard integer promotions, so we do the & on 'unsigned int' > types. So we're not truncating 4294967040 at all, and provably > the value that goes back into bar has the top 24 bits clear > anyway. Clang appears to think it's doing the & operation on > an 8-bit-wide type? Probably. I tried to change the type of the expression with casts, but then GCC would complain. I guess the problem is linked with the use of bitfields, with uint8_t or uint32_t I would not expect problems. That would limit the number of CPUs though. > > -- PMM
diff --git a/hw/arm_gic_internal.h b/hw/arm_gic_internal.h index db4fad5..219aef3 100644 --- a/hw/arm_gic_internal.h +++ b/hw/arm_gic_internal.h @@ -40,7 +40,8 @@ #define GIC_CLEAR_ENABLED(irq, cm) s->irq_state[irq].enabled &= ~(cm) #define GIC_TEST_ENABLED(irq, cm) ((s->irq_state[irq].enabled & (cm)) != 0) #define GIC_SET_PENDING(irq, cm) s->irq_state[irq].pending |= (cm) -#define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) +#define GIC_CLEAR_PENDING(irq, cm) \ + s->irq_state[irq].pending &= ~(cm) & ALL_CPU_MASK #define GIC_TEST_PENDING(irq, cm) ((s->irq_state[irq].pending & (cm)) != 0) #define GIC_SET_ACTIVE(irq, cm) s->irq_state[irq].active |= (cm) #define GIC_CLEAR_ACTIVE(irq, cm) s->irq_state[irq].active &= ~(cm)
Avoid this warning: CC arm-softmmu/hw/arm/../arm_gic.o /src/qemu/hw/arm/../arm_gic.c:432:17: error: implicit truncation from 'unsigned int' to bitfield changes value from 4294967040 to 0 [-Werror,-Wconstant-conversion] GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /src/qemu/hw/arm/../arm_gic_internal.h:43:62: note: expanded from: #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) ^ ~~~~~ 4294967040 is 0xffffff00 and field 'pending' is effectively 8 bits wide, so the masking has no effect except for avoiding the warning. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/arm_gic_internal.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)