Linux: Enable -Wimplicit-fallthrough for C
https://bugs.llvm.org/show_bug.cgi?id=39382
Why this is only enabled for C++ is enigmatic.
nickdesaulniers
All 38 comments
nathanchance
on 4 Aug 2019
Just as an FYI, this was applied today to make -Wimplicit-fallthrough available for clang: https://git.kernel.org/linus/bfd77145f35c3deafe57e9eb67fff4ccffdaef6e. Looks like Huck's patch has been tentatively accepted, which will turn on this class of warnings for the kernel.
However, I just realized that clang doesn't recognize the /* fall through */ comments that are currently in the kernel for GCC (for good reason... the C parser shouldn't care about comments) so to avoid a ton of warnings, we should get switched over to the fallthrough attribute sooner rather than later; unfortunately, it looks like upstream doesn't want to switch over until Coverity supports that attribute if I am reading the thread correctly.. cc @ojeda and @kees since you two have been apart of those discussions. I am worried that Huck's patch is going to get applied and our CI is going to explode as just an x86 defconfig build has a TON of warnings. If that happens and the attribute isn't going to be supported right away, we will need to revert that kernel commit.
nathanchance
on 11 Aug 2019
Thanks for the mention, I didn't see those messages (I wasn't CC'd). I am keen to move to the attribute solution, and as I said in the LKML, we are just waiting until people are generally in agreement.
However, even if we were to add the compiler attribute now, we would still need to go through all the conversion. I think Joe mentioned he is working on a script to patch the kernel completely, but it will still have to go through maintainers everywhere, which likely means next -rc1.
Unless Linus agrees to take it as a whole, of course; but we are at -rc4, so I wouldn't rely on that; and AFAIK the policy is to avoid non-trivial kernel-wide conversions at once.
Instead of revering, wouldn't it be simpler to make the CI stay on a given Clang (or kernel) commit for the moment; or make the CI apply the reverse patch on top of the latest Clang (or kernel)?
ojeda
on 11 Aug 2019
Thanks for the mention, I didn't see those messages (I wasn't CC'd).
I think you have been keyed into all of them, I was just skimming the archives :)
I think Joe mentioned he is working on a script to patch the kernel completely, but it will still have to go through maintainers everywhere, which likely means next -rc1.
Yes or it will be run by Linus after -rc1, I would assume. Either way, about a month and a half out or so.
Instead of revering, wouldn't it be simpler to make the CI stay on a given Clang (or kernel) commit for the moment
Currently, the clang version in our Docker image is currently about three weeks old so this won't be an immediate issue for our CI. We don't currently have a mechanism to pin the clang version because it all happens through apt.llvm.org; furthermore, that would defeat the purpose of our CI setup since we are trying to use both the latest clang and kernel :)
make the CI apply the reverse patch on top of the latest Clang (or kernel)?
Yes, that is what we would need to do. Luckily, Kernel CI is using clang-8 and I assume will switch to clang-9 when it goes stable so we won't have to worry about this there for a while but hopefully that script can be posted soon so we can start testing.
nathanchance
on 11 Aug 2019
I think you have been keyed into all of them, I was just skimming the archives :)
You are completely right! :)
furthermore, that would defeat the purpose of our CI setup since we are trying to use both the latest clang and kernel :)
Yeah, but it is better to defeat that purpose temporarily rather than revert an otherwise good commit in the mainline kernel. In other words, I would say reverting a commit just because it breaks CI (assuming the commit has nothing to do with the CI, of course) does not seem great; but since we are talking about an independent project's CI, it seems even worse ;)
Yes, that is what we would need to do. Luckily, Kernel CI is using clang-8 and I assume will switch to clang-9 when it goes stable so we won't have to worry about this there for a while but hopefully that script can be posted soon so we can start testing.
+1 If that can be done, it would be better than reverting!
Anyway, don't worry, I will try to move forward the conversion to __fallthrough or whatever we end up choosing; I don't like having it in my GitHub for months and months either ;)
ojeda
on 13 Aug 2019
If the feature causes warnings, keep the warning turned off for Clang builds in scripts/Makefile.extrawarn until we get the kernel and whatever other tools converted.
nickdesaulniers
on 13 Aug 2019
That is probably a wise idea; I don't think we need to modify anything in the main Makefile, we can just disable the warning in scripts/Makefile.extrawarn, which will be included after the main Makefile flag.
nathanchance
on 14 Aug 2019
235 was closed implemented. I'll leave this bug open as I assume there's some cleanup to do.
nickdesaulniers
on 20 Aug 2019
Patch disabling -Wimplicit-fallthrough for clang for now accepted: https://git.kernel.org/next/linux-next/c/e2079e93f562c7f7a030eb7642017ee5eabaaa10
nathanchance
on 30 Aug 2019
Disabled in mainline now: https://git.kernel.org/torvalds/c/e2079e93f562c7f7a030eb7642017ee5eabaaa10
nathanchance
on 20 Sep 2019
There's a patch outstanding from Joe Perches to mass rename the comments (TODO: find link, reping Joe).
https://reviews.llvm.org/rG398b4ed87d488b42032c8d0304324dce76ba9b66 may make this obsolete.
nickdesaulniers
on 8 Feb 2020
Looks like that patch does not like comments on the same line...
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 52269e56c514..83c151577a87 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -153,11 +153,14 @@ static inline void __mm_zero_struct_page(struct page *page)
switch (sizeof(struct page)) {
case 80:
- _pp[9] = 0; /* fallthrough */
+ _pp[9] = 0;
+ /* fallthrough */
case 72:
- _pp[8] = 0; /* fallthrough */
+ _pp[8] = 0;
+ /* fallthrough */
case 64:
- _pp[7] = 0; /* fallthrough */
+ _pp[7] = 0;
+ /* fallthrough */
case 56:
_pp[6] = 0;
_pp[5] = 0;
636 is also an issue still.
nathanchance
on 8 Feb 2020
cc @GustavoARSilva . I asked on list because the Makefile still has:
785 # Warn about unmarked fall-throughs in switch statement.
786 # Disabled for clang while comment to attribute conversion happens and
787 # https://github.com/ClangBuiltLinux/linux/issues/636 is discussed.
788 KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
789 endif
which should be reverted an ensured works with your fixup.
nickdesaulniers
on 1 Sep 2020
cc @GustavoARSilva . I asked on list because the Makefile still has:
785 # Warn about unmarked fall-throughs in switch statement. 786 # Disabled for clang while comment to attribute conversion happens and 787 # https://github.com/ClangBuiltLinux/linux/issues/636 is discussed. 788 KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,) 789 endifwhich should be reverted an ensured works with your fixup.
Hi!
With the following patch applied, the number of warnings is reduced from 79815 to 1330 for Linux 5.9-rc3.
diff --git a/include/linux/jhash.h b/include/linux/jhash.h
index cfb62e9f37be..d30ea9422761 100644
--- a/include/linux/jhash.h
+++ b/include/linux/jhash.h
@@ -99,6 +99,7 @@ static inline u32 jhash(const void *key, u32 length, u32 initval)
case 2: a += (u32)k[1]<<8; fallthrough;
case 1: a += k[0];
__jhash_final(a, b, c);
+ fallthrough;
case 0: /* Nothing left to add */
break;
}
@@ -136,6 +137,7 @@ static inline u32 jhash2(const u32 *k, u32 length, u32 initval)
case 2: b += k[1]; fallthrough;
case 1: a += k[0];
__jhash_final(a, b, c);
+ fallthrough;
case 0: /* Nothing left to add */
break;
}
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 7bbc0e9cf084..a2f01669133d 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -238,6 +238,7 @@ static inline void siginitset(sigset_t *set, unsigned long mask)
memset(&set->sig[1], 0, sizeof(long)*(_NSIG_WORDS-1));
break;
case 2: set->sig[1] = 0;
+ fallthrough;
case 1: ;
}
}
@@ -250,6 +251,7 @@ static inline void siginitsetinv(sigset_t *set, unsigned long mask)
memset(&set->sig[1], -1, sizeof(long)*(_NSIG_WORDS-1));
break;
case 2: set->sig[1] = -1;
+ fallthrough;
case 1: ;
}
}
In the meantime, I can try including the patch above in a pull-request to Linus for 5.9-rc4.
What do you think?
GustavoARSilva
on 2 Sep 2020
Yes those would be great. Or those empty cases could be removed.
nickdesaulniers
on 2 Sep 2020
Yes those would be great. Or those empty cases could be removed.
I'd prefer to leave them in place, as documentation. :)
GustavoARSilva
on 2 Sep 2020
I can try including the patch above in a pull-request to Linus for 5.9-rc4.
Please do so!
ojeda
on 2 Sep 2020
@GustavoARSilva
Please leave here a link to that patch, Thanks.
dileks
on 2 Sep 2020
With the following patch applied, the number of warnings is reduced from 79815 to 1330 for Linux 5.9-rc3.
diff --git a/include/linux/jhash.h b/include/linux/jhash.h index cfb62e9f37be..d30ea9422761 100644 --- a/include/linux/jhash.h +++ b/include/linux/jhash.h @@ -99,6 +99,7 @@ static inline u32 jhash(const void *key, u32 length, u32 initval) case 2: a += (u32)k[1]<<8; fallthrough; case 1: a += k[0]; __jhash_final(a, b, c); + fallthrough; case 0: /* Nothing left to add */ break; } @@ -136,6 +137,7 @@ static inline u32 jhash2(const u32 *k, u32 length, u32 initval) case 2: b += k[1]; fallthrough; case 1: a += k[0]; __jhash_final(a, b, c); + fallthrough; case 0: /* Nothing left to add */ break; } diff --git a/include/linux/signal.h b/include/linux/signal.h index 7bbc0e9cf084..a2f01669133d 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -238,6 +238,7 @@ static inline void siginitset(sigset_t *set, unsigned long mask) memset(&set->sig[1], 0, sizeof(long)*(_NSIG_WORDS-1)); break; case 2: set->sig[1] = 0; + fallthrough; case 1: ; } } @@ -250,6 +251,7 @@ static inline void siginitsetinv(sigset_t *set, unsigned long mask) memset(&set->sig[1], -1, sizeof(long)*(_NSIG_WORDS-1)); break; case 2: set->sig[1] = -1; + fallthrough; case 1: ; } }In the meantime, I can try including the patch above in a pull-request to Linus for 5.9-rc4.
Just to let you know that I'm already working on this, folks. :)
GustavoARSilva
on 2 Sep 2020
Just to let you know that I'm already working on this, folks. :)
OK. Here is the patch:
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/clang&id=b5f3ec944a0e600bc09c59a7e7134f3b81f4dea7
By the way, from the changelog text you might notice that I'm in the Clang team regarding this:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
The patch is currently in my for-next/clang tree. So, if you have any comments, this is the time! :)
GustavoARSilva
on 3 Sep 2020
Consider it:
Reviewed-by: Nathan Chancellor <[email protected]>
Might not be a terrible time to update the location of fallthrough; on the case 2 line in the jhash and jhash2 functions, looks kind of odd on the same line :)
nathanchance
on 3 Sep 2020
@GustavoARSilva The indentation w.r.t. the previous lines is broken in all but the first change -- did you copy-paste the first one? ;)
ojeda
on 3 Sep 2020
@GustavoARSilva
What do you mean by... in [1]:
[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for clang for now")
...
[5] commit a035d552a93b ("Makefile: Globally enable fall-through warning")
Fixes: Tag? Or just as a reference?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-linus/kspp&id=22e05690453409c96585845983fcabc6fb5eeae1
dileks
on 3 Sep 2020
@GustavoARSilva
Tested-by: Sedat Dilek sedat.dilek@gmail.com
dileks
on 3 Sep 2020
Consider it:
Reviewed-by: Nathan Chancellor <[email protected]>Might not be a terrible time to update the location of
fallthrough;on thecase 2line in thejhashandjhash2functions, looks kind of odd on the same line :)
I know, but based on my experience with Linus, it's better not to mess with the coding style adjacent to the change in question.
GustavoARSilva
on 3 Sep 2020
@GustavoARSilva The indentation w.r.t. the previous lines is broken in all but the first change -- did you copy-paste the first one? ;)
You're right. I just fixed it up. Thanks for the catch! :)
GustavoARSilva
on 3 Sep 2020
I know, but based on my experience with Linus, it's better not to mess with the coding style adjacent to the change in question.
Indeed, it increases the number of lines to review, it might make the patch harder to merge & revert, etc. (for those who might not know the reasoning behind that).
You're right. I just fixed it up. Thanks for the catch! :)
@GustavoARSilva You're welcome! :) I don't see this in the LKML, so I leave this here:
Reviewed-by: Miguel Ojeda <[email protected]>
The new patch is here [1].
Please add the missing credits, thanks.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/clang&id=b5f3ec944a0e600bc09c59a7e7134f3b81f4dea7
dileks
on 4 Sep 2020
Also, Joe just sent a patch that looks relevant here: https://lore.kernel.org/lkml/[email protected]/T/#t
nickdesaulniers
on 9 Sep 2020
Also, Joe just sent a patch that looks relevant here: https://lore.kernel.org/lkml/[email protected]/T/#t
Yep; I acked it a couple of minutes ago. I might take it in my tree, but I'll wait for comments from other people, first.
What I don't like is that he did more than straightforward replacements:
"Miscellanea:
o Move or coalesce a couple label blocks above a default: block."
That makes a bit difficult to have his patches taken by Linus.
GustavoARSilva
on 9 Sep 2020
v2: https://lore.kernel.org/lkml/[email protected]/
NVM, I tested v1 which was broken and not v2. v2 very obviously has very many issues to resolve still.
nickdesaulniers
on 7 Nov 2020
@nathanchance @GustavoARSilva what's going on with 6a9dc5fd6170d0a41c8a14eb19e63d94bea5705a?
nickdesaulniers
on 11 Nov 2020
If -D__KERNEL__ is not provided via KBUILD_CFLAGS, then compiler_attributes.h will not be included, which is the case for arch/powerpc/boot. Maybe something along the lines of this would not be too controversial?
diff --git a/include/linux/xz.h b/include/linux/xz.h
index 9884c8440188..885e47a608c7 100644
--- a/include/linux/xz.h
+++ b/include/linux/xz.h
@@ -12,9 +12,11 @@
#define XZ_H
#ifdef __KERNEL__
+# include <linux/compiler_attributes.h>
# include <linux/stddef.h>
# include <linux/types.h>
#else
+# define fallthrough do {} while (0) /* fallthrough */
# include <stddef.h>
# include <stdint.h>
#endif
v2: https://lore.kernel.org/lkml/[email protected]/
NVM, I tested v1 which was broken and not v2. v2 very obviously has very many issues to resolve still.
Ah, didn't notice v2 (and now I see I replied with the partial revert thing after you already sent v2...).
ojeda
on 12 Nov 2020
By the way, this is in mainline since 5.10-rc2: https://git.kernel.org/linus/4169e889e5889405d54cec27d6e9f7f0ce3c7096
And I'm waiting for a link to the test results from the 0-day folks for this treewide patch: https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/clang/fallthrough
I already have a KSPP issue for this, more details: https://github.com/KSPP/linux/issues/115
GustavoARSilva
on 12 Nov 2020
If -D__KERNEL__ is not provided via KBUILD_CFLAGS, then compiler_attributes.h will not be included, which is the case for arch/powerpc/boot.
If arch/powerpc/boot is using code from lib/ but then blowing away KBUILD_CFLAGS, then it's going to be dropping -include $(srctree)/include/linux/compiler_types.h from scripts/Makefile.lib. That is the bug that needs to be fixed. It's another case of #618.
Ah, didn't notice v2 (and now I see I replied with the partial revert thing after you already sent v2...).
Sorry, I should have mentioned it to you via email. That was my bad. I'm sorry.
nickdesaulniers
on 12 Nov 2020
@GustavoARSilva is leading the charge here; assigning to show that this is actively being worked on.
nickdesaulniers
on 3 Dec 2020
Sorry, I should have mentioned it to you via email. That was my bad. I'm sorry.
Not at all!
ojeda
on 3 Dec 2020
Related issues
tpimh
路
4Comments
nathanchance
路
3Comments
tpimh
路
3Comments
nathanchance
路
3Comments
nathanchance
路
3Comments