Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch fallthrough to empty case statements inconsistent between GCC and Clang #636

Open
nathanchance opened this issue Aug 12, 2019 · 5 comments

Comments

@nathanchance
Copy link
Member

commented Aug 12, 2019

While testing @Nathan-Huckleberry's patch (https://reviews.llvm.org/D64838):

../include/linux/signal.h:245:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
        case 1: ;
        ^
../include/linux/signal.h:245:2: note: insert '__attribute__((fallthrough));' to silence this warning
        case 1: ;
        ^
        __attribute__((fallthrough));
../include/linux/signal.h:245:2: note: insert 'break;' to avoid fall-through
        case 1: ;
        ^
        break;

This comes from include/linux/signal.h in siginitset and siginitsetinv:

static inline void siginitset(sigset_t *set, unsigned long mask)
{
	set->sig[0] = mask;
	switch (_NSIG_WORDS) {
	default:
		memset(&set->sig[1], 0, sizeof(long)*(_NSIG_WORDS-1));
		break;
	case 2: set->sig[1] = 0;
	case 1: ;
	}
}

static inline void siginitsetinv(sigset_t *set, unsigned long mask)
{
	set->sig[0] = ~mask;
	switch (_NSIG_WORDS) {
	default:
		memset(&set->sig[1], -1, sizeof(long)*(_NSIG_WORDS-1));
		break;
	case 2: set->sig[1] = -1;
	case 1: ;
	}
}

It appears that GCC and Clang disagree on emitting a diagnostic when falling through to an empty case statement.

C++ example because C doesn't work right now (#236): https://godbolt.org/z/dFJyAY

Not sure if that should be considering intentional or a bug but making a note of it because this warning appears 6849 times in an x86 allyesconfig build...

@nathanchance

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

I think this also falls under this issue:

../include/linux/jhash.h:113:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
        case 0: /* Nothing left to add */
        ^
../include/linux/jhash.h:113:2: note: insert 'break;' to avoid fall-through
        case 0: /* Nothing left to add */
        ^
        break;
../include/linux/jhash.h:152:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
        case 0: /* Nothing left to add */
        ^
../include/linux/jhash.h:152:2: note: insert 'break;' to avoid fall-through
        case 0: /* Nothing left to add */
        ^
        break;
12 warnings generated.

https://github.com/torvalds/linux/blob/v5.3-rc4/include/linux/jhash.h#L101

	case 2:  a += (u32)k[1]<<8;	/* fall through */
	case 1:  a += k[0];
		 __jhash_final(a, b, c);
	case 0: /* Nothing left to add */
		break;
@nickdesaulniers

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

It appears that GCC and Clang disagree on emitting a diagnostic when falling through to an empty case statement.

Which does what? (Who warns, who does not)?

@nathanchance

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

GCC does not warn, Clang does. Sorry, thought the godbolt link would have made that clear!

I am working through an x86 allyesconfig build after running Joe Perches's cvt_style perl script and most of the warnings I see from clang come from this pattern (falling through to the last case statement that either does nothing or breaks).

@nathanchance

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

These are the few patterns that I am seeing, hopefully this is a little clearer: https://godbolt.org/z/xgkvIh

@nathanchance

This comment has been minimized.

ColinIanKing pushed a commit to ColinIanKing/linux-next-mirror that referenced this issue Aug 30, 2019
This functionally reverts commit bfd7714 ("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]: llvm/llvm-project@1e0affb
[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]: ClangBuiltLinux/linux#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>
Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
fcicq pushed a commit to fcicq/chromiumos-third_party-kernel that referenced this issue Sep 9, 2019
Clang/GCC disagree on how to handle fall-through
especially on handling the fall through comment in code.
Temporarily disable it in the iwl7000 drivers.

GCC 4.9.2 used in Chrome OS does not support this warning
so any coverage loss should be minimal.

Details: ClangBuiltLinux/linux#636

BUG=chromium:997991
TEST=kernel compiles with ToT clang.

Signed-off-by: Manoj Gupta <manojgupta@google.com>
Change-Id: I079e40f3c9282238dd1e4cb964976232fbe971a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1792453
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Commit-Queue: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Manoj Gupta <manojgupta@chromium.org>
fcicq pushed a commit to fcicq/chromiumos-third_party-kernel that referenced this issue Sep 9, 2019
Clang/GCC disagree on how to handle fall-through
especially on handling the fall through comment in code.
Temporarily disable it in the iwl7000 drivers.

GCC 4.9.2 used in Chrome OS does not support this warning
so any coverage loss should be minimal.

Details: ClangBuiltLinux/linux#636

BUG=chromium:997991
TEST=kernel compiles with ToT clang.

Signed-off-by: Manoj Gupta <manojgupta@google.com>
Change-Id: I079e40f3c9282238dd1e4cb964976232fbe971a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1792417
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Commit-Queue: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Manoj Gupta <manojgupta@chromium.org>
fcicq pushed a commit to fcicq/chromiumos-third_party-kernel that referenced this issue Sep 10, 2019
Clang/GCC disagree on how to handle fall-through
especially on handling the fall through comment in code.
Temporarily disable it in the iwl7000 drivers.

GCC 4.9.2 used in Chrome OS does not support this warning
so any coverage loss should be minimal.

Details: ClangBuiltLinux/linux#636

BUG=chromium:997991
TEST=kernel compiles with ToT clang.

Signed-off-by: Manoj Gupta <manojgupta@google.com>
Change-Id: I079e40f3c9282238dd1e4cb964976232fbe971a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1792452
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Manoj Gupta <manojgupta@chromium.org>
Legacy-Commit-Queue: Commit Bot <commit-bot@chromium.org>
fcicq pushed a commit to fcicq/chromiumos-third_party-kernel that referenced this issue Sep 10, 2019
Clang/GCC disagree on how to handle fall-through
especially on handling the fall through comment in code.
Disable it in the iwl7000 drivers.

Kernel 4.4 does not have the annotations for fall through
so this warning was not testing anything on kernel 4.4 anyway.

Details: ClangBuiltLinux/linux#636

BUG=chromium:997991
TEST=kernel compiles with ToT clang.

Signed-off-by: Manoj Gupta <manojgupta@google.com>
Change-Id: I079e40f3c9282238dd1e4cb964976232fbe971a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1779318
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Manoj Gupta <manojgupta@chromium.org>
Legacy-Commit-Queue: Commit Bot <commit-bot@chromium.org>
raphielscape added a commit to raphielscape/linux-scape-workstation that referenced this issue Sep 20, 2019
This functionally reverts commit bfd7714 ("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]: llvm/llvm-project@1e0affb
[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]: ClangBuiltLinux/linux#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>
Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Raphiel Rollerscaperers <rapherion@raphielgang.org>
woodsts pushed a commit to woodsts/linux-stable that referenced this issue Oct 7, 2019
[ Upstream commit e2079e9 ]

This functionally reverts commit bfd7714 ("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]: llvm/llvm-project@1e0affb
[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]: ClangBuiltLinux/linux#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>
Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.