Re: [PATCH 1/1] kbuild: Only enable -Wtautological-constant-out-of-range-compare for W=2

[David Laight <david.laight.linux@gmail.com>]

On Sat, 20 Dec 2025 11:27:13 +0100
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Fri, Dec 19, 2025, at 23:18, David Laight wrote:
> > On Fri, 19 Dec 2025 13:12:31 -0700 Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > Somewhere I got confused and must have looked at the wrong email (or just
> > failed to separate two very long warning names).
> > The actual warning was:
> >  
> >>> drivers/gpu/drm/xe/xe_guc.c:639:19: error: converting the result of '<<' to a boolean always evaluates to true [-Werror,-Wtautological-constant-compare]    
> >      639 |                 klvs[count++] = 
> > PREP_GUC_KLV_TAG(OPT_IN_FEATURE_EXT_CAT_ERR_TYPE);  
> 
> 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.

Adding ' == 0' and ' != 0' would just make the line longer.

> 
> 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.

But maybe the whole set of checks is entirely pointless.
The simple FIELD_PREP() is just ((val * (mask & -mask)) & mask).
FIELD_GET() can be (reg & mask)/(mask & -mask) for constants, but that
isn't 'nice' if mask is a variable, (reg & mask) >> ffs(mask) is better.
But you only want to use builtin_ffs() for constants so do need to
select between __ffs() and __ffs64() for variables - three cases.

There is also no point in the u8 and u16 variants (same for GENMASK()).
The values get promoted to 'int' and 'unsigned int' would be better.

Maybe I'll do a 'dump all the crap' commit.

Probably the only useful check is statically_true(hi < lo) in GENMASK.

	David

> 
>      Arnd