Re: [PATCH] platform/x86: thinkpad_acpi: Fix bitwise vs. logical warning

[Nathan Chancellor <nathan@kernel.org>]

On Mon, Oct 18, 2021 at 05:38:09PM -1000, Linus Torvalds wrote:
> On Mon, Oct 18, 2021 at 10:14 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Right, the patch that added the warning explicitly checks for side effects.
> 
> Well, it's a bit questionable. The "side effects" are things like any
> pointer dereference, because it could fault, but if you know that
> isn't an issue, then clang basically ends up complaining about code
> that is perfectly fine. Maybe it was written that way on purpose, like
> the kvm code.
> 
> Now, it's probably not worth keeping that "bitops of booleans" logic -
> if it is a noticeable optimization, it's generally something that the
> compiler should do for us, but basically clang is warning about
> perfectly valid code.
> 
> And what I find absolutely disgusting is the suggested "fix" that
> clang gives you.
> 
> If the warning said "maybe you meant to use a logical or (||)", then
> that would be one thing. But what clang suggests as the "fix" for the
> warning is just bad coding practice.

For what it's worth, the suggested fix is the '||' underneath the
warning text:

In file included from arch/x86/kvm/mmu/tdp_iter.c:5:
arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
        return __is_bad_mt_xwr(rsvd_check, spte) |
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                 ||
arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
1 error generated.

Perhaps that hint should also be added to the warning text, like:

In file included from arch/x86/kvm/mmu/tdp_iter.c:5:
arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands; did you mean logical '||'? [-Werror,-Wbitwise-instead-of-logical]
        return __is_bad_mt_xwr(rsvd_check, spte) |
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                 ||
arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
1 error generated.

It is late for me but I can push that change to the clang developers and
see what they think tomorrow if that would help?

Cheers,
Nathan