Re: [PATCH v1 1/1] x86/percpu: Cast -1 to argument type when comparing in percpu_add_op()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Oct 16, 2024 at 08:44:56AM -0700, Dave Hansen wrote:
> Andy,
> 
> The subject here is not very informative.  It explains the "what" of the
> patch, but not the "why".
> 
> A better subject might have been:
> 
> 	x86/percpu: Fix clang warning when dealing with unsigned types

Thanks, makes sense!

> > --- a/arch/x86/include/asm/percpu.h
> > +++ b/arch/x86/include/asm/percpu.h
> > @@ -234,9 +234,10 @@ do {									\
> >   */
> >  #define percpu_add_op(size, qual, var, val)				\
> >  do {									\
> > -	const int pao_ID__ = (__builtin_constant_p(val) &&		\
> > -			      ((val) == 1 || (val) == -1)) ?		\
> > -				(int)(val) : 0;				\
> > +	const int pao_ID__ =						\
> > +		(__builtin_constant_p(val) &&				\
> > +			((val) == 1 ||					\
> > +			 (val) == (typeof(val))-1)) ? (int)(val) : 0;	\
> 
> This doesn't _look_ right.

But if feels right if we really want to supply unsigned types here.
Maybe some more magic is needed (like in min() case).

> Let's assume 'val' is a u8.  (u8)-1 is 255, right?  So casting the -1
> over to a u8 actually changed its value.  So the comparison that you
> added would actually trigger for 255:
> 
> 	(val) == (typeof(val))-1))
> 
> 	255 == (u8)-1
> 	255 == 255
> 
> That's not the end of the world because the pao_ID__ still ends up at
> 255 and the lower if() falls into the "add" bucket, but it isn't great
> for reading the macro.  It seems like it basically works on accident.

> Wouldn't casting 'val' over to an int be shorter, more readable, not
> have that logical false match *and* line up with the cast later on in
> the expression?

Maybe more readable, but wouldn't it be theoretically buggy for u64?
I'm talking about the case when u64 == UINT_MAX, which will be true
in your case and false in mine.

>         const int pao_ID__ = (__builtin_constant_p(val) &&
>                               ((val) == 1 || (int)(val) == -1)) ?
> 
>                                 (int)(val) : 0;
> 
> Other suggestions to make it more readable would be welcome.
> 
> Since I'm making comments, I would have really appreciated some extra
> info here like why you are hitting this and nobody else is.  This is bog
> standard code that everybody compiles.  Is clang use _that_ unusual?

Why are you asking me about this? I don't know...

> Or do most clang users just ignore all the warnings?

Same here. I don't know...

Both Qs sounds rhetorical to me.

> Or are you using a bleeding edge version of clang that spits out new warnings
> that other clang users aren't seeing?

AFAICT It's *not* even close to the bleeding edge. It's standard Debian supply.

> Another nice thing would have been to say that this produces the exact
> same code with and without the patch.  Or that you had tested it in
> *some* way.

I have run percpu_test in both cases and also checked code with `bloat-o-meter`
and `cmp -b`. Everything is the same. I even added a test case for the above
mentioned situation.

> It took me a couple of minutes to convince myself that your
> version works and doesn't do something silly like a "dec" if you hand in
> val==255.

It took me much more to find the best solution that appears
not everyone likes :-)

P.S. And as Nick pointed out it's simple `make W=1`,
     what the additional information you wanna see here?
     Care to provide a template?

-- 
With Best Regards,
Andy Shevchenko