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

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 Mon, 23 Nov 2020 17:32:51 -0800 Nick Desaulniers
wrote:
> 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
?

On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers
wrote:
> 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
I still think this isn't right -- it's a case statement that runs off
the end without an explicit flow control determination. I think Clang is
right to warn for these, and GCC should also warn.

-- 
Kees Cook

On 24/11/2020 21:25, Kees Cook wrote:
> I still think this isn't right -- it's a case statement that runs
off
> the end without an explicit flow control determination.
Proves too much ? for instance
    case foo:
    case bar:
        thing;
        break;
 doesn't require a fallthrough; after case foo:, and afaik
 no-one is suggesting it should.  Yet it, too, is "a case
 statement that runs off the end without an explicit flow
 control determination".

-ed

On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers
wrote:
> 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;
> }
This should not generate a warning.
> 
> 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:;
This should generate a warning.
> 
> switch (x) {
>   case 0:
>     ++x;
>   default:
>     return;
> }
Warn for this.

> 
> switch (x) {
>   case 0:
>     ++x;
>   default:
>     ;
> }
Don't warn for this.

If adding a break statement changes the flow of the code then warn about
potentially missing break statements, but if it doesn't change anything
then don't warn about it.

regards,
dan carpenter