On Fri, 06 Oct 2023, Nathan Chancellor <nathan@xxxxxxxxxx> wrote: > On Fri, Oct 06, 2023 at 03:34:47PM +0300, Jani Nikula wrote: >> We enable a bunch more compiler warnings than the kernel >> defaults. However, they've drifted to become a unique set of warnings, >> and have increasingly fallen behind from the W=1 set. >> >> Align with the W=1 warnings from scripts/Makefile.extrawarn for clarity, >> by copy-pasting them with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it >> easier to compare in the future. >> >> Cc: Arnd Bergmann <arnd@xxxxxxxx> >> Cc: Nick Desaulniers <ndesaulniers@xxxxxxxxxx> >> Cc: Nathan Chancellor <nathan@xxxxxxxxxx> >> Cc: Masahiro Yamada <masahiroy@xxxxxxxxxx> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> > > One meta comment and review comment below. Feel free to carry forward > > Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx> > > on future revisions. Thanks! > >> --- >> >> An alternative or future option would be to have Makefile.extrawarn >> assign W=1 etc. flags to intermediate variables, say KBUILD_CFLAGS_W1, >> like this: >> >> KBUILD_CFLAGS_W1 += -Wextra -Wunused -Wno-unused-parameter >> etc... >> >> export KBUILD_CFLAGS_W1 >> >> ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) >> >> KBUILD_CFLAGS += $(KBUILD_CFLAGS_W1) >> >> else >> etc... >> >> and then drivers and subsystems could simply use: >> >> subdir-ccflags-y += $(KBUILD_CFLAGS_W1) >> >> to enable and remain up-to-date with W=1 warnings. > > This has definitely come up a few times and while I am generally in > favor of something like this, it makes adding warnings to W=1 a little > bit harder because when we add warnings to W=1, we typically are not > concerned with breaking the build, as W=1 is not the default build. If a > subsystem has opted into "whatever the current W=1 is" by default, > changes to W=1 will have to be reviewed/tested within a normal build. > > Doing something like this patch with a more regular cadence (maybe every > update after the merge window) seems like a reasonable compromise to me, > although I know that means more work for individual subsystem > maintainers. Makes sense. > >> --- >> drivers/gpu/drm/i915/Makefile | 33 ++++++++++++++++++--------------- >> 1 file changed, 18 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile >> index 623f81217442..0485157054fc 100644 >> --- a/drivers/gpu/drm/i915/Makefile >> +++ b/drivers/gpu/drm/i915/Makefile >> @@ -3,22 +3,25 @@ >> # Makefile for the drm device driver. This driver provides support for the >> # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. >> >> -# Add a set of useful warning flags and enable -Werror for CI to prevent >> -# trivial mistakes from creeping in. We have to do this piecemeal as we reject >> -# any patch that isn't warning clean, so turning on -Wextra (or W=1) we >> -# need to filter out dubious warnings. Still it is our interest >> -# to keep running locally with W=1 C=1 until we are completely clean. >> -# >> -# Note the danger in using -Wextra is that when CI updates gcc we >> -# will most likely get a sudden build breakage... Hopefully we will fix >> -# new warnings before CI updates! >> -subdir-ccflags-y := -Wextra >> -subdir-ccflags-y += -Wno-unused-parameter >> -subdir-ccflags-y += -Wno-type-limits >> -subdir-ccflags-y += -Wno-missing-field-initializers >> -subdir-ccflags-y += -Wno-sign-compare >> -subdir-ccflags-y += -Wno-shift-negative-value > > As the test robot points out, you'll want to keep these four, as they > are only enabled for W=2 or W=3. With this diff on top of these two > patches: Right. I was confused by the refactoring done in Makefile.extrawarn, and didn't notice that the disables were moved to else branches of the checks for W=2 and W=3. BR, Jani. > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 0485157054fc..9c4e09c8aa4e 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -21,6 +21,12 @@ subdir-ccflags-y += $(call cc-option, -Wstringop-overflow) > subdir-ccflags-y += $(call cc-option, -Wstringop-truncation) > # --- end copy-paste > > +# The following turn off the warnings enabled by -Wextra > +subdir-ccflags-y += -Wno-type-limits > +subdir-ccflags-y += -Wno-missing-field-initializers > +subdir-ccflags-y += -Wno-sign-compare > +subdir-ccflags-y += -Wno-shift-negative-value > + > # Enable -Werror in CI and development > subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > I can continue to build i915 warning free with ARCH=x86_64 allmodconfig > using all supported versions of LLVM for building the kernel. > >> +# Unconditionally enable W=1 warnings locally >> +# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn >> +subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter >> +subdir-ccflags-y += -Wmissing-declarations >> +subdir-ccflags-y += $(call cc-option, -Wrestrict) >> +subdir-ccflags-y += -Wmissing-format-attribute >> +subdir-ccflags-y += -Wmissing-prototypes >> +subdir-ccflags-y += -Wold-style-definition >> +subdir-ccflags-y += -Wmissing-include-dirs >> subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) >> +subdir-ccflags-y += $(call cc-option, -Wunused-const-variable) >> +subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned) >> +subdir-ccflags-y += $(call cc-option, -Wformat-overflow) >> +subdir-ccflags-y += $(call cc-option, -Wformat-truncation) >> +subdir-ccflags-y += $(call cc-option, -Wstringop-overflow) >> +subdir-ccflags-y += $(call cc-option, -Wstringop-truncation) >> +# --- end copy-paste >> + >> +# Enable -Werror in CI and development >> subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror >> >> # Fine grained warnings disable >> -- >> 2.39.2 >> -- Jani Nikula, Intel
