Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR

[Nathan Chancellor <nathan@kernel.org>]

On Tue, Mar 05, 2024 at 11:07:36AM +0200, Jani Nikula wrote:
> Add kconfig to enable -Werror subsystem wide. This is useful for
> development and CI to keep the subsystem warning free, while avoiding
> issues outside of the subsystem that kernel wide CONFIG_WERROR=y might
> hit.
> 
> v2: Don't depend on COMPILE_TEST
> 
> Reviewed-by: Hamza Mahfooz <hamza.mahfooz@amd.com> # v1
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/Kconfig  | 13 +++++++++++++
>  drivers/gpu/drm/Makefile |  3 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 6e853acf15da..c08e18108c2a 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -416,3 +416,16 @@ config DRM_LIB_RANDOM
>  config DRM_PRIVACY_SCREEN
>  	bool
>  	default n
> +
> +config DRM_WERROR
> +	bool "Compile the drm subsystem with warnings as errors"
> +	depends on EXPERT
> +	default n
> +	help
> +	  A kernel build should not cause any compiler warnings, and this
> +	  enables the '-Werror' flag to enforce that rule in the drm subsystem.
> +
> +	  The drm subsystem enables more warnings than the kernel default, so
> +	  this config option is disabled by default.
> +
> +	  If in doubt, say N.

While I understand the desire for an easy switch that maintainers and
developers can use to ensure that their changes are warning free for the
drm subsystem specifically, I think subsystem specific configuration
options like this are actively detrimental to developers and continuous
integration systems that build test the entire kernel. For example, we
turned off CONFIG_WERROR for our Hexagon builds because of warnings that
appear with -Wextra that are legitimate but require treewide changes to
resolve in a manner sufficient for Linus:

https://github.com/ClangBuiltLinux/linux/issues/1285
https://lore.kernel.org/all/CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com/
https://lore.kernel.org/all/20230522105049.1467313-1-schnelle@linux.ibm.com/

But now, due to CONFIG_DRM_WERROR getting enabled by all{mod,yes}config
and -Wextra being unconditionally enabled for DRM, those warnings hard
break the build despite CONFIG_WERROR=n...

https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2eEBDGEqfmMZjGg3ZvDx2af2pde/build.log

Same thing with PowerPC allmodconfig because we see -Wframe-larger-than
that appears because allmodconfig enables CONFIG_KASAN or CONFIG_KCSAN
usually:

https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2eE2HDsODudQGqkMKAPQnId7pRd/build.log

I don't know what the solution for this conflict is through. I guess it
is just the nature of the kernel being a federation of independent
subsystems that want to have their own policies. I suppose we can just
set CONFIG_DRM_WERROR=n and be done with it but I would like to avoid
this issue from spreading to other subsystems because it does not scale
for folks like us who do many builds across many trees.

It would be nice if there was something like CONFIG_WERROR_DIRS or
something that could take a set of directories that should have -Werror
enabled so that you could do something like

  CONFIG_WERROR_DIRS="drivers/gpu/drm"

and have -Werror automatically added to all commands within that
directory like subdir-ccflags-y but it is explicitly opt in on the part
of the developer/tester, rather than just happening to get enabled due
to all{mod,yes}config. No idea if that is feasible or not though.

> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index ea456f057e8a..a73c04d2d7a3 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -30,6 +30,9 @@ subdir-ccflags-y += -Wno-sign-compare
>  endif
>  # --- end copy-paste
>  
> +# Enable -Werror in CI and development
> +subdir-ccflags-$(CONFIG_DRM_WERROR) += -Werror
> +
>  drm-y := \
>  	drm_aperture.o \
>  	drm_atomic.o \
> -- 
> 2.39.2
>