On 27/05/2020 23.54, Eric Blake wrote:
> On 5/27/20 4:40 PM, Peter Maydell wrote:
>> On Wed, 27 May 2020 at 20:21, Philippe Mathieu-Daudé
>> <1881...@bugs.launchpad.net> wrote:
>>>
>>> Public bug reported:
>>>
>>> Last time I built QEMU was on commit
>>> d5c75ec500d96f1d93447f990cd5a4ef5ba27fae,
>>> I just pulled to fea8f3ed739536fca027cf56af7f5576f37ef9cd and now get:
>>>
>>> CC lm32-softmmu/fpu/softfloat.o
>>> fpu/softfloat.c:3365:13: error: bitwise negation of a boolean
>>> expression; did you mean logical negation? [-Werror,-Wbool-operation]
>>> absZ &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> !
>>
>>
>> "(x & y)" is not a boolean expression, so we should report this to clang
>> as a bug (I assume what they actually are trying to complain about is
>> bitwise AND with a boolean expression).
>
> We have:
>
> uint64_t &= ~ ( ( ( int8_t ^ int ) == int ) & bool )
>
> which is
>
> uint64_t &= ~ ( bool & bool )
>
> which is then
>
> uint64_t &= ~ ( int )
>
> resulting in one of:
>
> uint64_t &= 0xffffffffffffffff
> uint64_t &= 0xfffffffffffffffe
>
> It is a very odd way of stating that 'if this condition is true, mask
> out the least-significant-bit'. In general, 'bool & bool' is used where
> the side-effect-skipping 'bool && bool' is inappropriate; I'm a bit
> surprised that clang is not questioning whether we meant '&&' instead of
> '&' (the two operators give the same effect in this case).
>
> You are right that clang is fishy for calling it logical negation of a
> bool, when it is really logical negation of an int, but we are also
> fishy in that we are using bitwise AND of two bools as an int in the
> first place.
>
> Regardless of whether clang changes, would it be better to write the
> code as:
>
> if (((roundBits ^ 0x40) == 0) && roundNearestEven) {
> absZ &= ~1;
> }
I agree, that's also much better to read.
And FWIW, WinUAE fixed a similar problem in the same way recently:
https://github.com/tonioni/WinUAE/commit/51f5e7bfc39cf37daf7283
So I think this is the right way to go. Could you send your suggestion
as a proper patch?
Thanks,
Thomas
