* [Qemu-devel] [PATCH] gic: avoid a warning from clang
@ 2012-09-23 16:33 Blue Swirl
2012-09-24 13:22 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Blue Swirl @ 2012-09-23 16:33 UTC (permalink / raw)
To: qemu-devel; +Cc: blauwirbel
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(-)
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)
--
1.7.2.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] gic: avoid a warning from clang
2012-09-23 16:33 [Qemu-devel] [PATCH] gic: avoid a warning from clang Blue Swirl
@ 2012-09-24 13:22 ` Peter Maydell
2012-09-24 14:56 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2012-09-24 13:22 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] gic: avoid a warning from clang
2012-09-24 13:22 ` Peter Maydell
@ 2012-09-24 14:56 ` Peter Maydell
2012-09-29 11:25 ` Blue Swirl
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2012-09-24 14:56 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] gic: avoid a warning from clang
2012-09-24 14:56 ` Peter Maydell
@ 2012-09-29 11:25 ` Blue Swirl
2012-09-29 11:44 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Blue Swirl @ 2012-09-29 11:25 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] gic: avoid a warning from clang
2012-09-29 11:25 ` Blue Swirl
@ 2012-09-29 11:44 ` Peter Maydell
2012-09-29 11:54 ` Blue Swirl
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2012-09-29 11:44 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] gic: avoid a warning from clang
2012-09-29 11:44 ` Peter Maydell
@ 2012-09-29 11:54 ` Blue Swirl
0 siblings, 0 replies; 6+ messages in thread
From: Blue Swirl @ 2012-09-29 11:54 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-29 11:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-23 16:33 [Qemu-devel] [PATCH] gic: avoid a warning from clang Blue Swirl
2012-09-24 13:22 ` Peter Maydell
2012-09-24 14:56 ` Peter Maydell
2012-09-29 11:25 ` Blue Swirl
2012-09-29 11:44 ` Peter Maydell
2012-09-29 11:54 ` Blue Swirl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).