[GIT PULL] Enable -Wimplicit-fallthrough for Clang for 5.14-rc7

[GIT PULL] Enable -Wimplicit-fallthrough for Clang for 5.14-rc7

[Gustavo A. R. Silva]

The following changes since commit 7c60610d476766e128cc4284bb6349732cbd6606:

  Linux 5.14-rc6 (2021-08-15 13:40:53 -1000)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git tags/implicit-fallthrough-clang-5.14-rc7

for you to fetch changes up to 7bc04ce6b914a06d5823509d1de237787c58e649:

  Makefile: Enable -Wimplicit-fallthrough for Clang (2021-08-18 16:46:44 -0500)

----------------------------------------------------------------
Enable -Wimplicit-fallthrough for Clang for 5.14-rc7

Hi Linus,

Please, pull the following patch that enables -Wimplicit-fallthrough
for Clang 14+, globally.

We had almost 40,000[1] of these issues for Clang in the beginning,
and there might be a couple more out there when building some
architectures with certain configurations. However, with the
recent fixes I think we are in good shape and it is now possible
to enable -Wimplicit-fallthrough for Clang. :)

[1] https://github.com/KSPP/linux/issues/115

Thanks!

----------------------------------------------------------------
Gustavo A. R. Silva (1):
      Makefile: Enable -Wimplicit-fallthrough for Clang

 Makefile | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Re: [GIT PULL] Enable -Wimplicit-fallthrough for Clang for 5.14-rc7

[Linus Torvalds]

On Wed, Aug 18, 2021 at 9:02 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> Enable -Wimplicit-fallthrough for Clang for 5.14-rc7

No. Much too late in the release, particularly knowing that we have
Clang pre-releases that claim to be 14.0.0 and get this wrong.

So no way does this happen like this.

That's doubly true since the clang support for this all adds exactly
_zero_ upside, only downside: we made the gcc checks be stricter, and
gcc ends up having (a) more wide coverage and (b) doesn't have the
bugs clang has.

Honestly, I think the clang "version 14 or more" check is simply
buggy. We *know* that check fails. Don't do it.

Make it "strictly more than 14.0.0" which hopefully doesn't fail, is
cheap and easy, and which will make clang work correctly in the not
too distant future.

In the meantime, we have the gcc checks,  and we'll have a release
without pointless warnings from garbage clang versions.

                Linus

Re: [GIT PULL] Enable -Wimplicit-fallthrough for Clang for 5.14-rc7

[Nathan Chancellor]

On 8/19/2021 12:19 PM, Linus Torvalds wrote:
> On Wed, Aug 18, 2021 at 9:02 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
>>
>> Enable -Wimplicit-fallthrough for Clang for 5.14-rc7
> 
> No. Much too late in the release, particularly knowing that we have
> Clang pre-releases that claim to be 14.0.0 and get this wrong.
> 
> So no way does this happen like this.
> 
> That's doubly true since the clang support for this all adds exactly
> _zero_ upside, only downside: we made the gcc checks be stricter, and
> gcc ends up having (a) more wide coverage and (b) doesn't have the
> bugs clang has.

For what it's worth, clang's version of -Wimplicit-fallthrough would 
have caught the bug in commit 652b44453ea9 ("habanalabs/gaudi: fix 
missing code in ECC handling"). Yes, small fix in the sea of patches 
that were needed to address clang's more pedantic version of the warning 
but this version of the warning is completely in line with the kernel's 
stance of switch statements in Documentation/process/deprecated.rst:

"All switch/case blocks must end in one of:

* break;
* fallthrough;
* continue;
* goto <label>;
* return [expression];"

> Honestly, I think the clang "version 14 or more" check is simply
> buggy. We *know* that check fails. Don't do it.
> 
> Make it "strictly more than 14.0.0" which hopefully doesn't fail, is
> cheap and easy, and which will make clang work correctly in the not
> too distant future.

Just to give some more context, Clang is not like GCC where x.0.0 is the 
development version and x.1.0 is the stable release. The first stable 
version of clang-14 will be clang 14.0.0 when it is released, just as it 
is now, so making it "strictly more than 14.0.0" will just delay 
enabling this even more despite the issue being fixed now. Intel already 
upgraded the clang they use for testing to one that is fixed and Mark 
Brown said Arm does testing with clang-14 but they regularly upgrade as 
well. Nobody should get bitten by this check in its current form unless 
they are not upgrading their development version of clang but in that 
case, they are doing it wrong to begin with in my opinion (would it not 
be the same as someone using v5.13-rc1 when v5.13-rc6 is out?)

If we waited until the next merge window, that would certainly give 
various entities enough time to upgrade their pre-release versions.

However, if you are truly opposed to a version check, can we at least 
just check for the presence of -Wunreachable-code-fallthrough as I 
suggested before? Yes, it is slightly more expensive as we have to call 
the compiler rather than using make builtins but then your concern of 
-Wimplicit-fallthrough getting enabled when it is buggy is completely 
addressed and people who have a fixed toolchain get access to the 
warning now.

Cheers,
Nathan

Re: [GIT PULL] Enable -Wimplicit-fallthrough for Clang for 5.14-rc7

[Linus Torvalds]

On Thu, Aug 19, 2021 at 1:17 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> For what it's worth, clang's version of -Wimplicit-fallthrough would
> have caught the bug in commit 652b44453ea9 [..]

The thing is, any warning can catch a bug. The "signed pointer"
warnings could catch things too if people really care about "unsigned
char *" vs just plain "char *".

But warnings that have too many false positives are more likely to
hide bugs than expose them. So it's very much a balancing act.

I personally think that false positives are deadly: I'd much rather
have a c completely clean build where are the warnings are big red
flags, than a build that has warnings that _may_ be signs of bugs, but
where you have known false positives that have no sane workaround.

Because with even a _single_ false positive, people will immediately
just stop caring about any other warnings at all. We've seen that over
and over again.

This is why a compiler warning - to be useful - has to be completely
unambiguously about bad code, or at least have a alternate "good code
pattern" that is no worse than the code that the warning about.

> However, if you are truly opposed to a version check, can we at least
> just check for the presence of -Wunreachable-code-fallthrough as I
> suggested before?

I'm ok with that, since at that point there are no false warnings.

But even then, it's much too late for 5.14 by now, considering that
this has had issues, and that we already enable the warnings on the
common gcc builds.

                Linus