RE: [PATCH 4/7] minmax: Simplify signedness check

[David Laight]

From: Lorenzo Stoakes
> Sent: 26 July 2024 10:44
> 
> On Thu, Jul 25, 2024 at 10:02:45AM GMT, Linus Torvalds wrote:
> > On Thu, 25 Jul 2024 at 02:01, David Laight <David.Laight@xxxxxxxxxx> wrote:
> > >
> > > The condition is '>= 0' so it doesn't matter if it is '1' or '0'.
> >
> > Yes, but that's because the whole conditional is so inexplicably complex.
> >
> > But the explanation is:
> >
> > > That gives a 'comparison of unsigned type against 0 is always true' warning.
> > > (The compiler generates that for code in the unused branches of both
> > > __builtin_choose_expr() and _Generic().)
> > > Moving the comparison to the outer level stops all such compiler warnings.
> >
> > Christ. This whole series is a nightmare of "add complexity to deal
> > with stupid issues".
> >
> > But the kernel test robot clearly found even more issues.
> >
> > I think we need to just go back to the old code. It was stupid and
> > limited and caused us to have to be more careful about types than was
> > strictly necessary.
> 
> The problem is simply reverting reveals that seemingly a _ton_ of code has
> come to rely on the relaxed conditions.
> 
> When I went to gather the numbers for my initial report I had to manually
> fix up every case which was rather painful, and that was just a defconfig +
> a few extra options. allmodconfig is likely to be a hellscape.
> 
> I've not dug deep into the ins + outs of this, so forgive me for being
> vague (Arnd has a far clearer understanding) - but it seems that the
> majority of the complexity comes from having to absolutely ensure all this
> works for compile-time constant values.

The problems arise due to some odd uses, not just the requirement for compile-time
constants for on-stack array sizes.

The test robot report is for a test between pointers.
I thought there was one in the build I do - and it doesn't usually generate a warning.
This one is related to the different between a 'compile time constant' and
a 'constant integer expression'.
This is due to is_unsigned_type(t) being (t)-1 > 0 but (type *)x not being
'constant enough' unless 'x' is zero.
Fixable by something like:
	(__if_constexpr((t)-1, (t)-1, 1) > 0)
But that requires two expansions of the type.
Now the type could be that of the unique temporary - but that would make it
all more complicated.

There are fewer min/max of pointers than when constants are needed.
So requiring them be min_ptr() wouldn't be a massive change.

> Arnd had a look through and determined there weren't _too_ many cases where
> we need this (for instance array sizes).
> 
> So I wonder whether we can't just vastly simplify this stuff (and reducing
> the macro expansion hell) for the usual case, and implement something like
> cmin()/cmax() or whatever for the true-constant cases?

I did do that in a patch set from much earlier in the year.
But Linus said they'd need to be MIN() and MAX() and that requires changes
to a few places where those are already defined.

> > But it was also about a million times simpler, and didn't cause build
> > time regressions.

Just bugs because people did min_t(short, 65536, 128) and didn't expect zero.

It has to be said that the chances of a min(negative_value, unsigned_constant)
appearing are pretty slim.
All these tests are there to trap that case.

There is always the option of disabling the tests for 'normal' build, but
leaving them there for (say) the W=1 builds.
Then it won't matter as much if the tests slow down the build a little.

I think I have tried a W=1 build - but there are too many warnings/errors
from other places to get anywhere.
A lot are for 'unsigned_var >= 0' in paths that get optimised away.
The compiler doesn't help!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)