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
