| Message ID | 48f11648d7169687e7242e4c9b4694a0c03c4263.1696595500.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: align with W=1 warnings | expand |
On Fri, Oct 06, 2023 at 03:34:46PM +0300, Jani Nikula wrote: > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, > have included -Wall, and the disables -Wno-format-security and > $(call cc-disable-warning,frame-address,) for a very long time. They're > redundant in our local subdir-ccflags-y and can be dropped. > > 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> Yeah, this seems totally reasonable. I always assumed the intention of -Wall was to re-enable some warnings that the rest of the kernel had turned off but I think we are getting better about auditing what warnings are explicitly turned off and getting some of those turned back on for the whole kernel, so I expect this to basically be a no-op. Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > drivers/gpu/drm/i915/Makefile | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index dec78efa452a..623f81217442 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -5,22 +5,20 @@ > > # 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 -Wall -Wextra (or W=1) we > +# 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 -Wall -Wextra is that when CI updates gcc we > +# 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 := -Wall -Wextra > -subdir-ccflags-y += -Wno-format-security > +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 > subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > -subdir-ccflags-y += $(call cc-disable-warning, frame-address) > subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > # Fine grained warnings disable > -- > 2.39.2 >
On Fri, Oct 6, 2023 at 5:35 AM Jani Nikula <jani.nikula@intel.com> wrote: > > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, > have included -Wall, and the disables -Wno-format-security and > $(call cc-disable-warning,frame-address,) for a very long time. They're > redundant in our local subdir-ccflags-y and can be dropped. > > 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> I didn't carefully cross reference these specific flags so I provide and ack rather than RB, but the logic in the description checks out IMO. Acked-by: Nick Desaulniers <ndesaulniers@google.com> > --- > drivers/gpu/drm/i915/Makefile | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index dec78efa452a..623f81217442 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -5,22 +5,20 @@ > > # 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 -Wall -Wextra (or W=1) we > +# 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 -Wall -Wextra is that when CI updates gcc we > +# 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 := -Wall -Wextra > -subdir-ccflags-y += -Wno-format-security > +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 > subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > -subdir-ccflags-y += $(call cc-disable-warning, frame-address) > subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > # Fine grained warnings disable > -- > 2.39.2 >
On Fri, Oct 6, 2023 at 9:35 PM Jani Nikula <jani.nikula@intel.com> wrote: > > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, > have included -Wall, and the disables -Wno-format-security and > $(call cc-disable-warning,frame-address,) for a very long time. They're > redundant in our local subdir-ccflags-y and can be dropped. > > 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> I made a similar suggestion in the past https://lore.kernel.org/dri-devel/20190515043753.9853-1-yamada.masahiro@socionext.com/ So, I am glad that Intel has decided to de-duplicate the flags. I think you can drop more flags. For example, subdir-ccflags-y += -Wno-sign-compare It is set by scripts/Makefile.extrawarn unless W=3 is passed. If W=3 is set by a user, -Wsign-compare should be warned as it is the user's request. drivers/gpu/drm/i915/Makefile negates W=3. There is no good reason to do so. Same applied to subdir-ccflags-y += -Wno-shift-negative-value > --- > drivers/gpu/drm/i915/Makefile | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index dec78efa452a..623f81217442 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -5,22 +5,20 @@ > > # 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 -Wall -Wextra (or W=1) we > +# 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 -Wall -Wextra is that when CI updates gcc we > +# 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 := -Wall -Wextra > -subdir-ccflags-y += -Wno-format-security > +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 > subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > -subdir-ccflags-y += $(call cc-disable-warning, frame-address) > subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > # Fine grained warnings disable > -- > 2.39.2 > -- Best Regards Masahiro Yamada
On Sun, Oct 08, 2023 at 12:28:46AM +0900, Masahiro Yamada wrote: > On Fri, Oct 6, 2023 at 9:35 PM Jani Nikula <jani.nikula@intel.com> wrote: > > > > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, > > have included -Wall, and the disables -Wno-format-security and > > $(call cc-disable-warning,frame-address,) for a very long time. They're > > redundant in our local subdir-ccflags-y and can be dropped. > > > > 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> > > > I made a similar suggestion in the past > https://lore.kernel.org/dri-devel/20190515043753.9853-1-yamada.masahiro@socionext.com/ > > So, I am glad that Intel has decided to de-duplicate the flags. > > > > I think you can drop more flags. > > For example, > > subdir-ccflags-y += -Wno-sign-compare > > > It is set by scripts/Makefile.extrawarn > unless W=3 is passed. > > > If W=3 is set by a user, -Wsign-compare should be warned > as it is the user's request. > > > drivers/gpu/drm/i915/Makefile negates W=3. > There is no good reason to do so. > > > Same applied to > > > subdir-ccflags-y += -Wno-shift-negative-value As I point out in my review of the second patch [1], I am not sure these should be dropped because -Wextra turns these warnings back on, at least for clang according to this build report [2] and my own testing, so they need to be disabled again. [1]: https://lore.kernel.org/20231006174550.GC3359308@dev-arch.thelio-3990X/ [2]: https://lore.kernel.org/202310070011.Fji48IBk-lkp@intel.com/ Cheers, Nathan.
On Mon, 09 Oct 2023, Nathan Chancellor <nathan@kernel.org> wrote: > On Sun, Oct 08, 2023 at 12:28:46AM +0900, Masahiro Yamada wrote: >> On Fri, Oct 6, 2023 at 9:35 PM Jani Nikula <jani.nikula@intel.com> wrote: >> > >> > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, >> > have included -Wall, and the disables -Wno-format-security and >> > $(call cc-disable-warning,frame-address,) for a very long time. They're >> > redundant in our local subdir-ccflags-y and can be dropped. >> > >> > 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> >> >> >> I made a similar suggestion in the past >> https://lore.kernel.org/dri-devel/20190515043753.9853-1-yamada.masahiro@socionext.com/ >> >> So, I am glad that Intel has decided to de-duplicate the flags. >> >> >> >> I think you can drop more flags. >> >> For example, >> >> subdir-ccflags-y += -Wno-sign-compare >> >> >> It is set by scripts/Makefile.extrawarn >> unless W=3 is passed. >> >> >> If W=3 is set by a user, -Wsign-compare should be warned >> as it is the user's request. >> >> >> drivers/gpu/drm/i915/Makefile negates W=3. >> There is no good reason to do so. >> >> >> Same applied to >> >> >> subdir-ccflags-y += -Wno-shift-negative-value > > As I point out in my review of the second patch [1], I am not sure these > should be dropped because -Wextra turns these warnings back on, at least > for clang according to this build report [2] and my own testing, so they > need to be disabled again. Yeah. The focus is on enabling W=1 warnings by default for i915. I get that the disables we have to add to achieve that also disable some W=2 and W=3 warnings. But taking all of that into account requires duplicating even more of Makefile.extrawarn (checking for warning levels, maintaining parity with the different levels, etc.). I guess we could check if KBUILD_EXTRA_WARN does not have any of 1, 2, or 3, but very few places outside of the build system look at KBUILD_EXTRA_WARN, so feels wrong. BR, Jani. > > [1]: https://lore.kernel.org/20231006174550.GC3359308@dev-arch.thelio-3990X/ > [2]: https://lore.kernel.org/202310070011.Fji48IBk-lkp@intel.com/ > > Cheers, > Nathan.
On Tue, 10 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote: > On Mon, 09 Oct 2023, Nathan Chancellor <nathan@kernel.org> wrote: >> On Sun, Oct 08, 2023 at 12:28:46AM +0900, Masahiro Yamada wrote: >>> On Fri, Oct 6, 2023 at 9:35 PM Jani Nikula <jani.nikula@intel.com> wrote: >>> > >>> > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, >>> > have included -Wall, and the disables -Wno-format-security and >>> > $(call cc-disable-warning,frame-address,) for a very long time. They're >>> > redundant in our local subdir-ccflags-y and can be dropped. >>> > >>> > 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> >>> >>> >>> I made a similar suggestion in the past >>> https://lore.kernel.org/dri-devel/20190515043753.9853-1-yamada.masahiro@socionext.com/ >>> >>> So, I am glad that Intel has decided to de-duplicate the flags. >>> >>> >>> >>> I think you can drop more flags. >>> >>> For example, >>> >>> subdir-ccflags-y += -Wno-sign-compare >>> >>> >>> It is set by scripts/Makefile.extrawarn >>> unless W=3 is passed. >>> >>> >>> If W=3 is set by a user, -Wsign-compare should be warned >>> as it is the user's request. >>> >>> >>> drivers/gpu/drm/i915/Makefile negates W=3. >>> There is no good reason to do so. >>> >>> >>> Same applied to >>> >>> >>> subdir-ccflags-y += -Wno-shift-negative-value >> >> As I point out in my review of the second patch [1], I am not sure these >> should be dropped because -Wextra turns these warnings back on, at least >> for clang according to this build report [2] and my own testing, so they >> need to be disabled again. > > Yeah. The focus is on enabling W=1 warnings by default for i915. I get > that the disables we have to add to achieve that also disable some W=2 > and W=3 warnings. But taking all of that into account requires > duplicating even more of Makefile.extrawarn (checking for warning > levels, maintaining parity with the different levels, etc.). > > I guess we could check if KBUILD_EXTRA_WARN does not have any of 1, 2, > or 3, but very few places outside of the build system look at > KBUILD_EXTRA_WARN, so feels wrong. This is the simplest I could think of: # The following turn off the warnings enabled by -Wextra ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),) KBUILD_CFLAGS += -Wno-missing-field-initializers KBUILD_CFLAGS += -Wno-type-limits KBUILD_CFLAGS += -Wno-shift-negative-value endif ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),) KBUILD_CFLAGS += -Wno-sign-compare endif Masahiro, I'd like to get your feedback on which to choose, unconditionally silencing the W=2/W=3 warnings for i915, or looking at KBUILD_EXTRA_WARN. Not silencing them is not an option, because we also use -Werror locally. BR, Jani. > > BR, > Jani. > > >> >> [1]: https://lore.kernel.org/20231006174550.GC3359308@dev-arch.thelio-3990X/ >> [2]: https://lore.kernel.org/202310070011.Fji48IBk-lkp@intel.com/ >> >> Cheers, >> Nathan.
On Tue, Oct 10, 2023 at 1:50 AM Jani Nikula <jani.nikula@intel.com> wrote: > > On Tue, 10 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote: > > On Mon, 09 Oct 2023, Nathan Chancellor <nathan@kernel.org> wrote: > >> On Sun, Oct 08, 2023 at 12:28:46AM +0900, Masahiro Yamada wrote: > >>> On Fri, Oct 6, 2023 at 9:35 PM Jani Nikula <jani.nikula@intel.com> wrote: > >>> > > >>> > The kernel top level Makefile, and recently scripts/Makefile.extrawarn, > >>> > have included -Wall, and the disables -Wno-format-security and > >>> > $(call cc-disable-warning,frame-address,) for a very long time. They're > >>> > redundant in our local subdir-ccflags-y and can be dropped. > >>> > > >>> > 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> > >>> > >>> > >>> I made a similar suggestion in the past > >>> https://lore.kernel.org/dri-devel/20190515043753.9853-1-yamada.masahiro@socionext.com/ > >>> > >>> So, I am glad that Intel has decided to de-duplicate the flags. > >>> > >>> > >>> > >>> I think you can drop more flags. > >>> > >>> For example, > >>> > >>> subdir-ccflags-y += -Wno-sign-compare > >>> > >>> > >>> It is set by scripts/Makefile.extrawarn > >>> unless W=3 is passed. > >>> > >>> > >>> If W=3 is set by a user, -Wsign-compare should be warned > >>> as it is the user's request. > >>> > >>> > >>> drivers/gpu/drm/i915/Makefile negates W=3. > >>> There is no good reason to do so. > >>> > >>> > >>> Same applied to > >>> > >>> > >>> subdir-ccflags-y += -Wno-shift-negative-value > >> > >> As I point out in my review of the second patch [1], I am not sure these > >> should be dropped because -Wextra turns these warnings back on, at least > >> for clang according to this build report [2] and my own testing, so they > >> need to be disabled again. > > > > Yeah. The focus is on enabling W=1 warnings by default for i915. I get > > that the disables we have to add to achieve that also disable some W=2 > > and W=3 warnings. But taking all of that into account requires > > duplicating even more of Makefile.extrawarn (checking for warning > > levels, maintaining parity with the different levels, etc.). > > > > I guess we could check if KBUILD_EXTRA_WARN does not have any of 1, 2, > > or 3, but very few places outside of the build system look at > > KBUILD_EXTRA_WARN, so feels wrong. > > This is the simplest I could think of: > > # The following turn off the warnings enabled by -Wextra > ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),) > KBUILD_CFLAGS += -Wno-missing-field-initializers > KBUILD_CFLAGS += -Wno-type-limits > KBUILD_CFLAGS += -Wno-shift-negative-value > endif > ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),) > KBUILD_CFLAGS += -Wno-sign-compare > endif > > Masahiro, I'd like to get your feedback on which to choose, > unconditionally silencing the W=2/W=3 warnings for i915, or looking at > KBUILD_EXTRA_WARN. KBUILD_EXTRA_WARN looks better to me; otherwise they would be hidden forever (or nearly). Suffer some duplication, w/e.
On Tue, 10 Oct 2023, Nick Desaulniers <ndesaulniers@google.com> wrote: > On Tue, Oct 10, 2023 at 1:50 AM Jani Nikula <jani.nikula@intel.com> wrote: >> This is the simplest I could think of: >> >> # The following turn off the warnings enabled by -Wextra >> ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),) >> KBUILD_CFLAGS += -Wno-missing-field-initializers >> KBUILD_CFLAGS += -Wno-type-limits >> KBUILD_CFLAGS += -Wno-shift-negative-value >> endif >> ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),) >> KBUILD_CFLAGS += -Wno-sign-compare >> endif >> >> Masahiro, I'd like to get your feedback on which to choose, >> unconditionally silencing the W=2/W=3 warnings for i915, or looking at >> KBUILD_EXTRA_WARN. > > KBUILD_EXTRA_WARN looks better to me; otherwise they would be hidden > forever (or nearly). Suffer some duplication, w/e. Thanks, sent v2 with this [1]. BR, Jani. [1] https://lore.kernel.org/r/cover.1697009258.git.jani.nikula@intel.com
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index dec78efa452a..623f81217442 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -5,22 +5,20 @@ # 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 -Wall -Wextra (or W=1) we +# 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 -Wall -Wextra is that when CI updates gcc we +# 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 := -Wall -Wextra -subdir-ccflags-y += -Wno-format-security +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 subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) -subdir-ccflags-y += $(call cc-disable-warning, frame-address) subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror # Fine grained warnings disable
The kernel top level Makefile, and recently scripts/Makefile.extrawarn, have included -Wall, and the disables -Wno-format-security and $(call cc-disable-warning,frame-address,) for a very long time. They're redundant in our local subdir-ccflags-y and can be dropped. 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> --- drivers/gpu/drm/i915/Makefile | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)