Message-ID: <20251222171457.0b4848a0@pumpkin>
Date: Mon, 22 Dec 2025 17:14:57 +0000
From: David Laight <david.laight.linux@...il.com>
To: "Arnd Bergmann" <arnd@...db.de>
Cc: "Linus Torvalds" <torvalds@...ux-foundation.org>, "Nathan Chancellor"
 <nathan@...nel.org>, "Nicolas Schier" <nsc@...nel.org>,
 linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] kbuild: Only enable
 -Wtautological-constant-out-of-range-compare for W=2

On Mon, 22 Dec 2025 11:20:18 +0100
"Arnd Bergmann" <arnd@...db.de> wrote:

> On Sat, Dec 20, 2025, at 13:15, David Laight wrote:
> > On Sat, 20 Dec 2025 11:27:13 +0100  
> >> 
> >> This does seem like a completely sensible warning to me, and it's
> >> always been enabled by default. I see three patches in the git history
> >> (all from Nathan), which all make sense as well.
> >>   
> >> > Inside FIELD_PREP_CONST(mask, val) there is (with the patch, and if I've
> >> > typed it correctly):
> >> > 	BUILD_BUG_ON_ZERO(!(mask) || (mask) & ((mask) + ((mask) & -(mask)))))
> >> > to check the mask is non-zero and contiguous bits.    
> >> 
> >> I think the problem is (as so often) the linux/bitfield.h headers
> >> making things way too complicated. That condition makes no sense to
> >> me, and neither would I expect a compiler to make sense of it either.  
> >
> > It is simple really :-)
> > -mask is (~mask + 1) so its lowest set bit is the same at that of mask.
> > Adding mask changes the adjacent 1s to zeros.
> > Anding with mask is then any high bits that are the same in both.
> > So is non-zero if mask has noncontiguous bits in it.  
> 
> The bit that I find most confusing here is how you have a boolean '||'
> operation of two integers, but then interpret the result as an
> integer again.

I'm not sure what you are getting at.
The BUILD_BUG() macros want a 'boolean' argument.
The _ON_ZERO() is the return value, nothing to do with the argument.
So LHS of the || is 'boolean' and the RHS has the implicit conversion.

> > Adding ' == 0' and ' != 0' would just make the line longer.  
> 
> I don't think we care about the link length here at all.
> Splitting it up into two BUILD_BUG_ON() or BUILD_BUG_ON_ZERO()
> lines would help here as well.

I'm merging them to reduce bloat.....
It's not as though either test fails often enough to need separate
error messages.

> >> If there is no way to express those conditions more clearly, I would
> >> prefer removing the BUILD_BUG_ON stuff from the bitfield.h header,
> >> it keeps causing way more false positives than finding actual bugs
> >> with the input.  
> >
> > I was just trying to reduce the .i lines line from 18KB for a typical use.  
> 
> That seems like a worthwhile goal, but I think the only way to
> make an actual impact here is to reduce the fan-out and evaluate
> 'mask' less than the current five times in that line (plus additional
> evalations. If the 'mask' value is defined using complex macros
> like ilog2() or max3() already, the expansion explodes.

It usually comes from GENMASK() and that is several hundred characters.
Changing type_max(type) to (type)-1 would help that a lot (type is unsigned).
Even changing type_max() to do (2 * (x - 1) + 1) instead of ((x - 1) + x)
saves quite a long expansion.

> Unfortunately the constant version of these macros can't use
> compound statements, otherwise we could use an __auto_type temporary
> here.

Fortunately there aren't that many uses of the _CONST version.
The change to bitmask.h that show this 'bug' used _auto_type
to reduce bloat for the common case.

> 
> > Probably the only useful check is statically_true(hi < lo) in GENMASK.  
> 
> Agreed, that one is clearly worth keeping.
> 
>     Arnd

Powered by blists - more mailing lists