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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Oct 2021 22:00:11 -0700
From:   Nathan Chancellor <nathan@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Henrique de Moraes Holschuh <hmh@....eng.br>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        ibm-acpi-devel@...ts.sourceforge.net,
        platform-driver-x86@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        llvm@...ts.linux.dev, Tor Vic <torvic9@...lbox.org>
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Fix bitwise vs. logical
 warning

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@...gle.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