Re: [PATCH v4 1/2] x86: fix bitops.h warning with a moved cast

[Jesse Brandeburg <jesse.brandeburg@intel.com>]

On Mon, 24 Feb 2020 10:54:02 +0100 Peter wrote:
> On Sat, Feb 22, 2020 at 11:39:57AM +0200, Andy Shevchenko wrote:
> > On Sat, Feb 22, 2020 at 2:04 AM Jesse Brandeburg  
> 
> > > -#define CONST_MASK(nr)                 (1 << ((nr) & 7))
> > > +#define CONST_MASK(nr)                 ((u8)1 << ((nr) & 7))
> > >
> > >  static __always_inline void
> > >  arch_set_bit(long nr, volatile unsigned long *addr)
> > > @@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr)
> > >         if (__builtin_constant_p(nr)) {
> > >                 asm volatile(LOCK_PREFIX "orb %1,%0"
> > >                         : CONST_MASK_ADDR(nr, addr)
> > > -                       : "iq" ((u8)CONST_MASK(nr))
> > > +                       : "iq" (CONST_MASK(nr))  
> 
> Note how this is not equivalent, the old code actually handed in a u8
> while the new code hands int. By moving the (u8) cast into the parens,
> you casl 1 to u8, which then instantly gets promoted to 'int' due to the
> '<<' operator.

True. Which is why I had decided to use the strongly typed local
variables, which as I recall you mentioned were "sad", so I had tried
to fix it with the simpler changes. Everything is a negotiation with
the compiler and tools here, about how clearly you can communicate the
code's intent and functionality while still keeping it performant.

> > >                         : "memory");
> > >         } else {
> > >                 asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0"
> > > @@ -74,7 +74,7 @@ arch_clear_bit(long nr, volatile unsigned long *addr)
> > >         if (__builtin_constant_p(nr)) {
> > >                 asm volatile(LOCK_PREFIX "andb %1,%0"
> > >                         : CONST_MASK_ADDR(nr, addr)
> > > -                       : "iq" ((u8)~CONST_MASK(nr)));
> > > +                       : "iq" (CONST_MASK(nr) ^ 0xff));  
> > 
> > I'm wondering if the original, by Peter Z, order allows us to drop
> > (u8) casting in the CONST_MASK completely.  
> 
> I'm thinking it's all nonsense anyway :-), the result of either << or ^
> is always promoted to int anyway.

Yeah, I realize this is *all* nonsense, but I *do* see value in making
the code not generate sparse warnings, as long as the end result
doesn't generate code change. It allows you to run more tools to find
bugs with less false positives.

> The sparse complaint was that ~CONST_MASK(nr) had high bits set which
> were lost, which is true, but a copmletely stupid warning IMO.
> 
> By using 0xff ^ CONST_MASK(nr), those bits will not be set and will not
> be lost.
> 
> None of that has anything to do with where we place a pointless cast
> more or less.

Well now that we have the test module, I'll check that the simplest
possible patch of just changing the one line for ~CONST_MASK(nr) to
0xFF ^ CONST_MASK(nr) will fix the issue.