Re: [PATCH 2/2] drm/i915: enable W=1 warnings by default

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 06 Oct 2023, Nathan Chancellor <nathan@kernel.org> 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@arndb.de>
>> Cc: Nick Desaulniers <ndesaulniers@google.com>
>> Cc: Nathan Chancellor <nathan@kernel.org>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> One meta comment and review comment below. Feel free to carry forward
>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>
> 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