Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
On Wed, 27 Mar 2024, Nathan Chancellor 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 2/2] drm: Add CONFIG_DRM_WERROR
On Wed, Mar 27, 2024 at 09:59:01AM +0200, Jani Nikula wrote: > On Wed, 27 Mar 2024, Maxime Ripard 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 # v1 > >> > Signed-off-by: Jani Nikula > >> > --- > >> > 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_...@mail.gmail.com/ > >> https://lore.kernel.org/all/20230522105049.1467313-1-schne...@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;
Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
On Wed, 27 Mar 2024, Maxime Ripard 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 # v1 >> > Signed-off-by: Jani Nikula >> > --- >> > 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_...@mail.gmail.com/ >> https://lore.kernel.org/all/20230522105049.1467313-1-schne...@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
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 # v1 > > Signed-off-by: Jani Nikula > > --- > > 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_...@mail.gmail.com/ > https://lore.kernel.org/all/20230522105049.1467313-1-schne...@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 signature.asc Description: PGP signature
Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
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 # v1 > Signed-off-by: Jani Nikula > --- > 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_...@mail.gmail.com/ https://lore.kernel.org/all/20230522105049.1467313-1-schne...@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
Jani Nikula 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 # v1 > Signed-off-by: Jani Nikula > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
[RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
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 # v1 Signed-off-by: Jani Nikula --- 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