Nouveau - Nov 2020 - [PATCH 000/141] Fix fall-through warnings for Clang

On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski
wrote:
> On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:  
> > > > This series aims to fix almost all remaining fall-through
warnings in
> > > > order to enable -Wimplicit-fallthrough for Clang.
> > > > 
> > > > In preparation to enable -Wimplicit-fallthrough for Clang,
explicitly
> > > > add multiple break/goto/return/fallthrough statements
instead of just
> > > > letting the code fall through to the next case.
> > > > 
> > > > Notice that in order to enable -Wimplicit-fallthrough for
Clang, this
> > > > change[1] is meant to be reverted at some point. So, this
patch helps
> > > > to move in that direction.
> > > > 
> > > > Something important to mention is that there is currently a
discrepancy
> > > > between GCC and Clang when dealing with switch fall-through
to empty case
> > > > statements or to cases that only contain a
break/continue/return
> > > > statement[2][3][4].  
> > > 
> > > Are we sure we want to make this change? Was it discussed before?
> > > 
> > > Are there any bugs Clangs puritanical definition of fallthrough
helped
> > > find?
> > > 
> > > IMVHO compiler warnings are supposed to warn about issues that
could
> > > be bugs. Falling through to default: break; can hardly be a bug?!
> > 
> > It's certainly a place where the intent is not always clear. I
think
> > this makes all the cases unambiguous, and doesn't impact the
machine
> > code, since the compiler will happily optimize away any behavioral
> > redundancy.
> 
> If none of the 140 patches here fix a real bug, and there is no change
> to machine code then it sounds to me like a W=2 kind of a warning.
FWIW, this series has found at least one bug so far:
https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ
at mail.gmail.com/

-- 
Kees Cook

On Sun, 2020-11-22 at 08:17 -0800, Kees Cook wrote:
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva
wrote:
> > > > > This series aims to fix almost all remaining
fall-through
> > > > > warnings in order to enable -Wimplicit-fallthrough for
Clang.
> > > > > 
> > > > > In preparation to enable -Wimplicit-fallthrough for
Clang,
> > > > > explicitly add multiple break/goto/return/fallthrough
> > > > > statements instead of just letting the code fall
through to
> > > > > the next case.
> > > > > 
> > > > > Notice that in order to enable -Wimplicit-fallthrough
for
> > > > > Clang, this change[1] is meant to be reverted at some
point.
> > > > > So, this patch helps to move in that direction.
> > > > > 
> > > > > Something important to mention is that there is
currently a
> > > > > discrepancy between GCC and Clang when dealing with
switch
> > > > > fall-through to empty case statements or to cases that
only
> > > > > contain a break/continue/return statement[2][3][4].  
> > > > 
> > > > Are we sure we want to make this change? Was it discussed
> > > > before?
> > > > 
> > > > Are there any bugs Clangs puritanical definition of
fallthrough
> > > > helped find?
> > > > 
> > > > IMVHO compiler warnings are supposed to warn about issues
that
> > > > could be bugs. Falling through to default: break; can hardly
be
> > > > a bug?!  
> > > 
> > > It's certainly a place where the intent is not always clear.
I
> > > think this makes all the cases unambiguous, and doesn't
impact
> > > the machine code, since the compiler will happily optimize away
> > > any behavioral redundancy.
> > 
> > If none of the 140 patches here fix a real bug, and there is no
> > change to machine code then it sounds to me like a W=2 kind of a
> > warning.
> 
> FWIW, this series has found at least one bug so far:
>
https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ
at mail.gmail.com/

Well, it's a problem in an error leg, sure, but it's not a really
compelling reason for a 141 patch series, is it?  All that fixing this
error will do is get the driver to print "oh dear there's a
problem"
under four more conditions than it previously did.

We've been at this for three years now with nearly a thousand patches,
firstly marking all the fall throughs with /* fall through */ and later
changing it to fallthrough.  At some point we do have to ask if the
effort is commensurate with the protection afforded.  Please tell me
our reward for all this effort isn't a single missing error print.

James

On Sun, Nov 22, 2020 at 8:17 AM Kees Cook <keescook at chromium.org>
wrote:
>
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
>
> FWIW, this series has found at least one bug so far:
>
https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ
at mail.gmail.com/
So looks like the bulk of these are:
switch (x) {
  case 0:
    ++x;
  default:
    break;
}

I have a patch that fixes those up for clang:
https://reviews.llvm.org/D91895

There's 3 other cases that don't quite match between GCC and Clang I
observe in the kernel:
switch (x) {
  case 0:
    ++x;
  default:
    goto y;
}
y:;

switch (x) {
  case 0:
    ++x;
  default:
    return;
}

switch (x) {
  case 0:
    ++x;
  default:
    ;
}

Based on your link, and Nathan's comment on my patch, maybe Clang
should continue to warn for the above (at least the `default: return;`
case) and GCC should change?  While the last case looks harmless,
there were only 1 or 2 across the tree in my limited configuration
testing; I really think we should just add `break`s for those.
-- 
Thanks,
~Nick Desaulniers

On Sun, Nov 22, 2020 at 08:17:03AM -0800, Kees Cook
wrote:
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva
wrote:
> > > > > This series aims to fix almost all remaining
fall-through warnings in
> > > > > order to enable -Wimplicit-fallthrough for Clang.
> > > > > 
> > > > > In preparation to enable -Wimplicit-fallthrough for
Clang, explicitly
> > > > > add multiple break/goto/return/fallthrough statements
instead of just
> > > > > letting the code fall through to the next case.
> > > > > 
> > > > > Notice that in order to enable -Wimplicit-fallthrough
for Clang, this
> > > > > change[1] is meant to be reverted at some point. So,
this patch helps
> > > > > to move in that direction.
> > > > > 
> > > > > Something important to mention is that there is
currently a discrepancy
> > > > > between GCC and Clang when dealing with switch
fall-through to empty case
> > > > > statements or to cases that only contain a
break/continue/return
> > > > > statement[2][3][4].  
> > > > 
> > > > Are we sure we want to make this change? Was it discussed
before?
> > > > 
> > > > Are there any bugs Clangs puritanical definition of
fallthrough helped
> > > > find?
> > > > 
> > > > IMVHO compiler warnings are supposed to warn about issues
that could
> > > > be bugs. Falling through to default: break; can hardly be a
bug?!
> > > 
> > > It's certainly a place where the intent is not always clear.
I think
> > > this makes all the cases unambiguous, and doesn't impact the
machine
> > > code, since the compiler will happily optimize away any
behavioral
> > > redundancy.
> > 
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
> 
> FWIW, this series has found at least one bug so far:
>
https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ
at mail.gmail.com/
This is a fallthrough to a return and not to a break.  That should
trigger a warning.  The fallthrough to a break should not generate a
warning.

The bug we're trying to fix is "missing break statement" but if
the
result of the bug is "we hit a break statement" then now we're
just
talking about style.  GCC should limit itself to warning about
potentially buggy code.

regards,
dan carpenter