On Wed, 10 Jan 2024, Hamza Mahfooz <hamza.mahfooz@xxxxxxx> wrote: > On 1/10/24 12:39, 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. >> >> v2: >> - Drop -Wformat-truncation (too many warnings) >> - Drop -Wstringop-overflow (enabled by default upstream) >> >> Cc: David Airlie <airlied@xxxxxxxxx> >> Cc: Daniel Vetter <daniel@xxxxxxxx> >> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >> Cc: Maxime Ripard <mripard@xxxxxxxxxx> >> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> >> Cc: Alex Deucher <alexander.deucher@xxxxxxx> >> Cc: Christian König <christian.koenig@xxxxxxx> >> Cc: Pan, Xinhui <Xinhui.Pan@xxxxxxx> >> Cc: Karol Herbst <kherbst@xxxxxxxxxx> >> Cc: Lyude Paul <lyude@xxxxxxxxxx> >> Cc: Danilo Krummrich <dakr@xxxxxxxxxx> >> Cc: Rob Clark <robdclark@xxxxxxxxx> >> Cc: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> >> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> >> Cc: Sean Paul <sean@xxxxxxxxxx> >> Cc: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx> >> Cc: Hamza Mahfooz <hamza.mahfooz@xxxxxxx> >> Acked-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> >> Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx> >> Acked-by: Sui Jingfeng <sui.jingfeng@xxxxxxxxx> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> drivers/gpu/drm/Makefile | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 104b42df2e95..8b6be830f7c3 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -5,6 +5,33 @@ >> >> 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 += -Wmissing-declarations >> +subdir-ccflags-y += $(call cc-option, -Wrestrict) > > It would be safer to do something along the lines of: > > cond-flags := $(call cc-option, -Wrestrict) \ > $(call cc-option, -Wunused-but-set-variable) \ > $(call cc-option, -Wunused-const-variable) \ > $(call cc-option, -Wunused-const-variable) \ > $(call cc-option, -Wformat-overflow) \ > $(call cc-option, -Wstringop-truncation) > subdir-ccflags-y += $(cond-flags) > > Otherwise, you will end up breaking `$ make M=drivers/gpu/drm` > for a bunch of people. I discussed this with Alex on IRC yesterday. The above seems obviously correct in that it just changes the evaluation time of $(call cc-option, ...). Apparently not having it may lead to: scripts/Makefile.lib:10: *** Recursive variable 'KBUILD_CFLAGS' references itself (eventually). Stop. Of course, I could just throw that in and be happy, but me being me, I'd really like to know what is going on here first. :) For one thing, I always thought M=dir was only for out-of-tree modules, though the IRC discussion seems to indicate a lot of people also use it for in-tree modules. But I can't even make it to work for a lot of cases on top of current drm-tip, without the changes here. M=drivers/gpu/drm/i915 fails immediately. So does M=drivers/gpu/drm/amd. And M=drivers/gpu/drm/nouveau. And M=drivers/gpu/drm/radeon. M=drivers/gpu/drm fails with the same cases as above. I always use KBUILD_OUTPUT=dir but I don't know if it makes a difference, I didn't try. However M=drivers/gpu/drm/amd/amdgpu works. The only way I could reproduce the "recursive variable" issue in that was using: subdir-ccflags-y = -Wextra instead of: subdir-ccflags-y := -Wextra or: subdir-ccflags-y += -Wextra in amdgpu/Makefile Since I use the latter form in this pach, I think it should be fine for M=dir users even if M=dir doesn't really seem to generally work for in-tree modules (at least not for me). Cc: Masahiro BR, Jani. > >> +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) >> +# 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 \ -- Jani Nikula, Intel
