[Intel-gfx] [PATCH 1/2] drm/i915: drop -Wall and related disables from cflags as redundant

[Nick Desaulniers]

On Tue, Oct 10, 2023 at 1:50 AM Jani Nikula <jani.nikula at intel.com> wrote:
>
> On Tue, 10 Oct 2023, Jani Nikula <jani.nikula at intel.com> wrote:
> > On Mon, 09 Oct 2023, Nathan Chancellor <nathan at 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 at 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 at arndb.de>
> >>> > Cc: Nick Desaulniers <ndesaulniers at google.com>
> >>> > Cc: Nathan Chancellor <nathan at kernel.org>
> >>> > Cc: Masahiro Yamada <masahiroy at kernel.org>
> >>> > Signed-off-by: Jani Nikula <jani.nikula at 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.

-- 
Thanks,
~Nick Desaulniers