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

[Jani Nikula <jani.nikula@intel.com>]

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.

-- 
Jani Nikula, Intel