Re: [PATCH] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()

From: Matthias Kaehlcke <mka@chromium.org>
To: Joe Perches <joe@perches.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	rkrcmar@redhat.com, Thomas Gleixner <tglx@linutronix.de>,
	hpa@zytor.com, x86@kernel.org, kvm@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] kvm: x86: mmu: Add cast to negated bitmasks in update_permission_bitmask()
Date: Tue, 19 Jun 2018 11:36:22 -0700	[thread overview]
Message-ID: <20180619183622.GB169030@google.com> (raw)
In-Reply-To: <66b5e5c3ed82a5e1559419deb841c57bf0751fd3.camel@perches.com>

On Tue, Jun 19, 2018 at 11:07:47AM -0700, Joe Perches wrote:
> On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote:
> > On 19/06/2018 19:23, Joe Perches wrote:
> > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote:
> > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > > 
> > > > > On 15/06/2018 20:45, Nick Desaulniers wrote:
> > > > > > > 
> > > > > > > > In any case I think it it preferable to fix the code over disabling
> > > > > > > > the warning, unless the warning is bogus or there are just too many
> > > > > > > > occurrences.
> > > > > > > 
> > > > > > > Maybe.
> > > > > > 
> > > > > > Spurious warning today, actual bug tomorrow?  I prefer to not to
> > > > > > disable warnings wholesale.  They don't need to find actual bugs to be
> > > > > > useful.  Flagging code that can be further specified does not hurt.
> > > > > > Part of the effort to compile the kernel with different compilers is
> > > > > > to add warning coverage, not remove it.  That said, there may be
> > > > > > warnings that are never useful (or at least due to some invariant that
> > > > > > only affects the kernel).  I cant think of any off the top of my head,
> > > > > > but I'm also not sure this is one.
> > > > > 
> > > > > This one really makes the code uglier though, so I'm not really inclined
> > > > > to applying the patch.
> > > > 
> > > > Note that of the three variables (w, u, x), only u is used later on.
> > > > What about declaring them as negated with the cast, that way there's
> > > > no cast in a ternary?
> > > 
> > > It'd be simpler to cast in the BYTE_MASK macro itself
> > 
> > I don't think that would work, as the ~ would be done on a zero-extended
> > signed int.
> 
> True, but the whole concept is dubious.
> The implicit casts are all over the place,
> not just in the file.

Would that have been any different with the solution you proposed (if
it worked)?

Apparently both gcc and clang limit the warning to the potentially
more problematic case where a value with sign bit is promoted.