[PATCH] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[PATCH] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Nathan Huckleberry]

Clang is updating to support -Wimplicit-fallthrough on C
https://reviews.llvm.org/D64838. Since clang does not
support the comment version of fallthrough annotations
this update causes an additional 50k warnings. Most
of these warnings (>49k) are duplicates from header files.

This patch is intended to be reverted after the warnings
have been cleaned up.

Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
 Makefile                   | 4 ++++
 scripts/Makefile.extrawarn | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/Makefile b/Makefile
index 1b23f95db176..93b9744e66a2 100644
--- a/Makefile
+++ b/Makefile
@@ -846,7 +846,11 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 KBUILD_CFLAGS += -Wdeclaration-after-statement
 
 # Warn about unmarked fall-throughs in switch statement.
+# If the compiler is clang, this warning is only enabled if W=1 in
+# Makefile.extrawarn
+ifndef CONFIG_CC_IS_CLANG
 KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
+endif
 
 # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
 KBUILD_CFLAGS += -Wvla
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index a74ce2e3c33e..e12359d69bb7 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -30,6 +30,9 @@ warning-1 += $(call cc-option, -Wunused-but-set-variable)
 warning-1 += $(call cc-option, -Wunused-const-variable)
 warning-1 += $(call cc-option, -Wpacked-not-aligned)
 warning-1 += $(call cc-option, -Wstringop-truncation)
+ifdef CONFIG_CC_IS_CLANG
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
+endif
 # The following turn off the warnings enabled by -Wextra
 warning-1 += -Wno-missing-field-initializers
 warning-1 += -Wno-sign-compare
-- 
2.23.0.rc1.153.gdeed80330f-goog

Re: [PATCH] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Nathan Chancellor]

On Thu, Aug 15, 2019 at 11:20:29AM -0700, 'Nathan Huckleberry' via Clang Built Linux wrote:
> Clang is updating to support -Wimplicit-fallthrough on C
> https://reviews.llvm.org/D64838. Since clang does not
> support the comment version of fallthrough annotations
> this update causes an additional 50k warnings. Most
> of these warnings (>49k) are duplicates from header files.
> 
> This patch is intended to be reverted after the warnings
> have been cleaned up.
> 
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> ---
>  Makefile                   | 4 ++++
>  scripts/Makefile.extrawarn | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 1b23f95db176..93b9744e66a2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -846,7 +846,11 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>  KBUILD_CFLAGS += -Wdeclaration-after-statement
>  
>  # Warn about unmarked fall-throughs in switch statement.
> +# If the compiler is clang, this warning is only enabled if W=1 in
> +# Makefile.extrawarn
> +ifndef CONFIG_CC_IS_CLANG
>  KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> +endif
>  
>  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
>  KBUILD_CFLAGS += -Wvla
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index a74ce2e3c33e..e12359d69bb7 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -30,6 +30,9 @@ warning-1 += $(call cc-option, -Wunused-but-set-variable)
>  warning-1 += $(call cc-option, -Wunused-const-variable)
>  warning-1 += $(call cc-option, -Wpacked-not-aligned)
>  warning-1 += $(call cc-option, -Wstringop-truncation)
> +ifdef CONFIG_CC_IS_CLANG
> +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)

Shouldn't this be warning-1?

> +endif
>  # The following turn off the warnings enabled by -Wextra
>  warning-1 += -Wno-missing-field-initializers
>  warning-1 += -Wno-sign-compare
> -- 
> 2.23.0.rc1.153.gdeed80330f-goog
> 

I am still not a huge fan of the CONFIG_CC_IS_CLANG ifdefs but I don't
really see a much cleaner way to get around this. Some that come to
mind:

* Leave Makefile alone and add

KBUILD_CFLAGS += -Wno-implicit-fallthrough

in the CONFIG_CC_IS_CLANG section of scripts/Makefile.extrawarn

* Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
to just -Wimplicit-fallthrough for clang") for the time being and just
rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.

Regardless:

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

Re: [PATCH] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Nick Desaulniers]

On Thu, Aug 15, 2019 at 1:45 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Aug 15, 2019 at 11:20:29AM -0700, 'Nathan Huckleberry' via Clang Built Linux wrote:
> > Clang is updating to support -Wimplicit-fallthrough on C
> > https://reviews.llvm.org/D64838. Since clang does not
> > support the comment version of fallthrough annotations
> > this update causes an additional 50k warnings. Most
> > of these warnings (>49k) are duplicates from header files.
> >
> > This patch is intended to be reverted after the warnings
> > have been cleaned up.
> >
> > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > ---
> >  Makefile                   | 4 ++++
> >  scripts/Makefile.extrawarn | 3 +++
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/Makefile b/Makefile
> > index 1b23f95db176..93b9744e66a2 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -846,7 +846,11 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> >  KBUILD_CFLAGS += -Wdeclaration-after-statement
> >
> >  # Warn about unmarked fall-throughs in switch statement.
> > +# If the compiler is clang, this warning is only enabled if W=1 in
> > +# Makefile.extrawarn
> > +ifndef CONFIG_CC_IS_CLANG
> >  KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> > +endif
> >
> >  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> >  KBUILD_CFLAGS += -Wvla
> > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> > index a74ce2e3c33e..e12359d69bb7 100644
> > --- a/scripts/Makefile.extrawarn
> > +++ b/scripts/Makefile.extrawarn
> > @@ -30,6 +30,9 @@ warning-1 += $(call cc-option, -Wunused-but-set-variable)
> >  warning-1 += $(call cc-option, -Wunused-const-variable)
> >  warning-1 += $(call cc-option, -Wpacked-not-aligned)
> >  warning-1 += $(call cc-option, -Wstringop-truncation)
> > +ifdef CONFIG_CC_IS_CLANG
> > +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
>
> Shouldn't this be warning-1?

+1

>
> > +endif
> >  # The following turn off the warnings enabled by -Wextra
> >  warning-1 += -Wno-missing-field-initializers
> >  warning-1 += -Wno-sign-compare
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
>
> I am still not a huge fan of the CONFIG_CC_IS_CLANG ifdefs but I don't
> really see a much cleaner way to get around this. Some that come to
> mind:
>
> * Leave Makefile alone and add
>
> KBUILD_CFLAGS += -Wno-implicit-fallthrough
>
> in the CONFIG_CC_IS_CLANG section of scripts/Makefile.extrawarn

Yeah, I think this might be cleaner.  -Wimplicit-fallthrough
-Wno-implicit-fallthrough will be passed to Clang, but "last one
wins."  A smaller patch makes it more likely to be revertable without
potentially having to resolve any conflicts.  Would you mind sending a
V2 with that change? You can include Nathan's Suggested-by tag.
-- 
Thanks,
~Nick Desaulniers

[PATCH v2] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Nathan Huckleberry]

Clang is updating to support -Wimplicit-fallthrough on C
https://reviews.llvm.org/D64838. Since clang does not
support the comment version of fallthrough annotations
this update causes an additional 50k warnings. Most
of these warnings (>49k) are duplicates from header files.

This patch is intended to be reverted after the warnings
have been cleaned up.

Signed-off-by: Nathan Huckleberry <nhuck@google.com>
Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
---
Changes v1->v2
* Move code to preexisting ifdef
 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index a74ce2e3c33e..95973a1ee999 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
 KBUILD_CFLAGS += -Wno-format
 KBUILD_CFLAGS += -Wno-sign-compare
 KBUILD_CFLAGS += -Wno-format-zero-length
+KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
 endif
 endif
-- 
2.23.0.rc1.153.gdeed80330f-goog

Re: [PATCH] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Miguel Ojeda]

On Thu, Aug 15, 2019 at 10:45 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> I am still not a huge fan of the CONFIG_CC_IS_CLANG ifdefs but I don't
> really see a much cleaner way to get around this. Some that come to
> mind:

Yeah...

> * Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
> to just -Wimplicit-fallthrough for clang") for the time being and just
> rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.

I would avoid applying commits that will have to be reverted just for
Clang, particularly since it is not fully supported yet.

Cheers,
Miguel

Re: [PATCH v2] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Nick Desaulniers]

On Thu, Aug 15, 2019 at 3:59 PM 'Nathan Huckleberry' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> Clang is updating to support -Wimplicit-fallthrough on C
> https://reviews.llvm.org/D64838. Since clang does not
> support the comment version of fallthrough annotations
> this update causes an additional 50k warnings. Most
> of these warnings (>49k) are duplicates from header files.
>
> This patch is intended to be reverted after the warnings
> have been cleaned up.
>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> Changes v1->v2
> * Move code to preexisting ifdef
>  scripts/Makefile.extrawarn | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index a74ce2e3c33e..95973a1ee999 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
>  KBUILD_CFLAGS += -Wno-format
>  KBUILD_CFLAGS += -Wno-sign-compare
>  KBUILD_CFLAGS += -Wno-format-zero-length
> +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)

Clang seems to support -Wno-implicit-fallthrough since 3.2.  You can
remove the cc-option, since you'll need a newer clang than that to
build the kernel.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Nick Desaulniers]

On Thu, Aug 15, 2019 at 3:59 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Aug 15, 2019 at 10:45 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > * Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
> > to just -Wimplicit-fallthrough for clang") for the time being and just
> > rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.
>
> I would avoid applying commits that will have to be reverted just for
> Clang, particularly since it is not fully supported yet.

"not fully supported yet" you say? *drops monocle*
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS?h=v5.3-rc4#n4001
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Miguel Ojeda]

On Fri, Aug 16, 2019 at 1:05 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Aug 15, 2019 at 3:59 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Thu, Aug 15, 2019 at 10:45 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > > * Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
> > > to just -Wimplicit-fallthrough for clang") for the time being and just
> > > rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.
> >
> > I would avoid applying commits that will have to be reverted just for
> > Clang, particularly since it is not fully supported yet.
>
> "not fully supported yet" you say? *drops monocle*
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS?h=v5.3-rc4#n4001

By fully supported I mean it already works and people can rely on it
out of the box using a released version of Clang/LLVM. Is that the
case already? If so, many places need updating!

  * include/linux/compiler-clang.h should check for the minimum
supported version
  * Documentation/process/programming-language.rst should be updated
  * https://github.com/ClangBuiltLinux/linux/wiki does not mention anything

etc.

Cheers,
Miguel

Re: [PATCH v2] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Masahiro Yamada]

Hi.

On Fri, Aug 16, 2019 at 7:59 AM Nathan Huckleberry <nhuck@google.com> wrote:
>
> Clang is updating to support -Wimplicit-fallthrough on C
> https://reviews.llvm.org/D64838. Since clang does not
> support the comment version of fallthrough annotations
> this update causes an additional 50k warnings. Most
> of these warnings (>49k) are duplicates from header files.
>
> This patch is intended to be reverted after the warnings
> have been cleaned up.
>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> Changes v1->v2
> * Move code to preexisting ifdef
>  scripts/Makefile.extrawarn | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index a74ce2e3c33e..95973a1ee999 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
>  KBUILD_CFLAGS += -Wno-format
>  KBUILD_CFLAGS += -Wno-sign-compare
>  KBUILD_CFLAGS += -Wno-format-zero-length
> +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
>  endif
>  endif
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>


Perhaps, is the following even cleaner?



diff --git a/Makefile b/Makefile
index 1b23f95db176..cebc6bf5372e 100644
--- a/Makefile
+++ b/Makefile
@@ -751,6 +751,9 @@ else
 # These warnings generated too much noise in a regular build.
 # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
 KBUILD_CFLAGS += -Wno-unused-but-set-variable
+
+# Warn about unmarked fall-throughs in switch statement.
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
 endif

 KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
@@ -845,9 +848,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
-print-file-name=include)
 # warn about C99 declaration after statement
 KBUILD_CFLAGS += -Wdeclaration-after-statement

-# Warn about unmarked fall-throughs in switch statement.
-KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
-
 # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
 KBUILD_CFLAGS += -Wvla



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Nathan Chancellor]

On Mon, Aug 19, 2019 at 01:43:08AM +0900, Masahiro Yamada wrote:
> Hi.
> 
> On Fri, Aug 16, 2019 at 7:59 AM Nathan Huckleberry <nhuck@google.com> wrote:
> >
> > Clang is updating to support -Wimplicit-fallthrough on C
> > https://reviews.llvm.org/D64838. Since clang does not
> > support the comment version of fallthrough annotations
> > this update causes an additional 50k warnings. Most
> > of these warnings (>49k) are duplicates from header files.
> >
> > This patch is intended to be reverted after the warnings
> > have been cleaned up.
> >
> > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> > Changes v1->v2
> > * Move code to preexisting ifdef
> >  scripts/Makefile.extrawarn | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> > index a74ce2e3c33e..95973a1ee999 100644
> > --- a/scripts/Makefile.extrawarn
> > +++ b/scripts/Makefile.extrawarn
> > @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
> >  KBUILD_CFLAGS += -Wno-format
> >  KBUILD_CFLAGS += -Wno-sign-compare
> >  KBUILD_CFLAGS += -Wno-format-zero-length
> > +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
> >  endif
> >  endif
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
> 
> 
> Perhaps, is the following even cleaner?
> 
> 
> 
> diff --git a/Makefile b/Makefile
> index 1b23f95db176..cebc6bf5372e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -751,6 +751,9 @@ else
>  # These warnings generated too much noise in a regular build.
>  # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
>  KBUILD_CFLAGS += -Wno-unused-but-set-variable
> +
> +# Warn about unmarked fall-throughs in switch statement.
> +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
>  endif
> 
>  KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> @@ -845,9 +848,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> -print-file-name=include)
>  # warn about C99 declaration after statement
>  KBUILD_CFLAGS += -Wdeclaration-after-statement
> 
> -# Warn about unmarked fall-throughs in switch statement.
> -KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> -
>  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
>  KBUILD_CFLAGS += -Wvla
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

I like this more than anything suggested so far. I think a comment
should be added regarding why this is only enabled for GCC right now but
that is pretty easy to revert once we have figured out the right course
of action.

Cheers,
Nathan

Re: [PATCH v2] kbuild: Require W=1 for -Wimplicit-fallthrough with clang

[Masahiro Yamada]

On Mon, Aug 19, 2019 at 3:43 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Mon, Aug 19, 2019 at 01:43:08AM +0900, Masahiro Yamada wrote:
> > Hi.
> >
> > On Fri, Aug 16, 2019 at 7:59 AM Nathan Huckleberry <nhuck@google.com> wrote:
> > >
> > > Clang is updating to support -Wimplicit-fallthrough on C
> > > https://reviews.llvm.org/D64838. Since clang does not
> > > support the comment version of fallthrough annotations
> > > this update causes an additional 50k warnings. Most
> > > of these warnings (>49k) are duplicates from header files.
> > >
> > > This patch is intended to be reverted after the warnings
> > > have been cleaned up.
> > >
> > > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > > Changes v1->v2
> > > * Move code to preexisting ifdef
> > >  scripts/Makefile.extrawarn | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> > > index a74ce2e3c33e..95973a1ee999 100644
> > > --- a/scripts/Makefile.extrawarn
> > > +++ b/scripts/Makefile.extrawarn
> > > @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
> > >  KBUILD_CFLAGS += -Wno-format
> > >  KBUILD_CFLAGS += -Wno-sign-compare
> > >  KBUILD_CFLAGS += -Wno-format-zero-length
> > > +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
> > >  endif
> > >  endif
> > > --
> > > 2.23.0.rc1.153.gdeed80330f-goog
> > >
> >
> >
> > Perhaps, is the following even cleaner?
> >
> >
> >
> > diff --git a/Makefile b/Makefile
> > index 1b23f95db176..cebc6bf5372e 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -751,6 +751,9 @@ else
> >  # These warnings generated too much noise in a regular build.
> >  # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> >  KBUILD_CFLAGS += -Wno-unused-but-set-variable
> > +
> > +# Warn about unmarked fall-throughs in switch statement.
> > +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> >  endif
> >
> >  KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> > @@ -845,9 +848,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> > -print-file-name=include)
> >  # warn about C99 declaration after statement
> >  KBUILD_CFLAGS += -Wdeclaration-after-statement
> >
> > -# Warn about unmarked fall-throughs in switch statement.
> > -KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> > -
> >  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> >  KBUILD_CFLAGS += -Wvla
> >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
>
> I like this more than anything suggested so far. I think a comment
> should be added regarding why this is only enabled for GCC right now but
> that is pretty easy to revert once we have figured out the right course
> of action.

Agree. This is well-explained in the commit log,
but adding a short comment will be nice.



BTW, I personally like the traditional
comment version of fallthrough annotations.

Is there a plan for Clang to support it
as well as the attribute?

Thanks.

-- 
Best Regards
Masahiro Yamada

[PATCH] kbuild: Do not enable -Wimplicit-fallthrough for clang for now

[Nathan Chancellor]

This functionally reverts commit bfd77145f35c ("Makefile: Convert
-Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang").

clang enabled support for -Wimplicit-fallthrough in C in r369414 [1],
which causes a lot of warnings when building the kernel for two reasons:

1. Clang does not support the /* fall through */ comments. There seems
   to be a general consensus in the LLVM community that this is not
   something they want to support. Joe Perches wrote a script to convert
   all of the comments to a "fallthrough" keyword that will be added to
   compiler_attributes.h [2] [3], which catches the vast majority of the
   comments. There doesn't appear to be any consensus in the kernel
   community when to do this conversion.

2. Clang and GCC disagree about falling through to final case statements
   with no content or cases that simply break:

   https://godbolt.org/z/c8csDu

   This difference contributes at least 50 warnings in an allyesconfig
   build for x86, not considering other architectures. This difference
   will need to be discussed to see which compiler is right [4] [5].

[1]: https://github.com/llvm/llvm-project/commit/1e0affb6e564b7361b0aadb38805f26deff4ecfc
[2]: https://lore.kernel.org/lkml/61ddbb86d5e68a15e24ccb06d9b399bbf5ce2da7.camel@perches.com/
[3]: https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git.joe@perches.com/
[4]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[5]: https://github.com/ClangBuiltLinux/linux/issues/636

Given these two problems need discussion and coordination, do not enable
-Wimplicit-fallthrough with clang right now. Add a comment to explain
what is going on as well. This commit should be reverted once these two
issues are fully flushed out and resolved.

Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 Makefile | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index f125625efd60..6007a56bdbee 100644
--- a/Makefile
+++ b/Makefile
@@ -751,6 +751,11 @@ else
 # These warnings generated too much noise in a regular build.
 # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
 KBUILD_CFLAGS += -Wno-unused-but-set-variable
+
+# Warn about unmarked fall-throughs in switch statement.
+# Disabled for clang while comment to attribute conversion happens and
+# https://github.com/ClangBuiltLinux/linux/issues/636 is discussed.
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
 endif
 
 KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
@@ -845,9 +850,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 # warn about C99 declaration after statement
 KBUILD_CFLAGS += -Wdeclaration-after-statement
 
-# Warn about unmarked fall-throughs in switch statement.
-KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
-
 # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
 KBUILD_CFLAGS += -Wvla
 
-- 
2.23.0

Re: [PATCH] kbuild: Do not enable -Wimplicit-fallthrough for clang for now

[Masahiro Yamada]

On Tue, Aug 27, 2019 at 9:42 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> This functionally reverts commit bfd77145f35c ("Makefile: Convert
> -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang").
>
> clang enabled support for -Wimplicit-fallthrough in C in r369414 [1],
> which causes a lot of warnings when building the kernel for two reasons:
>
> 1. Clang does not support the /* fall through */ comments. There seems
>    to be a general consensus in the LLVM community that this is not
>    something they want to support. Joe Perches wrote a script to convert
>    all of the comments to a "fallthrough" keyword that will be added to
>    compiler_attributes.h [2] [3], which catches the vast majority of the
>    comments. There doesn't appear to be any consensus in the kernel
>    community when to do this conversion.
>
> 2. Clang and GCC disagree about falling through to final case statements
>    with no content or cases that simply break:
>
>    https://godbolt.org/z/c8csDu
>
>    This difference contributes at least 50 warnings in an allyesconfig
>    build for x86, not considering other architectures. This difference
>    will need to be discussed to see which compiler is right [4] [5].
>
> [1]: https://github.com/llvm/llvm-project/commit/1e0affb6e564b7361b0aadb38805f26deff4ecfc
> [2]: https://lore.kernel.org/lkml/61ddbb86d5e68a15e24ccb06d9b399bbf5ce2da7.camel@perches.com/
> [3]: https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git.joe@perches.com/
> [4]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
> [5]: https://github.com/ClangBuiltLinux/linux/issues/636
>
> Given these two problems need discussion and coordination, do not enable
> -Wimplicit-fallthrough with clang right now. Add a comment to explain
> what is going on as well. This commit should be reverted once these two
> issues are fully flushed out and resolved.
>
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---

Applied to linux-kbuild. Thanks.

(If other clang folks give tags, I will add them later.)



>  Makefile | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f125625efd60..6007a56bdbee 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -751,6 +751,11 @@ else
>  # These warnings generated too much noise in a regular build.
>  # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
>  KBUILD_CFLAGS += -Wno-unused-but-set-variable
> +
> +# Warn about unmarked fall-throughs in switch statement.
> +# Disabled for clang while comment to attribute conversion happens and
> +# https://github.com/ClangBuiltLinux/linux/issues/636 is discussed.
> +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
>  endif
>
>  KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> @@ -845,9 +850,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
>  # warn about C99 declaration after statement
>  KBUILD_CFLAGS += -Wdeclaration-after-statement
>
> -# Warn about unmarked fall-throughs in switch statement.
> -KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> -
>  # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
>  KBUILD_CFLAGS += -Wvla
>
> --
> 2.23.0
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: Do not enable -Wimplicit-fallthrough for clang for now

[Miguel Ojeda]

On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Applied to linux-kbuild. Thanks.
>
> (If other clang folks give tags, I will add them later.)

Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

Cheers,
Miguel

Re: [PATCH] kbuild: Do not enable -Wimplicit-fallthrough for clang for now

[Nick Desaulniers]

On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > Applied to linux-kbuild. Thanks.
> >
> > (If other clang folks give tags, I will add them later.)
>
> Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>

I verified that GCC didn't get support for -Wimplicit-fallthrough
until GCC ~7.1 release, so the cc-option guard is still required.
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
Thanks for the patch Nathan.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] kbuild: Do not enable -Wimplicit-fallthrough for clang for now

[Nick Desaulniers]

On Wed, Aug 28, 2019 at 10:39 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > >
> > > Applied to linux-kbuild. Thanks.
> > >
> > > (If other clang folks give tags, I will add them later.)
> >
> > Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
>
> I verified that GCC didn't get support for -Wimplicit-fallthrough
> until GCC ~7.1 release, so the cc-option guard is still required.
> Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> Thanks for the patch Nathan.

Also, there's an inconsistency between Makefile vs
scripts/Makefile.extrawarn that's been bugging me: it seems we enable
GCC only flags in Makefile, then disable certain Clang flags in
scripts/Makefile.extrawarn.  Not necessary to fix here and now, but I
hope one day to have one file that has all of the compiler specific
flags in one place (maybe its own file), so I only have to look in one
place.  I know, I know, "patches welcome." ;)

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] kbuild: Do not enable -Wimplicit-fallthrough for clang for now

[Gustavo A. R. Silva]

On 8/28/19 11:20 AM, Masahiro Yamada wrote:

>>
>> Given these two problems need discussion and coordination, do not enable
>> -Wimplicit-fallthrough with clang right now. Add a comment to explain
>> what is going on as well. This commit should be reverted once these two
>> issues are fully flushed out and resolved.
>>
>> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>> ---
> 
> Applied to linux-kbuild. Thanks.
> 
> (If other clang folks give tags, I will add them later.)
> 

Acked-by: Gustavo A. R. Silva <gustavo@embeddedor.com>


Thanks
--
Gustavo

Re: [PATCH] kbuild: Do not enable -Wimplicit-fallthrough for clang for now

[Masahiro Yamada]

On Thu, Aug 29, 2019 at 2:44 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Aug 28, 2019 at 10:39 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > >
> > > On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > >
> > > > Applied to linux-kbuild. Thanks.
> > > >
> > > > (If other clang folks give tags, I will add them later.)
> > >
> > > Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> >
> > I verified that GCC didn't get support for -Wimplicit-fallthrough
> > until GCC ~7.1 release, so the cc-option guard is still required.
> > Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> > Thanks for the patch Nathan.
>
> Also, there's an inconsistency between Makefile vs
> scripts/Makefile.extrawarn that's been bugging me: it seems we enable
> GCC only flags in Makefile, then disable certain Clang flags in
> scripts/Makefile.extrawarn.


All the flags listed in scripts/Makefile.extrawarn depend on W= option.
The options in Makefile are passed irrespective of W=.

There is no inconsistency.


> Not necessary to fix here and now, but I
> hope one day to have one file that has all of the compiler specific
> flags in one place (maybe its own file), so I only have to look in one
> place.  I know, I know, "patches welcome." ;)
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada