[PATCH 0/2] drm/i915: align with W=1 warnings

[PATCH 0/2] drm/i915: align with W=1 warnings

[Jani Nikula]

Update i915 extra warnings to align with W=1 warnings.

Cc'ing some folks who have contributed to i915 warnings in the past. I'm
only running gcc 12 locally, so I may not hit all issues that other gcc
or clang versions might hit.

BR,
Jani.


Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>

Jani Nikula (2):
  drm/i915: drop -Wall and related disables from cflags as redundant
  drm/i915: enable W=1 warnings by default

 drivers/gpu/drm/i915/Makefile | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

-- 
2.39.2

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

[Jani Nikula]

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(-)

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

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

[Nathan Chancellor]

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
> 

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

[Nick Desaulniers]

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
>


-- 
Thanks,
~Nick Desaulniers

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

[Masahiro Yamada]

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

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

[Nathan Chancellor]

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.

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

[Jani Nikula]

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.

-- 
Jani Nikula, Intel

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

[Jani Nikula]

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

Re: [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@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.

-- 
Thanks,
~Nick Desaulniers

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel

[PATCH 2/2] drm/i915: enable W=1 warnings by default

[Jani Nikula]

We enable a bunch more compiler warnings than the kernel
defaults. However, they've drifted to become a unique set of warnings,
and have increasingly fallen behind from the W=1 set.

Align with the W=1 warnings from scripts/Makefile.extrawarn for clarity,
by copy-pasting them with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it
easier to compare in the future.

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>

---

An alternative or future option would be to have Makefile.extrawarn
assign W=1 etc. flags to intermediate variables, say KBUILD_CFLAGS_W1,
like this:

KBUILD_CFLAGS_W1 += -Wextra -Wunused -Wno-unused-parameter
etc...

export KBUILD_CFLAGS_W1

ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)

KBUILD_CFLAGS += $(KBUILD_CFLAGS_W1)

else
etc...

and then drivers and subsystems could simply use:

subdir-ccflags-y += $(KBUILD_CFLAGS_W1)

to enable and remain up-to-date with W=1 warnings.
---
 drivers/gpu/drm/i915/Makefile | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 623f81217442..0485157054fc 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -3,22 +3,25 @@
 # Makefile for the drm device driver.  This driver provides support for the
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
-# 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 -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 -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 := -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
+# 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)
+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)
+subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
+subdir-ccflags-y += $(call cc-option, -Wstringop-overflow)
+subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
+# --- end copy-paste
+
+# Enable -Werror in CI and development
 subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
 
 # Fine grained warnings disable
-- 
2.39.2

Re: [Intel-gfx] [PATCH 2/2] drm/i915: enable W=1 warnings by default

[kernel test robot]

Hi Jani,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-i915-drop-Wall-and-related-disables-from-cflags-as-redundant/20231006-203658
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/f50c8ea0e63615aea28fe7f6049703e1d28ba7eb.1696595500.git.jani.nikula%40intel.com
patch subject: [Intel-gfx] [PATCH 2/2] drm/i915: enable W=1 warnings by default
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231007/202310070011.Fji48IBk-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231007/202310070011.Fji48IBk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310070011.Fji48IBk-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/i915_driver.c:30:
   In file included from include/linux/acpi.h:13:
   In file included from include/linux/resource_ext.h:11:
   In file included from include/linux/slab.h:16:
   In file included from include/linux/gfp.h:7:
   In file included from include/linux/mmzone.h:8:
   In file included from include/linux/spinlock.h:56:
   In file included from include/linux/preempt.h:79:
   In file included from arch/x86/include/asm/preempt.h:9:
>> include/linux/thread_info.h:240:29: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
           if (unlikely(sz >= 0 && sz < bytes)) {
                                   ~~ ^ ~~~~~
   include/linux/compiler.h:77:42: note: expanded from macro 'unlikely'
   # define unlikely(x)    __builtin_expect(!!(x), 0)
                                               ^
   In file included from drivers/gpu/drm/i915/i915_driver.c:30:
   In file included from include/linux/acpi.h:13:
   In file included from include/linux/resource_ext.h:11:
   In file included from include/linux/slab.h:16:
   In file included from include/linux/gfp.h:7:
   In file included from include/linux/mmzone.h:22:
>> include/linux/mm_types.h:1036:13: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           return cid == MM_CID_UNSET;
                  ~~~ ^  ~~~~~~~~~~~~
   include/linux/mm_types.h:1074:2: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           for_each_possible_cpu(i) {
           ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:974:36: note: expanded from macro 'for_each_possible_cpu'
   #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask)
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:282:2: note: expanded from macro 'for_each_cpu'
           for_each_set_bit(cpu, cpumask_bits(mask), small_cpumask_bits)
           ^                ~~~                      ~~~~~~~~~~~~~~~~~~
   include/linux/find.h:559:70: note: expanded from macro 'for_each_set_bit'
           for ((bit) = 0; (bit) = find_next_bit((addr), (size), (bit)), (bit) < (size); (bit)++)
                                                                          ~~~  ^  ~~~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:30:
   In file included from include/linux/acpi.h:13:
   In file included from include/linux/resource_ext.h:11:
   In file included from include/linux/slab.h:16:
   In file included from include/linux/gfp.h:7:
>> include/linux/mmzone.h:1627:44: warning: comparison of integers of different signs: 'int' and 'enum zone_type' [-Wsign-compare]
           if (likely(!nodes && zonelist_zone_idx(z) <= highest_zoneidx))
                                ~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
   # define likely(x)      __builtin_expect(!!(x), 1)
                                               ^
   In file included from drivers/gpu/drm/i915/i915_driver.c:30:
   In file included from include/linux/acpi.h:13:
   In file included from include/linux/resource_ext.h:11:
   In file included from include/linux/slab.h:20:
>> include/linux/percpu-refcount.h:205:3: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
                   this_cpu_add(*percpu_count, nr);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:491:33: note: expanded from macro 'this_cpu_add'
   #define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:368:11: note: expanded from macro '__pcpu_size_call'
                   case 8: stem##8(variable, __VA_ARGS__);break;           \
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:49:1: note: expanded from here
   this_cpu_add_8
   ^
   arch/x86/include/asm/percpu.h:370:35: note: expanded from macro 'this_cpu_add_8'
   #define this_cpu_add_8(pcp, val)                percpu_add_op(8, volatile, (pcp), val)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
                                 ((val) == 1 || (val) == -1)) ?            \
                                                 ~~~  ^  ~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:30:
   In file included from include/linux/acpi.h:13:
   In file included from include/linux/resource_ext.h:11:
   In file included from include/linux/slab.h:20:
   include/linux/percpu-refcount.h:244:3: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
                   this_cpu_add(*percpu_count, nr);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:491:33: note: expanded from macro 'this_cpu_add'
   #define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:368:11: note: expanded from macro '__pcpu_size_call'
                   case 8: stem##8(variable, __VA_ARGS__);break;           \
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:101:1: note: expanded from here
   this_cpu_add_8
   ^
   arch/x86/include/asm/percpu.h:370:35: note: expanded from macro 'this_cpu_add_8'
   #define this_cpu_add_8(pcp, val)                percpu_add_op(8, volatile, (pcp), val)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
                                 ((val) == 1 || (val) == -1)) ?            \
                                                 ~~~  ^  ~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:30:
   In file included from include/linux/acpi.h:13:
   In file included from include/linux/resource_ext.h:11:
   In file included from include/linux/slab.h:20:
   include/linux/percpu-refcount.h:333:3: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
                   this_cpu_sub(*percpu_count, nr);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:500:33: note: expanded from macro 'this_cpu_sub'
   #define this_cpu_sub(pcp, val)          this_cpu_add(pcp, -(typeof(pcp))(val))
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:491:33: note: expanded from macro 'this_cpu_add'
   #define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:368:11: note: expanded from macro '__pcpu_size_call'
                   case 8: stem##8(variable, __VA_ARGS__);break;           \
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:209:1: note: expanded from here
   this_cpu_add_8
   ^
   arch/x86/include/asm/percpu.h:370:35: note: expanded from macro 'this_cpu_add_8'
   #define this_cpu_add_8(pcp, val)                percpu_add_op(8, volatile, (pcp), val)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
                                 ((val) == 1 || (val) == -1)) ?            \
                                                 ~~~  ^  ~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:33:
   In file included from include/linux/oom.h:11:
   In file included from include/linux/mm.h:2168:
>> include/linux/vmstat.h:231:2: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           for_each_online_cpu(cpu)
           ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:975:36: note: expanded from macro 'for_each_online_cpu'
   #define for_each_online_cpu(cpu)   for_each_cpu((cpu), cpu_online_mask)
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:282:2: note: expanded from macro 'for_each_cpu'
           for_each_set_bit(cpu, cpumask_bits(mask), small_cpumask_bits)
           ^                ~~~                      ~~~~~~~~~~~~~~~~~~
   include/linux/find.h:559:70: note: expanded from macro 'for_each_set_bit'
           for ((bit) = 0; (bit) = find_next_bit((addr), (size), (bit)), (bit) < (size); (bit)++)
                                                                          ~~~  ^  ~~~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:39:
   In file included from include/linux/vga_switcheroo.h:34:
   In file included from include/linux/fb.h:7:
   In file included from include/uapi/linux/fb.h:6:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:22:
   In file included from include/linux/writeback.h:203:
>> include/linux/bio.h:401:14: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           if (sectors >= bio_sectors(bio))
               ~~~~~~~ ^  ~~~~~~~~~~~~~~~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:39:
   In file included from include/linux/vga_switcheroo.h:34:
   In file included from include/linux/fb.h:7:
   In file included from include/uapi/linux/fb.h:6:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:13:
>> include/linux/pagemap.h:813:36: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
           return index - folio_index(folio) < folio_nr_pages(folio);
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:52:
   In file included from drivers/gpu/drm/i915/display/intel_display_types.h:31:
>> include/linux/pwm.h:335:50: warning: comparison of integers of different signs: 'u64' (aka 'unsigned long long') and 'int' [-Wsign-compare]
           if (state.duty_cycle == duty_ns && state.period == period_ns)
                                              ~~~~~~~~~~~~ ^  ~~~~~~~~~
   include/linux/pwm.h:335:23: warning: comparison of integers of different signs: 'u64' (aka 'unsigned long long') and 'int' [-Wsign-compare]
           if (state.duty_cycle == duty_ns && state.period == period_ns)
               ~~~~~~~~~~~~~~~~ ^  ~~~~~~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:52:
   In file included from drivers/gpu/drm/i915/display/intel_display_types.h:49:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_object.h:15:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_object_types.h:13:
   In file included from include/drm/ttm/ttm_bo.h:39:
   In file included from include/drm/ttm/ttm_device.h:30:
>> include/drm/ttm/ttm_resource.h:312:16: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           for (i = 0; i < TTM_MAX_BO_PRIORITY; i++)
                       ~ ^ ~~~~~~~~~~~~~~~~~~~
   In file included from drivers/gpu/drm/i915/i915_driver.c:52:
   In file included from drivers/gpu/drm/i915/display/intel_display_types.h:49:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_object.h:16:
   In file included from drivers/gpu/drm/i915/i915_gem_gtt.h:14:
>> drivers/gpu/drm/i915/gt/intel_gtt.h:435:16: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
           if ((int)type >= ARRAY_SIZE(vm->min_alignment))
               ~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   14 warnings generated.
--
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:7:
   In file included from include/linux/slab.h:16:
   In file included from include/linux/gfp.h:7:
   In file included from include/linux/mmzone.h:8:
   In file included from include/linux/spinlock.h:56:
   In file included from include/linux/preempt.h:79:
   In file included from arch/x86/include/asm/preempt.h:9:
>> include/linux/thread_info.h:240:29: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
           if (unlikely(sz >= 0 && sz < bytes)) {
                                   ~~ ^ ~~~~~
   include/linux/compiler.h:77:42: note: expanded from macro 'unlikely'
   # define unlikely(x)    __builtin_expect(!!(x), 0)
                                               ^
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:7:
   In file included from include/linux/slab.h:16:
   In file included from include/linux/gfp.h:7:
   In file included from include/linux/mmzone.h:22:
>> include/linux/mm_types.h:1036:13: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           return cid == MM_CID_UNSET;
                  ~~~ ^  ~~~~~~~~~~~~
   include/linux/mm_types.h:1074:2: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           for_each_possible_cpu(i) {
           ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:974:36: note: expanded from macro 'for_each_possible_cpu'
   #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask)
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:282:2: note: expanded from macro 'for_each_cpu'
           for_each_set_bit(cpu, cpumask_bits(mask), small_cpumask_bits)
           ^                ~~~                      ~~~~~~~~~~~~~~~~~~
   include/linux/find.h:559:70: note: expanded from macro 'for_each_set_bit'
           for ((bit) = 0; (bit) = find_next_bit((addr), (size), (bit)), (bit) < (size); (bit)++)
                                                                          ~~~  ^  ~~~~
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:7:
   In file included from include/linux/slab.h:16:
   In file included from include/linux/gfp.h:7:
>> include/linux/mmzone.h:1627:44: warning: comparison of integers of different signs: 'int' and 'enum zone_type' [-Wsign-compare]
           if (likely(!nodes && zonelist_zone_idx(z) <= highest_zoneidx))
                                ~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
   # define likely(x)      __builtin_expect(!!(x), 1)
                                               ^
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:7:
   In file included from include/linux/slab.h:20:
>> include/linux/percpu-refcount.h:205:3: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
                   this_cpu_add(*percpu_count, nr);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:491:33: note: expanded from macro 'this_cpu_add'
   #define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:368:11: note: expanded from macro '__pcpu_size_call'
                   case 8: stem##8(variable, __VA_ARGS__);break;           \
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:28:1: note: expanded from here
   this_cpu_add_8
   ^
   arch/x86/include/asm/percpu.h:370:35: note: expanded from macro 'this_cpu_add_8'
   #define this_cpu_add_8(pcp, val)                percpu_add_op(8, volatile, (pcp), val)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
                                 ((val) == 1 || (val) == -1)) ?            \
                                                 ~~~  ^  ~~
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:7:
   In file included from include/linux/slab.h:20:
   include/linux/percpu-refcount.h:244:3: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
                   this_cpu_add(*percpu_count, nr);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:491:33: note: expanded from macro 'this_cpu_add'
   #define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:368:11: note: expanded from macro '__pcpu_size_call'
                   case 8: stem##8(variable, __VA_ARGS__);break;           \
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:80:1: note: expanded from here
   this_cpu_add_8
   ^
   arch/x86/include/asm/percpu.h:370:35: note: expanded from macro 'this_cpu_add_8'
   #define this_cpu_add_8(pcp, val)                percpu_add_op(8, volatile, (pcp), val)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
                                 ((val) == 1 || (val) == -1)) ?            \
                                                 ~~~  ^  ~~
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:7:
   In file included from include/linux/slab.h:20:
   include/linux/percpu-refcount.h:333:3: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
                   this_cpu_sub(*percpu_count, nr);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:500:33: note: expanded from macro 'this_cpu_sub'
   #define this_cpu_sub(pcp, val)          this_cpu_add(pcp, -(typeof(pcp))(val))
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:491:33: note: expanded from macro 'this_cpu_add'
   #define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:368:11: note: expanded from macro '__pcpu_size_call'
                   case 8: stem##8(variable, __VA_ARGS__);break;           \
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:188:1: note: expanded from here
   this_cpu_add_8
   ^
   arch/x86/include/asm/percpu.h:370:35: note: expanded from macro 'this_cpu_add_8'
   #define this_cpu_add_8(pcp, val)                percpu_add_op(8, volatile, (pcp), val)
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
                                 ((val) == 1 || (val) == -1)) ?            \
                                                 ~~~  ^  ~~
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:14:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_context.h:10:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_context_types.h:20:
   In file included from drivers/gpu/drm/i915/gt/intel_context_types.h:18:
   In file included from drivers/gpu/drm/i915/gt/intel_engine_types.h:21:
   In file included from drivers/gpu/drm/i915/i915_pmu.h:11:
   In file included from include/linux/perf_event.h:18:
   In file included from include/uapi/linux/bpf_perf_event.h:11:
   In file included from ./arch/x86/include/generated/uapi/asm/bpf_perf_event.h:1:
   In file included from include/uapi/asm-generic/bpf_perf_event.h:4:
   In file included from include/linux/ptrace.h:10:
   In file included from include/linux/pid_namespace.h:7:
   In file included from include/linux/mm.h:2168:
>> include/linux/vmstat.h:231:2: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           for_each_online_cpu(cpu)
           ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:975:36: note: expanded from macro 'for_each_online_cpu'
   #define for_each_online_cpu(cpu)   for_each_cpu((cpu), cpu_online_mask)
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:282:2: note: expanded from macro 'for_each_cpu'
           for_each_set_bit(cpu, cpumask_bits(mask), small_cpumask_bits)
           ^                ~~~                      ~~~~~~~~~~~~~~~~~~
   include/linux/find.h:559:70: note: expanded from macro 'for_each_set_bit'
           for ((bit) = 0; (bit) = find_next_bit((addr), (size), (bit)), (bit) < (size); (bit)++)
                                                                          ~~~  ^  ~~~~
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:14:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_context.h:12:
   In file included from drivers/gpu/drm/i915/gt/intel_context.h:14:
   In file included from drivers/gpu/drm/i915/i915_drv.h:37:
   In file included from include/drm/ttm/ttm_device.h:30:
>> include/drm/ttm/ttm_resource.h:312:16: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
           for (i = 0; i < TTM_MAX_BO_PRIORITY; i++)
                       ~ ^ ~~~~~~~~~~~~~~~~~~~
   In file included from drivers/gpu/drm/i915/i915_drm_client.c:14:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_context.h:12:
   In file included from drivers/gpu/drm/i915/gt/intel_context.h:14:
   In file included from drivers/gpu/drm/i915/i915_drv.h:46:
   In file included from drivers/gpu/drm/i915/gt/intel_engine.h:18:
   In file included from drivers/gpu/drm/i915/gt/intel_gt_types.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc.h:9:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h:9:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h:14:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_object.h:16:
   In file included from drivers/gpu/drm/i915/i915_gem_gtt.h:14:
>> drivers/gpu/drm/i915/gt/intel_gtt.h:435:16: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
           if ((int)type >= ARRAY_SIZE(vm->min_alignment))
               ~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   10 warnings generated.
..


vim +240 include/linux/thread_info.h

b0377fedb65280 Al Viro   2017-06-29  235  
9dd819a15162f8 Kees Cook 2019-09-25  236  static __always_inline __must_check bool
b0377fedb65280 Al Viro   2017-06-29  237  check_copy_size(const void *addr, size_t bytes, bool is_source)
b0377fedb65280 Al Viro   2017-06-29  238  {
c80d92fbb67b2c Kees Cook 2021-06-17  239  	int sz = __builtin_object_size(addr, 0);
b0377fedb65280 Al Viro   2017-06-29 @240  	if (unlikely(sz >= 0 && sz < bytes)) {
b0377fedb65280 Al Viro   2017-06-29  241  		if (!__builtin_constant_p(bytes))
b0377fedb65280 Al Viro   2017-06-29  242  			copy_overflow(sz, bytes);
b0377fedb65280 Al Viro   2017-06-29  243  		else if (is_source)
b0377fedb65280 Al Viro   2017-06-29  244  			__bad_copy_from();
b0377fedb65280 Al Viro   2017-06-29  245  		else
b0377fedb65280 Al Viro   2017-06-29  246  			__bad_copy_to();
b0377fedb65280 Al Viro   2017-06-29  247  		return false;
b0377fedb65280 Al Viro   2017-06-29  248  	}
6d13de1489b6bf Kees Cook 2019-12-04  249  	if (WARN_ON_ONCE(bytes > INT_MAX))
6d13de1489b6bf Kees Cook 2019-12-04  250  		return false;
b0377fedb65280 Al Viro   2017-06-29  251  	check_object_size(addr, bytes, is_source);
b0377fedb65280 Al Viro   2017-06-29  252  	return true;
b0377fedb65280 Al Viro   2017-06-29  253  }
b0377fedb65280 Al Viro   2017-06-29  254  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] drm/i915: enable W=1 warnings by default

[Nathan Chancellor]

On Fri, Oct 06, 2023 at 03:34:47PM +0300, Jani Nikula wrote:
> We enable a bunch more compiler warnings than the kernel
> defaults. However, they've drifted to become a unique set of warnings,
> and have increasingly fallen behind from the W=1 set.
> 
> Align with the W=1 warnings from scripts/Makefile.extrawarn for clarity,
> by copy-pasting them with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it
> easier to compare in the future.
> 
> 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>

One meta comment and review comment below. Feel free to carry forward

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

on future revisions.

> ---
> 
> An alternative or future option would be to have Makefile.extrawarn
> assign W=1 etc. flags to intermediate variables, say KBUILD_CFLAGS_W1,
> like this:
> 
> KBUILD_CFLAGS_W1 += -Wextra -Wunused -Wno-unused-parameter
> etc...
> 
> export KBUILD_CFLAGS_W1
> 
> ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
> 
> KBUILD_CFLAGS += $(KBUILD_CFLAGS_W1)
> 
> else
> etc...
> 
> and then drivers and subsystems could simply use:
> 
> subdir-ccflags-y += $(KBUILD_CFLAGS_W1)
> 
> to enable and remain up-to-date with W=1 warnings.

This has definitely come up a few times and while I am generally in
favor of something like this, it makes adding warnings to W=1 a little
bit harder because when we add warnings to W=1, we typically are not
concerned with breaking the build, as W=1 is not the default build. If a
subsystem has opted into "whatever the current W=1 is" by default,
changes to W=1 will have to be reviewed/tested within a normal build.

Doing something like this patch with a more regular cadence (maybe every
update after the merge window) seems like a reasonable compromise to me,
although I know that means more work for individual subsystem
maintainers.

> ---
>  drivers/gpu/drm/i915/Makefile | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 623f81217442..0485157054fc 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -3,22 +3,25 @@
>  # Makefile for the drm device driver.  This driver provides support for the
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
> -# 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 -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 -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 := -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

As the test robot points out, you'll want to keep these four, as they
are only enabled for W=2 or W=3. With this diff on top of these two
patches:

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0485157054fc..9c4e09c8aa4e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -21,6 +21,12 @@ subdir-ccflags-y += $(call cc-option, -Wstringop-overflow)
 subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
 # --- end copy-paste
 
+# The following turn off the warnings enabled by -Wextra
+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
+
 # Enable -Werror in CI and development
 subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
 
I can continue to build i915 warning free with ARCH=x86_64 allmodconfig
using all supported versions of LLVM for building the kernel.

> +# 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)
> +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)
> +subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
> +subdir-ccflags-y += $(call cc-option, -Wstringop-overflow)
> +subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
> +# --- end copy-paste
> +
> +# Enable -Werror in CI and development
>  subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
>  
>  # Fine grained warnings disable
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] drm/i915: enable W=1 warnings by default

[Jani Nikula]

On Fri, 06 Oct 2023, Nathan Chancellor <nathan@kernel.org> wrote:
> On Fri, Oct 06, 2023 at 03:34:47PM +0300, Jani Nikula wrote:
>> We enable a bunch more compiler warnings than the kernel
>> defaults. However, they've drifted to become a unique set of warnings,
>> and have increasingly fallen behind from the W=1 set.
>> 
>> Align with the W=1 warnings from scripts/Makefile.extrawarn for clarity,
>> by copy-pasting them with s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it
>> easier to compare in the future.
>> 
>> 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>
>
> One meta comment and review comment below. Feel free to carry forward
>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>
> on future revisions.

Thanks!

>
>> ---
>> 
>> An alternative or future option would be to have Makefile.extrawarn
>> assign W=1 etc. flags to intermediate variables, say KBUILD_CFLAGS_W1,
>> like this:
>> 
>> KBUILD_CFLAGS_W1 += -Wextra -Wunused -Wno-unused-parameter
>> etc...
>> 
>> export KBUILD_CFLAGS_W1
>> 
>> ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
>> 
>> KBUILD_CFLAGS += $(KBUILD_CFLAGS_W1)
>> 
>> else
>> etc...
>> 
>> and then drivers and subsystems could simply use:
>> 
>> subdir-ccflags-y += $(KBUILD_CFLAGS_W1)
>> 
>> to enable and remain up-to-date with W=1 warnings.
>
> This has definitely come up a few times and while I am generally in
> favor of something like this, it makes adding warnings to W=1 a little
> bit harder because when we add warnings to W=1, we typically are not
> concerned with breaking the build, as W=1 is not the default build. If a
> subsystem has opted into "whatever the current W=1 is" by default,
> changes to W=1 will have to be reviewed/tested within a normal build.
>
> Doing something like this patch with a more regular cadence (maybe every
> update after the merge window) seems like a reasonable compromise to me,
> although I know that means more work for individual subsystem
> maintainers.

Makes sense.

>
>> ---
>>  drivers/gpu/drm/i915/Makefile | 33 ++++++++++++++++++---------------
>>  1 file changed, 18 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 623f81217442..0485157054fc 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -3,22 +3,25 @@
>>  # Makefile for the drm device driver.  This driver provides support for the
>>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>>  
>> -# 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 -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 -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 := -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
>
> As the test robot points out, you'll want to keep these four, as they
> are only enabled for W=2 or W=3. With this diff on top of these two
> patches:

Right. I was confused by the refactoring done in Makefile.extrawarn, and
didn't notice that the disables were moved to else branches of the
checks for W=2 and W=3.

BR,
Jani.

>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 0485157054fc..9c4e09c8aa4e 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -21,6 +21,12 @@ subdir-ccflags-y += $(call cc-option, -Wstringop-overflow)
>  subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
>  # --- end copy-paste
>  
> +# The following turn off the warnings enabled by -Wextra
> +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
> +
>  # Enable -Werror in CI and development
>  subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
>  
> I can continue to build i915 warning free with ARCH=x86_64 allmodconfig
> using all supported versions of LLVM for building the kernel.
>
>> +# 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)
>> +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)
>> +subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
>> +subdir-ccflags-y += $(call cc-option, -Wstringop-overflow)
>> +subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
>> +# --- end copy-paste
>> +
>> +# Enable -Werror in CI and development
>>  subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
>>  
>>  # Fine grained warnings disable
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel