Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 18 Oct 2021 22:00:11 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Nick Desaulniers <ndesaulniers@google.com>,
        Henrique de Moraes Holschuh <hmh@hmh.eng.br>,
        Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        ibm-acpi-devel@lists.sourceforge.net,
        platform-driver-x86@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        llvm@lists.linux.dev, Tor Vic <torvic9@mailbox.org>
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Fix bitwise vs. logical
 warning
Message-ID: <YW5Q253s0Y+zYxdi@archlinux-ax161>
References: <20211018182537.2316800-1-nathan@kernel.org>
 <CAKwvOdn4_DrgaZoAo-v7CbYurNUpfAK5tnzT023=WCDzkmYQVg@mail.gmail.com>
 <CAHk-=wi7hUsTTcmPfZCkUEw51Y3ayq3JJxzFsNgodsxxDyk9Ww@mail.gmail.com>
 <CAKwvOd=wGjd_L1703Y9Kngcr9-_wTvcRLToiydXYkR=S_9xWDw@mail.gmail.com>
 <CAHk-=wjwOnrHXDSqnmhiKujk=5XieJFvfnQwc2WKOKFwzcqvaA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wjwOnrHXDSqnmhiKujk=5XieJFvfnQwc2WKOKFwzcqvaA@mail.gmail.com>
Precedence: bulk

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