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

[Lorenzo Stoakes]

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.

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?

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

:)

>              Linus