All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Laight <david.laight.linux@gmail.com>
To: Nathan Chancellor <nathan@kernel.org>
Cc: Yury Norov <ynorov@nvidia.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Arnd Bergmann <arnd@arndb.de>,
	linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org,
	Yury Norov <yury.norov@gmail.com>,
	Lucas De Marchi <lucas.demarchi@intel.com>,
	Jani Nikula <jani.nikula@intel.com>,
	Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH next 02/14] kbuild: Add W=c for additional compile time checks
Date: Tue, 3 Feb 2026 11:14:13 +0000	[thread overview]
Message-ID: <20260203111413.7cf29fa3@pumpkin> (raw)
In-Reply-To: <20260203044726.GA3049363@ax162>

On Mon, 2 Feb 2026 21:47:26 -0700
Nathan Chancellor <nathan@kernel.org> wrote:

> On Mon, Feb 02, 2026 at 08:07:43PM +0000, David Laight wrote:
> > On Mon, 2 Feb 2026 13:33:22 -0500
> > Yury Norov <ynorov@nvidia.com> wrote:
> >   
> > > On Wed, Jan 21, 2026 at 02:57:19PM +0000, david.laight.linux@gmail.com wrote:  
> > > > From: David Laight <david.laight.linux@gmail.com>
> > > > 
> > > > Some compile time checks significantly bloat the pre-processor output
> > > > (particularly when the get nested).
> > > > Since the checks aren't really needed on every compilation enable with
> > > > W=c (adds -DKBUILD_EXTRA_WARNc) so the checks can be enabled per-build.
> > > > Make W=1 imply W=c so the build-bot includes the checks.
> > > > 
> > > > As well as reducing the bloat from existing checks (like those in
> > > > GENMASK() and FIELD_PREP()) it lets additional checks be added
> > > > while there are still 'false positives' without breaking normal builds.
> > > > 
> > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>    
> > > 
> > > Honestly I don't understand this. AFAIU, you've outlined a list of
> > > compiler warnings that slow the compilation down, and you group them
> > > under 'W=c' option.
> > > 
> > > What is the use case for it outside of your series. I think, a typical
> > > user would find more value in an option that enables some warnings but
> > > doesn't sacrifices performance.  
> > 
> > All the compile-time warnings slow down the compilation.
> > Even apparently trivial ones (like the check in the generic READ_ONCE()
> > that the size is 1, 2, 4 or 8 bytes) have a measurable effect.    
> 
> What kind of difference are we talking about here across a regular
> build?

IIRC a couple of %.
I repeated a few builds with/without and it was much greater than the
variation between the builds.
That is a pretty trivial test, but there are a lot of READ_ONCE().
Find a few of those and the difference is significant.

> 
> > The code a typical user compiles won't have anything that trips any of
> > the compile-time asserts.
> > They only really happen when compiling new code or adding new checks.
> >   
> > > Can you consider flipping the 'W=c' behavior?  
> > 
> > Why, most of the time you don't want the checks because the code is
> > known to pass them all.
> > 
> > It also means it can be used for new checks before all the bugs (and
> > false positives) have been fixed.
> > I did think of just enabling the checks for W=1 builds, but that makes
> > it far to hard to enable them.
> > As it is you can use W=ce to enable them and stop on warnings and errors.
> > 
> > Also W=xxx can only really add checks (there are some checks for it being
> > non-empty). Documenting that W=x stopped the 'x' checks being done
> > would be painful.  
> 
> Yeah, I don't think we should be overloading the meaning of W=.

I did thing of supporting W=-e so that you can do W=1-e on a config
that enables WERROR, since there isn't much chance of a W=1e build
succeeding any time soon.

> > > Can you please explicitly mention warnings included in W=c vs W=1? Can
> > > you report compilation time for W=0, W=1 and W=c? What if one needs to
> > > enable fast/slow warnings from 2nd or 3rd level? Would W=2c or W=3c
> > > work in this case?  
> > 
> > The W=123 options are all completely independent, my W=c is the same.
> > I'm not sure it is sane to run W=2 rather than W=12, but it is allowed.
> > 
> > I made W=1 imply W=1c so that the build bot would pick up the extra checks.
> > Apart from that all the 'W' flags are independent.
> > W=123 fiddle with the command line -W options and set -DKBUILD_EXTRA_WARN[123]
> > so that files can include extra checks.
> > W=c just sets the equivalent -D option.  
> 
> Why even have a different option then? You mention above placing the
> checks in W=1 makes them too hard to enable but then you say that you
> made W=1 imply W=c?

What I meant was it is hard to see the errors in a W=1 build.
So I wanted the enable the checks without picking up W=1.

> 
> > > What does this 'c' stands for?  
> > 
> > Anything you want it to :-)
> > Discussion session arranged for 2pm-5pm by the bike shed.  
> 
> W=c already exists, it is documented as enabling extra Kconfig checks in
> 'make help'. Maybe W=x for "ex"pensive checks? Or maybe if we really
> want W=c, maybe rename W=c to W=k for Kconfig? I do not really have a
> strong preference.

It was 'c' for Compile time (a bit silly - but they are source 'compile-time'
tests rather then 'run-time' ones).
I'd only looked in makefile.warn for other uses.

> > In some sense it is 'more warnings than default, but not as many as W=1'.
> > Like a lot of the W=1 warnings I wanted to be able to pick up 'code quality'
> > issues without breaking the build for normal people.  
> 
> So something like a W=0.5? :)

That is really what I was looking for.
Perhaps setting -DKBUILD_WARN_LEVEL=level*10 would do?
So W=1 would set 10, W=2 20 (etc) with some scheme for setting intermediate
values (or at least (say) 5).

> > There are definitely some other checks that could be relegated to W=c
> > once it has been added.
> > 
> > I'd also like to add some checks to min_t/max_t/clamp_t to pick up the
> > worst of the dodgy/broken code without having to get all the patches
> > accepted before the test itself is committed.
> > Tests for code like clamp_t(u32, u64_val, 0, ~0u) (yes there are some)
> > tend to get very long and may be problematic if enabled by default
> > (I got burnt by the 10MB expansion of nested min().)  
> 
> I do not have a super strong opinion about this overall but it feels
> like this is a bit of a slippery slope with warning fragmentation. I
> think I would rather see these be treated like regular compiler warnings
> where the majority of instances are cleaned up then it is added to W=1
> to keep the tree clean.

That is slightly different because (mostly) they are warnings from a
new compiler version so are picked up by people running the new compiler
before they break things and then added to W=1.
Adding an extra check to an existing header needs the change committing
to get test coverage somewhat earlier - maybe to force some reluctant
maintainers to actually fix their code.

I definitely needed to conditionally include the checks with W=e but
without W=1, this seemed the best way to do it.

	David

> 
> Cheers,
> Nathan


  reply	other threads:[~2026-02-03 11:14 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-21 14:57 [PATCH next 00/14] bits: De-bloat expansion of GENMASK() david.laight.linux
2026-01-21 14:57 ` [PATCH next 01/14] overflow: Reduce expansion of __type_max() david.laight.linux
2026-01-21 20:59   ` Kees Cook
2026-02-02 16:45   ` Yury Norov
2026-01-21 14:57 ` [PATCH next 02/14] kbuild: Add W=c for additional compile time checks david.laight.linux
2026-02-02 18:33   ` Yury Norov
2026-02-02 20:07     ` David Laight
2026-02-03  4:47       ` Nathan Chancellor
2026-02-03 11:14         ` David Laight [this message]
2026-02-03 19:41       ` Yury Norov
2026-01-21 14:57 ` [PATCH next 03/14] media: videobuf2-core: Use static_assert() for sanity check david.laight.linux
2026-01-21 14:57 ` [PATCH next 04/14] media: atomisp: " david.laight.linux
2026-01-21 14:57 ` [PATCH next 05/14] ixgbevf: Use C test for PAGE_SIZE > IXGBE_MAX_DATA_PER_TXD david.laight.linux
2026-01-23 15:44   ` Simon Horman
2026-01-21 14:57 ` [PATCH next 06/14] asm-generic: include linux/bits.h not vdso/bits.h david.laight.linux
2026-01-21 14:57 ` [PATCH next 07/14] x86/tlb: " david.laight.linux
2026-01-21 14:57 ` [PATCH next 08/14] bits: simplify GENMASK_TYPE() david.laight.linux
2026-02-08  2:36   ` Yury Norov
2026-02-09  9:42     ` David Laight
2026-01-21 14:57 ` [PATCH next 09/14] bits: Change BIT_U8/16() and GENMASK_U8/16() to have unsigned values david.laight.linux
2026-01-21 14:57 ` [PATCH next 10/14] bits: Fix assmebler expansions of GENMASK_Uxx() and BIT_Uxx() david.laight.linux
2026-02-08  3:31   ` Yury Norov
2026-02-08 11:42     ` David Laight
2026-02-08 21:20       ` Yury Norov
2026-02-08 22:27         ` David Laight
2026-01-21 14:57 ` [PATCH next 11/14] bit: Strengthen compile-time tests in GENMASK() and BIT() david.laight.linux
2026-01-21 18:43   ` Vincent Mailhol
2026-01-21 19:14     ` David Laight
2026-01-22  1:11   ` kernel test robot
2026-01-22 10:25     ` David Laight
2026-01-22 20:10       ` David Laight
2026-01-22  4:41   ` kernel test robot
2026-01-22 10:33     ` David Laight
2026-01-22 14:26       ` Andy Shevchenko
2026-01-22 14:55         ` David Laight
2026-01-23  1:25         ` Philip Li
2026-01-23  8:01           ` Vincent Mailhol
2026-01-23  8:11             ` Andy Shevchenko
2026-01-23  8:20               ` Al Viro
2026-01-23  8:24                 ` Andy Shevchenko
2026-01-23  8:32                   ` Vincent Mailhol
2026-01-23  8:46                     ` Andy Shevchenko
2026-01-23  1:24       ` Philip Li
2026-01-21 14:57 ` [PATCH next 12/14] bits: move the defitions of BIT() and BIT_ULL() back to linux/bits.h david.laight.linux
2026-01-21 15:17   ` Thomas Weißschuh
2026-01-21 19:24     ` David Laight
2026-01-22  7:39       ` Thomas Weißschuh
2026-01-22  0:50   ` kernel test robot
2026-01-22  1:23   ` kernel test robot
2026-01-22 10:30     ` David Laight
2026-02-07 22:40   ` Thomas Gleixner
2026-02-08  4:23     ` Yury Norov
2026-01-21 14:57 ` [PATCH next 13/14] test_bits: Change all the tests to be compile-time tests david.laight.linux
2026-02-08  4:37   ` Yury Norov
2026-02-08 11:32     ` David Laight
2026-01-21 14:57 ` [PATCH next 14/14] test_bits: include some invalid input tests for GENMASK_INPUT_CHECK() david.laight.linux

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260203111413.7cf29fa3@pumpkin \
    --to=david.laight.linux@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=jani.nikula@intel.com \
    --cc=keescook@chromium.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lucas.demarchi@intel.com \
    --cc=mailhol.vincent@wanadoo.fr \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mingo@kernel.org \
    --cc=nathan@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=ynorov@nvidia.com \
    --cc=yury.norov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.