Skip to contentSkip to conversation

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

PATCH Superseded Nathan Huckleberry 6 participants 18 messages Aug 15, 2019 – Aug 29, 2019 Kbuild

lore
Share & tools
lore

Nathan Huckleberry

author
#0 lore raw

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

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 <[email protected]>
---
 Makefile                   | 4 ++++
 scripts/Makefile.extrawarn | 3 +++
 2 files changed, 7 insertions(+)
Makefile +4 -0
index 1b23f95db176..93b9744e66a2 100644
@@ -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
scripts/Makefile.extrawarn +3 -0
index a74ce2e3c33e..e12359d69bb7 100644
@@ -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
Signature · 1 line
2.23.0.rc1.153.gdeed80330f-goog
Reviewed-by on v1 1/1
#1 lore raw
Quoted 36 lines · Nathan Huckleberry' via Clang Built Linux

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 <[email protected]>
---
 Makefile                   | 4 ++++
 scripts/Makefile.extrawarn | 3 +++
 2 files changed, 7 insertions(+)
Makefile +4 -0
index 1b23f95db176..93b9744e66a2 100644
@@ -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
scripts/Makefile.extrawarn +0 -0
index a74ce2e3c33e..e12359d69bb7 100644

scripts/Makefile.extrawarn warning-1 += $(call cc-option, -Wunused-but-set-variable) · line 34

Quoted 5 lines
scripts/Makefile.extrawarn +2 -0
@@ -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?

Quoted 7 lines
scripts/Makefile.extrawarn +1 -0
+endif
 # The following turn off the warnings enabled by -Wextra
 warning-1 += -Wno-missing-field-initializers
 warning-1 += -Wno-sign-compare
Signature · 1 line
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 <[email protected]>
#2 lore raw
Quoted 31 lines · Nathan Chancellor
Quoted 29 lines · Nathan Huckleberry' via Clang Built Linux

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 <[email protected]>
---
 Makefile                   | 4 ++++
 scripts/Makefile.extrawarn | 3 +++
 2 files changed, 7 insertions(+)
Makefile +4 -0
index 1b23f95db176..93b9744e66a2 100644
@@ -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
Quoted 14 lines

scripts/Makefile.extrawarn warning-1 += $(call cc-option, -Wunused-but-set-variable) · line 34

Quoted 12 lines
Makefile +0 -0
 # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
 KBUILD_CFLAGS += -Wvla
scripts/Makefile.extrawarn +2 -0
index a74ce2e3c33e..e12359d69bb7 100644
@@ -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

Quoted 13 lines
Quoted 8 lines
scripts/Makefile.extrawarn +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:

Quoted 5 lines

* 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.

Signature · 2 lines
Thanks,
~Nick Desaulniers
author
#3 lore raw

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

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 <[email protected]>
Suggested-by Nathan Chancellor <[email protected]>
Reviewed-by Nathan Chancellor <[email protected]>

---
Changes v1->v2
* Move code to preexisting ifdef

 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)
scripts/Makefile.extrawarn +1 -0
index a74ce2e3c33e..95973a1ee999 100644
@@ -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
Signature · 1 line
2.23.0.rc1.153.gdeed80330f-goog
#4 lore raw
Quoted 24 lines · Nathan Huckleberry' via Clang Built Linux

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 <[email protected]>
Suggested-by Nathan Chancellor <[email protected]>
Reviewed-by Nathan Chancellor <[email protected]>

---
Changes v1->v2
* Move code to preexisting ifdef

 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)
scripts/Makefile.extrawarn +0 -0
index a74ce2e3c33e..95973a1ee999 100644

scripts/Makefile.extrawarn KBUILD_CFLAGS += -Wno-initializer-overrides · line 73

Quoted 4 lines
scripts/Makefile.extrawarn +1 -0
@@ -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.

Signature · 2 lines
Thanks,
~Nick Desaulniers
#5 lore raw
In reply to Nathan Huckleberry

Hi.

Quoted 31 lines · 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 <[email protected]>
Suggested-by Nathan Chancellor <[email protected]>
Reviewed-by Nathan Chancellor <[email protected]>

---
Changes v1->v2
* Move code to preexisting ifdef

 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)
scripts/Makefile.extrawarn +1 -1
index a74ce2e3c33e..95973a1ee999 100644
@@ -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
--
Quoted 2 lines

2.23.0.rc1.153.gdeed80330f-goog

Perhaps, is the following even cleaner?

Makefile +3 -4
index 1b23f95db176..cebc6bf5372e 100644
@@ -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
Signature · 2 lines
Best Regards
Masahiro Yamada
#6 lore raw
Quoted 72 lines · Masahiro Yamada

Hi.

Quoted 31 lines · 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 <[email protected]>
Suggested-by Nathan Chancellor <[email protected]>
Reviewed-by Nathan Chancellor <[email protected]>

---
Changes v1->v2
* Move code to preexisting ifdef

 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)
scripts/Makefile.extrawarn +1 -1
index a74ce2e3c33e..95973a1ee999 100644
@@ -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
--
Quoted 2 lines

2.23.0.rc1.153.gdeed80330f-goog

Perhaps, is the following even cleaner?

Makefile +3 -4
index 1b23f95db176..cebc6bf5372e 100644
@@ -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
Signature · 3 lines
-- 
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

#7 lore raw
Quoted 71 lines · Nathan Chancellor
Quoted 69 lines · Masahiro Yamada

Hi.

Quoted 31 lines · 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 <[email protected]>
Suggested-by Nathan Chancellor <[email protected]>
Reviewed-by Nathan Chancellor <[email protected]>

---
Changes v1->v2
* Move code to preexisting ifdef

 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)
scripts/Makefile.extrawarn +1 -1
index a74ce2e3c33e..95973a1ee999 100644
@@ -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
--
Quoted 2 lines

2.23.0.rc1.153.gdeed80330f-goog

Perhaps, is the following even cleaner?

Makefile +3 -4
index 1b23f95db176..cebc6bf5372e 100644
@@ -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
Quoted 8 lines
Quoted 3 lines

--
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.

Signature · 2 lines
Best Regards
Masahiro Yamada
#8 lore raw

PATCH 1/1 kbuild: Do not enable -Wimplicit-fallthrough for clang for now

In reply to Nathan Huckleberry

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/[email protected]/
[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 <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>
---
 Makefile | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Makefile +5 -3
index f125625efd60..6007a56bdbee 100644
@@ -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
Signature · 1 line
2.23.0
#9 lore raw

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

Quoted 30 lines · 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/[email protected]/
[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

Quoted 8 lines

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 <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>

---

Applied to linux-kbuild. Thanks.

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

Quoted 32 lines
 Makefile | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Makefile +5 -4
index f125625efd60..6007a56bdbee 100644
@@ -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

Signature · 2 lines
Best Regards
Masahiro Yamada
Acked-by
#10 lore raw

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

Quoted 4 lines · Masahiro Yamada

Applied to linux-kbuild. Thanks.

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

Acked-by Miguel Ojeda <[email protected]>

Cheers,
Miguel

Acked-by
#11 lore raw

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

Quoted 9 lines · Miguel Ojeda
Quoted 4 lines · Masahiro Yamada

Applied to linux-kbuild. Thanks.

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

Acked-by Miguel Ojeda <[email protected]>

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 <[email protected]>

Thanks for the patch Nathan.

Signature · 2 lines
Thanks,
~Nick Desaulniers
#12 lore raw

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

Quoted 9 lines · Nick Desaulniers
Quoted 6 lines · Miguel Ojeda
Quoted 3 lines · Masahiro Yamada

Applied to linux-kbuild. Thanks.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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

Acked-by Miguel Ojeda <[email protected]>

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 <[email protected]>

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." ;)

Signature · 2 lines
Thanks,
~Nick Desaulniers
#13 lore raw

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

Quoted 21 lines · Nick Desaulniers
Quoted 9 lines · Nick Desaulniers
Quoted 6 lines · Miguel Ojeda
Quoted 3 lines · Masahiro Yamada

Applied to linux-kbuild. Thanks.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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

Acked-by Miguel Ojeda <[email protected]>

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 <[email protected]>

Thanks for the patch Nathan.

Quoted 4 lines

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.

Quoted 8 lines

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

Signature · 2 lines
Best Regards
Masahiro Yamada
Acked-by
#14 lore raw

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

In reply to Masahiro Yamada
Quoted 6 lines · Masahiro Yamada
Quoted 6 lines

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.

Quoted 8 lines
Quoted 3 lines
Suggested-by Masahiro Yamada <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>

---

Applied to linux-kbuild. Thanks.

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

Acked-by Gustavo A. R. Silva <[email protected]>

Thanks
--
Gustavo

#15 lore raw
In reply to Nathan Chancellor
Quoted 4 lines · Nathan Chancellor

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...

Quoted 3 lines

* 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

#16 lore raw
Quoted 9 lines · Miguel Ojeda
Quoted 3 lines · Nathan Chancellor

* 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

Signature · 2 lines
Thanks,
~Nick Desaulniers
#17 lore raw
Quoted 10 lines · Nick Desaulniers
Quoted 7 lines · Miguel Ojeda
Quoted 3 lines · Nathan Chancellor

* 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.

Quoted 5 lines
Quoted 2 lines

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