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] [day] [month] [year] [list]

Message-ID: <YW8B6yMdnIJnFkHE@archlinux-ax161>
Date:   Tue, 19 Oct 2021 10:35:39 -0700
From:   Nathan Chancellor <nathan@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        llvm@...ts.linux.dev, Tor Vic <torvic9@...lbox.org>,
        Dávid Bolvanský <david.bolvansky@...il.com>
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Fix bitwise vs. logical
 warning

Trimming up the replies as we are not really talking about the x86
platform drivers warning anymore.

On Mon, Oct 18, 2021 at 08:27:02PM -1000, Linus Torvalds wrote:
> On Mon, Oct 18, 2021 at 7:00 PM Nathan Chancellor <nathan@...nel.org> wrote:
> >
> > 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.
> 
> Hmm. That's not at all obvious.

I agree, hence the question added to the warning.

> The *much* bigger part is that
> 
>    note: cast one or both operands to int to silence this warning
> 
> which is what I'm complaining about. That note should die. It should
> say "maybe you meant to use a logical or" or something like that.
> 
> > 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
> 
> I don't understand why you seem to continue to ignore the "note"
> message, which makes a completely crazy suggestion.

Sorry, I was not intentionally ignoring the note. As far as I understand
it, it is fairly common for clang to offer a fix up in case the answer
to the question of "did you mean to do this?" is "no" but also offer a
way to shut the warning up in case the answer is "yes, I know what I am
doing", hence the note about casting.

Changing to logical OR is not always the answer, as something like

    int a, b, c;

    changed = foo(&a) | foo(&b) | foo(&c);
    if (changed)
        bar(a, b, c);

no longer works. It could be changed to

    int a, b, c;

    changed = foo(&a);
    changed |= foo(&b);
    changed |= foo(&c);
    if (changed)
        baz(a, b, c);

to make it clearer to both humans and the compiler that every call to
foo() needs to happen and the results are being aggregated. With that,
perhaps the note could be changed to something like:

note: separate expressions with '|=' to silence this warning

Although, that would require that the expression was being assigned to a
variable, rather than being returned or used in a loop like the KVM one
or this other one that is seen in fs/namei.c on big endian ARM
configurations with CONFIG_DCACHE_WORD_ACCESS because has_zero() returns
bool instead of unsigned long on little endian architectures:

fs/namei.c:2149:13: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
        } while (!(has_zero(a, &adata, &constants) | has_zero(b, &bdata, &constants)));
                  ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                   ||
fs/namei.c:2149:13: note: cast one or both operands to int to silence this warning
1 warning generated.

Perhaps the note should just be eliminated entirely so that developers
can be left to try and figure out a way to silence it on their own or
just rework the code to use logical OR or not use boolean types, I do
not know.

There was some discussion upstream around how the warning should be
silenced during the warning's creation. I have added the author of the
warning, David, to this thread to see if he has any insights.

David, you can see the start of this thread here and follow along with
the threading at the bottom:

https://lore.kernel.org/r/CAHk-=wi7hUsTTcmPfZCkUEw51Y3ayq3JJxzFsNgodsxxDyk9Ww@mail.gmail.com/

Cheers,
Nathan