[RESEND v3 0/2] drm: enable W=1 warnings by default across the subsystem

[RESEND v3 0/2] drm: enable W=1 warnings by default across the subsystem

[Jani Nikula]

Resend of [1] with an intent to merge after the CI results come in. This
is aiming for v6.10, so we'll have maximal time to find all the issues
my configs didn't catch.

I built this on x86-64 (both gcc and clang), arm and arm64, and
everything passed.

BR,
Jani.

[1] https://lore.kernel.org/all/cover.1706797803.git.jani.nikula@intel.com/


Jani Nikula (2):
  drm: enable (most) W=1 warnings by default across the subsystem
  drm: Add CONFIG_DRM_WERROR

 drivers/gpu/drm/Kconfig  | 13 +++++++++++++
 drivers/gpu/drm/Makefile | 28 ++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

-- 
2.39.2

[RESEND v3 1/2] drm: enable (most) W=1 warnings by default across the subsystem

[Jani Nikula]

At least the i915 and amd drivers enable a bunch more compiler warnings
than the kernel defaults.

Extend most of the W=1 warnings to the entire drm subsystem by
default. Use the copy-pasted warnings from scripts/Makefile.extrawarn
with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it easier to compare and
keep up with them in the future.

This is similar to the approach currently used in i915.

Some of the -Wextra warnings do need to be disabled, just like in
Makefile.extrawarn, but take care to not disable them for W=2 or W=3
builds, depending on the warning.

There are too many -Wformat-truncation warnings to cleanly fix up front;
leave that warning disabled for now.

v3:
- Drop -Wmissing-declarations (already enabled by default)
- Drop -Wmissing-prototypes (already enabled by default)

v2:
- Drop -Wformat-truncation (too many warnings)
- Drop -Wstringop-overflow (already enabled by default)

Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Pan, Xinhui <Xinhui.Pan@amd.com>
Cc: Karol Herbst <kherbst@redhat.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Sean Paul <sean@poorly.run>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Hamza Mahfooz <hamza.mahfooz@amd.com>
Acked-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Acked-by: Danilo Krummrich <dakr@redhat.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/Makefile | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6eb2b553a163..ea456f057e8a 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -5,6 +5,31 @@
 
 CFLAGS-$(CONFIG_DRM_USE_DYNAMIC_DEBUG)	+= -DDYNAMIC_DEBUG_MODULE
 
+# 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 += $(call cc-option, -Wrestrict)
+subdir-ccflags-y += -Wmissing-format-attribute
+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)
+# FIXME: fix -Wformat-truncation warnings and uncomment
+#subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
+subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
+# The following turn off the warnings enabled by -Wextra
+ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
+subdir-ccflags-y += -Wno-missing-field-initializers
+subdir-ccflags-y += -Wno-type-limits
+subdir-ccflags-y += -Wno-shift-negative-value
+endif
+ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),)
+subdir-ccflags-y += -Wno-sign-compare
+endif
+# --- end copy-paste
+
 drm-y := \
 	drm_aperture.o \
 	drm_atomic.o \
-- 
2.39.2

Re: [RESEND v3 1/2] drm: enable (most) W=1 warnings by default across the subsystem

[Guenter Roeck]

Hi,

On Tue, Mar 05, 2024 at 11:07:35AM +0200, Jani Nikula wrote:
> At least the i915 and amd drivers enable a bunch more compiler warnings
> than the kernel defaults.
> 
> Extend most of the W=1 warnings to the entire drm subsystem by
> default. Use the copy-pasted warnings from scripts/Makefile.extrawarn
> with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it easier to compare and
> keep up with them in the future.
> 
> This is similar to the approach currently used in i915.
> 
> Some of the -Wextra warnings do need to be disabled, just like in
> Makefile.extrawarn, but take care to not disable them for W=2 or W=3
> builds, depending on the warning.
> 
> There are too many -Wformat-truncation warnings to cleanly fix up front;
> leave that warning disabled for now.
> 

With this patch in the mainline kernel, I get the following build error
when trying to build parisc:allmodconfig.

Error log:
drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_mthd':
drivers/gpu/drm/nouveau/nvif/object.c:161:9: error: 'memcpy' accessing 4294967264 or more bytes at offsets 0 and 32 overlaps 6442450881 bytes at offset -2147483617 [-Werror=restrict]
  161 |         memcpy(data, args->mthd.data, size);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/nouveau/nvif/object.c: In function 'nvif_object_ctor':
drivers/gpu/drm/nouveau/nvif/object.c:298:17: error: 'memcpy' accessing 4294967240 or more bytes at offsets 0 and 56 overlaps 6442450833 bytes at offset -2147483593 [-Werror=restrict]
  298 |                 memcpy(data, args->new.data, size);

The problem is also seen with v6.9 when trying to build an image
with W=1, so it is not triggered by a code change. I don't know
if other architectures are affected. The problem is not seen with
gcc 11.4, but it is seen with gcc 12.3 and 13.2. I did not try
with older versions of gcc.

Bisect log is attached for reference.

The odd error makes me wonder if I should revert to testing with gcc 11.4
and no longer bother with later versions of gcc, at least for any affected
architectures. Any recommendations ?

Thanks,
Guenter

---
# bad: [7ee332c9f12bc5b380e36919cd7d056592a7073f] Merge tag 'parisc-for-6.10-1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux
# good: [a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6] Linux 6.9
git bisect start 'HEAD' 'v6.9'
# good: [1b294a1f35616977caddaddf3e9d28e576a1adbc] Merge tag 'net-next-6.10' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
git bisect good 1b294a1f35616977caddaddf3e9d28e576a1adbc
# bad: [d34672777da3ea919e8adb0670ab91ddadf7dea0] Merge tag 'fbdev-for-6.10-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev
git bisect bad d34672777da3ea919e8adb0670ab91ddadf7dea0
# bad: [2871ec40994912ce4f2e2d5072a428eb84c77d3c] Merge tag 'drm-misc-next-2024-04-19' of https://gitlab.freedesktop.org/drm/misc/kernel into drm-next
git bisect bad 2871ec40994912ce4f2e2d5072a428eb84c77d3c
# bad: [34633158b8eb8fca145c9a73f8fe4f98c7275b06] Merge tag 'amd-drm-next-6.10-2024-04-13' of https://gitlab.freedesktop.org/agd5f/linux into drm-next
git bisect bad 34633158b8eb8fca145c9a73f8fe4f98c7275b06
# good: [4b0cb230bdb71c23981acfa5e7b367c7dde02a41] drm/amdgpu: retire UMC v12 mca_addr_to_pa
git bisect good 4b0cb230bdb71c23981acfa5e7b367c7dde02a41
# bad: [6376eb8b911534735fec104c1a0d780e4cf3116a] drm/dp: Clarify that wait_hpd_asserted() is not optional for panels
git bisect bad 6376eb8b911534735fec104c1a0d780e4cf3116a
# bad: [9c86b03863844ce69f99aa66404c79492ec9e208] drm/panthor: Fix panthor_devfreq kerneldoc
git bisect bad 9c86b03863844ce69f99aa66404c79492ec9e208
# bad: [b5d7cb76f2674c9d01b611141702723a95d12553] drm: add missing header guards to drm_internal.h
git bisect bad b5d7cb76f2674c9d01b611141702723a95d12553
# good: [4bdca11507928a4c9174e9b7240e9d058c12a71d] drm/panthor: Add the driver frontend block
git bisect good 4bdca11507928a4c9174e9b7240e9d058c12a71d
# good: [b2ec429b69280001d85029dc50b5427af41eb641] drm/tidss: Use dev_err_probe() over dev_dbg() when failing to probe the port
git bisect good b2ec429b69280001d85029dc50b5427af41eb641
# bad: [a61ddb4393ad1be61d2ffd92576d42707b05be17] drm: enable (most) W=1 warnings by default across the subsystem
git bisect bad a61ddb4393ad1be61d2ffd92576d42707b05be17
# good: [113cc3ad8566e06d6c8ef4fc0075a938dedefab5] drm/bridge: Document bridge init order with pre_enable_prev_first
git bisect good 113cc3ad8566e06d6c8ef4fc0075a938dedefab5
# good: [460be1d527a8e296d85301e8b14923299508d4fc] drm/nouveau: move more missing UAPI bits
git bisect good 460be1d527a8e296d85301e8b14923299508d4fc
# first bad commit: [a61ddb4393ad1be61d2ffd92576d42707b05be17] drm: enable (most) W=1 warnings by default across the subsystem

[RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR

[Jani Nikula]

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

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

[Javier Martinez Canillas]

Jani Nikula <jani.nikula@intel.com> writes:

Hello Jani,

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

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Nathan Chancellor]

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
> 

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

Hi,

On Tue, Mar 26, 2024 at 03:56:50PM -0700, Nathan Chancellor wrote:
> 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...

Would making DRM_WERROR depends on WERROR address your concerns?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Jani Nikula]

On Wed, 27 Mar 2024, Maxime Ripard <mripard@kernel.org> wrote:
> Hi,
>
> On Tue, Mar 26, 2024 at 03:56:50PM -0700, Nathan Chancellor wrote:
>> 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...
>
> Would making DRM_WERROR depends on WERROR address your concerns?

But then what would be the point of having DRM_WERROR at all? For me the
point is, "werror in drm, ignore the rest, they're someone else's
problem".

An alternative would be to "depends on !COMPILE_TEST" that we have in
i915, but then some folks want to have COMPILE_TEST in drm, because some
drivers are otherwise hard for people to build.

Nathan, we do want to fix any issues switfly. Are you hitting specific
build problems?


BR,
Jani.



>
> Maxime

-- 
Jani Nikula, Intel

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

[Nathan Chancellor]

On Wed, Mar 27, 2024 at 09:59:01AM +0200, Jani Nikula wrote:
> On Wed, 27 Mar 2024, Maxime Ripard <mripard@kernel.org> wrote:
> > On Tue, Mar 26, 2024 at 03:56:50PM -0700, Nathan Chancellor wrote:
> >> 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...
> >
> > Would making DRM_WERROR depends on WERROR address your concerns?
> 
> But then what would be the point of having DRM_WERROR at all? For me the
> point is, "werror in drm, ignore the rest, they're someone else's
> problem".

Right, I do think this is a valid view point and one I am sympathetic
to, especially since it is in the pursuit of increased code quality. I
do not want to disrupt that.

> An alternative would be to "depends on !COMPILE_TEST" that we have in
> i915, but then some folks want to have COMPILE_TEST in drm, because some
> drivers are otherwise hard for people to build.

Right. I think it is unfortunate how (at least in my opinion)
CONFIG_COMPILE_TEST has two meanings: genuinely just compile testing or
"allmodconfig". For the first case, we would want CONFIG_DRM_WERROR=y
but for the second case, it would be nice for CONFIG_DRM_WERROR to
default to off (because CONFIG_WERROR is enabled) but allow developers
to turn it on explicitly.

Another lofty/wistful idea to solve this would be to implement something
similar to compiler diagnostic groups for Kconfig, where there would be
a hierarchy like

  CONFIG_WERROR
    CONFIG_DRM_WERROR
    CONFIG_SUBSYSTEM_A_WERROR
    CONFIG_SUBSYSTEM_B_WERROR

where the value of CONFIG_WERROR is the same value for all
subconfigurations under it but they could still be enabled individually
without any additional dependencies (ala something like '-Wno-unused
-Wunused-variable'), which would allow my use case of CONFIG_WERROR=n
removing all instances of -Werror to continue to work but allow other
developers and CI systems to just set their specific -Werror
configuration and be done with it. I don't think something like that
exists but maybe I don't know Kconfig as well as I think I do :)

> Nathan, we do want to fix any issues switfly. Are you hitting specific
> build problems?

Yes, I see three distinct set of problems from our CI as a direct result
of this series. I already covered two in the prior mail but I'll be a
little more expansive below.

1. Instances of -Wunused-but-set-variable from variables that only have
   unary operations applied to them. Clang can warn in this case where
   GCC cannot: https://godbolt.org/z/d368q3coP

      int main(void)
      {
          int a = 0;
          a++;
          return 0;
      }

   which shows up in a few drm drivers. Most have a patch on the mailing
   list that has not been applied.

     drivers/gpu/drm/qxl/qxl_cmd.c:424:6: error: variable 'count' set but not used [-Werror,-Wunused-but-set-variable]
       424 |         int count = 0;
           |             ^
     https://lore.kernel.org/all/20230408165023.2706235-1-trix@redhat.com/ (almost a year old)

     drivers/gpu/drm/qxl/qxl_ioctl.c:148:14: error: variable 'num_relocs' set but not used [-Werror,-Wunused-but-set-variable]
       148 |         int i, ret, num_relocs;
           |                     ^
     https://lore.kernel.org/all/20240307104119.1980621-1-colin.i.king@gmail.com/

     drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c:843:6: error: variable 'out' set but not used [-Werror,-Wunused-but-set-variable]
       843 |         u64 out = dumper->iova + A6XX_CD_DATA_OFFSET;
           |             ^
     https://lore.kernel.org/all/20240326212324.185832-1-ojeda@kernel.org/ (recent patch)

     drivers/gpu/drm/panthor/panthor_sched.c:2048:6: error: variable 'csg_mod_mask' set but not used [-Werror,-Wunused-but-set-variable]
      2048 |         u32 csg_mod_mask = 0, free_csg_slots = 0;
           |             ^
     No patch, new driver, not reported yet it seems.

2. High stack usage in AMDGPU files for ARCH=powerpc allmodconfig. This
   might be a compiler issue but until now, there have been more
   important fires.

     drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c:1096:12: error: stack frame size (2064) exceeds limit (2048) in 'vcn_v3_0_start' [-Werror,-Wframe-larger-than]
      1096 | static int vcn_v3_0_start(struct amdgpu_device *adev)
           |            ^

     drivers/gpu/drm/amd/amdgpu/vcn_v4_0_5.c:955:12: error: stack frame size (2112) exceeds limit (2048) in 'vcn_v4_0_5_start' [-Werror,-Wframe-larger-than]
       955 | static int vcn_v4_0_5_start(struct amdgpu_device *adev)
           |            ^

     drivers/gpu/drm/amd/amdgpu/vcn_v5_0_0.c:713:12: error: stack frame size (2992) exceeds limit (2048) in 'vcn_v5_0_0_start' [-Werror,-Wframe-larger-than]
       713 | static int vcn_v5_0_0_start(struct amdgpu_device *adev)
           |            ^

   Taking a brief look at it while writing this email, it appears
   related to CONFIG_UBSAN_BOUNDS, as none of the warnings appear when
   that is disabled on top of allmodconfig. I suspect that the sanitizer
   instrumentation and inlining might be messing something up here, it
   has happened with other sanitizers like KASAN and KCSAN in the past.
   Without CONFIG_UBSAN_BOUNDS, the stack usage of these functions does
   not seem too bad:

     drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c:1096:12: warning: stack frame size (816) exceeds limit (512) in 'vcn_v3_0_start' [-Wframe-larger-than]
     drivers/gpu/drm/amd/amdgpu/vcn_v4_0_5.c:955:12: warning: stack frame size (800) exceeds limit (512) in 'vcn_v4_0_5_start' [-Wframe-larger-than]
     drivers/gpu/drm/amd/amdgpu/vcn_v5_0_0.c:713:12: warning: stack frame size (1040) exceeds limit (512) in 'vcn_v5_0_0_start' [-Wframe-larger-than]

3. -Wnull-pointer-arithmetic from IO port accessors on architectures
   that do not have them (such as hexagon and s390). For example:

     In file included from drivers/gpu/drm/virtio/virtgpu_plane.c:26:
     In file included from include/drm/drm_atomic_helper.h:31:
     In file included from include/drm/drm_crtc.h:32:
     In file included from include/drm/drm_modes.h:33:
     In file included from include/drm/drm_connector.h:32:
     In file included from include/drm/drm_util.h:35:
     In file included from include/linux/interrupt.h:11:
     In file included from include/linux/hardirq.h:11:
     In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
     In file included from include/asm-generic/hardirq.h:17:
     In file included from include/linux/irq.h:20:
     In file included from include/linux/io.h:13:
     In file included from arch/hexagon/include/asm/io.h:328:
     include/asm-generic/io.h:584:33: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
       584 |         __raw_writeb(value, PCI_IOBASE + addr);
           |                             ~~~~~~~~~~ ^
     include/asm-generic/io.h:594:59: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
       594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
           |                                                       ~~~~~~~~~~ ^
     include/asm-generic/io.h:604:59: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
       604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
           |                                                       ~~~~~~~~~~ ^

   These warnings are numerous throughout drivers/gpu/drm/ because this
   warning is enabled with -Wextra. Again, this is not exactly your
   problem and it should eventually be fixed by [1] (it appears that
   Niklas is working on a new version at [2]) but it is exacerbated by
   the default combo of W=1 + -Werror for DRM with allmodconfig now,
   even with CONFIG_WERROR=n.

Hopefully that helps clear things up. I am more than happy to send
patches or work towards solutions that satisfies everyone (or at least a
majority/consensus). Wider testing with clang never hurts as well but I
understand increasing build matrices is not always an easy sell.

[1]: https://lore.kernel.org/all/20230522105049.1467313-45-schnelle@linux.ibm.com/
[2]: https://git.kernel.org/niks/l/has_ioport_v6

Cheers,
Nathan

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

[Jani Nikula]

On Wed, 27 Mar 2024, Nathan Chancellor <nathan@kernel.org> wrote:
> On Wed, Mar 27, 2024 at 09:59:01AM +0200, Jani Nikula wrote:
>> An alternative would be to "depends on !COMPILE_TEST" that we have in
>> i915, but then some folks want to have COMPILE_TEST in drm, because some
>> drivers are otherwise hard for people to build.
>
> Right. I think it is unfortunate how (at least in my opinion)
> CONFIG_COMPILE_TEST has two meanings: genuinely just compile testing or
> "allmodconfig". For the first case, we would want CONFIG_DRM_WERROR=y
> but for the second case, it would be nice for CONFIG_DRM_WERROR to
> default to off (because CONFIG_WERROR is enabled) but allow developers
> to turn it on explicitly.

Yes, CONFIG_COMPILE_TEST has become overloaded.

> Another lofty/wistful idea to solve this would be to implement something
> similar to compiler diagnostic groups for Kconfig, where there would be
> a hierarchy like
>
>   CONFIG_WERROR
>     CONFIG_DRM_WERROR
>     CONFIG_SUBSYSTEM_A_WERROR
>     CONFIG_SUBSYSTEM_B_WERROR
>
> where the value of CONFIG_WERROR is the same value for all
> subconfigurations under it but they could still be enabled individually
> without any additional dependencies (ala something like '-Wno-unused
> -Wunused-variable'), which would allow my use case of CONFIG_WERROR=n
> removing all instances of -Werror to continue to work but allow other
> developers and CI systems to just set their specific -Werror
> configuration and be done with it. I don't think something like that
> exists but maybe I don't know Kconfig as well as I think I do :)

Yet another idea is to have a way to mark a config option "manual", that
is, never enable this automatically under any circumstances, not in make
allyesconfig or allmodconfig, don't ask in make oldconfig, don't allow
selects. The only way to enable is to toggle it manually. If you want it
and enable it and see problems, it's on you.

CONFIG_WERROR and CONFIG_DRM_WERROR could both be like this. The problem
with them is that they're not so much different configurations, they are
about how to deal with build errors, and that's not really what, say,
make allyesconfig should be about.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [RESEND v3 0/2] drm: enable W=1 warnings by default across the subsystem

[Maxime Ripard]

On Tue, 5 Mar 2024 11:07:34 +0200, Jani Nikula wrote:
> Resend of [1] with an intent to merge after the CI results come in. This
> is aiming for v6.10, so we'll have maximal time to find all the issues
> my configs didn't catch.
> 
> I built this on x86-64 (both gcc and clang), arm and arm64, and
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [RESEND v3 0/2] drm: enable W=1 warnings by default across the subsystem

[Jani Nikula]

On Tue, 05 Mar 2024, "Maxime Ripard" <mripard@kernel.org> wrote:
> On Tue, 5 Mar 2024 11:07:34 +0200, Jani Nikula wrote:
>> Resend of [1] with an intent to merge after the CI results come in. This
>> is aiming for v6.10, so we'll have maximal time to find all the issues
>> my configs didn't catch.
>> 
>> I built this on x86-64 (both gcc and clang), arm and arm64, and
>> 
>> [ ... ]
>
> Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks everyone for acks and reviews, pushed to drm-misc-next.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [RESEND v3 0/2] drm: enable W=1 warnings by default across the subsystem

[Lucas De Marchi]

On Tue, Mar 05, 2024 at 07:43:07PM +0200, Jani Nikula wrote:
>On Tue, 05 Mar 2024, "Maxime Ripard" <mripard@kernel.org> wrote:
>> On Tue, 5 Mar 2024 11:07:34 +0200, Jani Nikula wrote:
>>> Resend of [1] with an intent to merge after the CI results come in. This
>>> is aiming for v6.10, so we'll have maximal time to find all the issues
>>> my configs didn't catch.
>>>
>>> I built this on x86-64 (both gcc and clang), arm and arm64, and
>>>
>>> [ ... ]
>>
>> Acked-by: Maxime Ripard <mripard@kernel.org>
>
>Thanks everyone for acks and reviews, pushed to drm-misc-next.

should we start removing the now duplicate ones in i915 and xe?

Lucas De Marchi

>
>BR,
>Jani.
>
>
>-- 
>Jani Nikula, Intel

Re: [RESEND v3 0/2] drm: enable W=1 warnings by default across the subsystem

[Jani Nikula]

On Tue, 05 Mar 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Tue, Mar 05, 2024 at 07:43:07PM +0200, Jani Nikula wrote:
>>Thanks everyone for acks and reviews, pushed to drm-misc-next.
>
> should we start removing the now duplicate ones in i915 and xe?

After the drm-misc-next to drm-next merge and subsequent backmerge to
drm-intel-next and drm-xe-next i.e. sometime after the merge window.

BR,
Jani.



-- 
Jani Nikula, Intel