Re: [Qemu-devel] [PATCH] gic: avoid a warning from clang

[Peter Maydell]

On 29 September 2012 12:25, Blue Swirl <address@hidden> wrote:
> On Mon, Sep 24, 2012 at 2:56 PM, Peter Maydell <address@hidden> wrote:
>> On 24 September 2012 14:22, Peter Maydell <address@hidden> wrote:
>>> On 23 September 2012 17:33, Blue Swirl <address@hidden> 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