Patchwork gic: avoid a warning from clang

login
register
mail settings
Submitter Blue Swirl
Date Sept. 23, 2012, 4:33 p.m.
Message ID <23216f8d6e3e00bdf667e1c7f942500d3c892a18.1348417996.git.blauwirbel@gmail.com>
Download mbox | patch
Permalink /patch/186247/
State New
Headers show

Comments

Blue Swirl - Sept. 23, 2012, 4:33 p.m.
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(-)
Peter Maydell - Sept. 24, 2012, 1:22 p.m.
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
Peter Maydell - Sept. 24, 2012, 2:56 p.m.
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
Blue Swirl - Sept. 29, 2012, 11:25 a.m.
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
Peter Maydell - Sept. 29, 2012, 11:44 a.m.
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
Blue Swirl - Sept. 29, 2012, 11:54 a.m.
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

Patch

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)