| Message ID | 20200916200255.1382086-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] nfs: remove incorrect fallthrough label | expand |
On Wed, Sep 16, 2020 at 01:02:55PM -0700, Nick Desaulniers wrote: > There is no case after the default from which to fallthrough to. Clang > will error in this case (unhelpfully without context, see link below) > and GCC will with -Wswitch-unreachable. > > The previous commit should have just replaced the comment with a break > statement. > > If we consider implicit fallthrough to be a design mistake of C, then > all case statements should be terminated with one of the following > statements: > * break > * continue > * return > * __attribute__(__fallthrough__) > * goto (plz no) > * (call of function with __attribute__(__noreturn__)) > > Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro") > Link: https://bugs.llvm.org/show_bug.cgi?id=47539 > Suggested-by: Joe Perches <joe@perches.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks -- Gustavo > --- > Changes v2: > * add break rather than no terminating statement as per Joe. > * add Joe's suggested by tag. > * add blurb about acceptable terminal statements. > > fs/nfs/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > index d20326ee0475..eb2401079b04 100644 > --- a/fs/nfs/super.c > +++ b/fs/nfs/super.c > @@ -889,7 +889,7 @@ static struct nfs_server *nfs_try_mount_request(struct fs_context *fc) > default: > if (rpcauth_get_gssinfo(flavor, &info) != 0) > continue; > - fallthrough; > + break; > } > dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor); > ctx->selected_flavor = flavor; > -- > 2.28.0.618.gf4bc123cb7-goog >
On Wed, 2020-09-16 at 13:02 -0700, Nick Desaulniers wrote: > There is no case after the default from which to fallthrough to. Clang > will error in this case (unhelpfully without context, see link below) > and GCC will with -Wswitch-unreachable. > > The previous commit should have just replaced the comment with a break > statement. > If we consider implicit fallthrough to be a design mistake of C, then > all case statements should be terminated with one of the following > statements: > > * break > * continue > * return > * __attribute__(__fallthrough__) Just fallthrough. __attribute__((__fallthrough__) is only used once in code for the #define. And maybe add see: Documentation/process/deprecated.rst > * goto (plz no) goto is a valid style inside a switch/case label block. There are more than 1500 of these goto <label> uses in the kernel so the 'please no' here doesn't seem reasonable. > * (call of function with __attribute__(__noreturn__)) I guess panic counts. I count 11 of those. Are there any other uses of functions with __noreturn in switch/case label blocks?
On Wed, Sep 16, 2020 at 01:02:55PM -0700, Nick Desaulniers wrote: > There is no case after the default from which to fallthrough to. Clang > will error in this case (unhelpfully without context, see link below) > and GCC will with -Wswitch-unreachable. > > The previous commit should have just replaced the comment with a break > statement. > > If we consider implicit fallthrough to be a design mistake of C, then > all case statements should be terminated with one of the following > statements: > * break > * continue > * return > * __attribute__(__fallthrough__) > * goto (plz no) > * (call of function with __attribute__(__noreturn__)) > > Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro") > Link: https://bugs.llvm.org/show_bug.cgi?id=47539 > Suggested-by: Joe Perches <joe@perches.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > --- > Changes v2: > * add break rather than no terminating statement as per Joe. > * add Joe's suggested by tag. > * add blurb about acceptable terminal statements. > > fs/nfs/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > index d20326ee0475..eb2401079b04 100644 > --- a/fs/nfs/super.c > +++ b/fs/nfs/super.c > @@ -889,7 +889,7 @@ static struct nfs_server *nfs_try_mount_request(struct fs_context *fc) > default: > if (rpcauth_get_gssinfo(flavor, &info) != 0) > continue; > - fallthrough; > + break; > } > dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor); > ctx->selected_flavor = flavor; > -- > 2.28.0.618.gf4bc123cb7-goog >
On Wed, Sep 16, 2020 at 1:19 PM Joe Perches <joe@perches.com> wrote: > > On Wed, 2020-09-16 at 13:02 -0700, Nick Desaulniers wrote: > > * (call of function with __attribute__(__noreturn__)) > > I guess panic counts. I count 11 of those. > > Are there any other uses of functions with __noreturn > in switch/case label blocks? If you look at global_noreturns in tools/objtool/check.c: 145 static const char * const global_noreturns[] = { 146 "__stack_chk_fail", 147 "panic", 148 "do_exit", 149 "do_task_dead", 150 "__module_put_and_exit", 151 "complete_and_exit", 152 "__reiserfs_panic", 153 "lbug_with_loc", 154 "fortify_panic", 155 "usercopy_abort", 156 "machine_real_restart", 157 "rewind_stack_do_exit", 158 "kunit_try_catch_throw", 159 }; Whether they occur or not at the position you ask; I haven't checked.
On Thu, 2020-09-17 at 14:41 -0700, Nick Desaulniers wrote: > On Wed, Sep 16, 2020 at 1:19 PM Joe Perches <joe@perches.com> wrote: > > On Wed, 2020-09-16 at 13:02 -0700, Nick Desaulniers wrote: > > > * (call of function with __attribute__(__noreturn__)) > > > > I guess panic counts. I count 11 of those. > > > > Are there any other uses of functions with __noreturn > > in switch/case label blocks? > > If you look at global_noreturns in tools/objtool/check.c: > 145 static const char * const global_noreturns[] = { > 146 "__stack_chk_fail", > 147 "panic", > 148 "do_exit", > 149 "do_task_dead", > 150 "__module_put_and_exit", > 151 "complete_and_exit", > 152 "__reiserfs_panic", > 153 "lbug_with_loc", > 154 "fortify_panic", > 155 "usercopy_abort", > 156 "machine_real_restart", > 157 "rewind_stack_do_exit", > 158 "kunit_try_catch_throw", > 159 }; > > Whether they occur or not at the position you ask; I haven't checked. Just fyi: Other than the 11 instances of panic, I found only a single use of any other function above in a switch/case: drivers/pnp/pnpbios/core.c:163: complete_and_exit(&unload_sem, 0); case PNP_SYSTEM_NOT_DOCKED: Found with: $ grep-2.5.4 -rP --include=*.[ch] -n '\b(?:__stack_chk_fail|panic|do_exit|do_task_dead|__module_put_and_exit|complete_and_exit|__reiserfs_panic|lbug_with_loc|fortify_panic|usercopy_abort|machine_real_restart|rewind_stack_do_exit|kunit_try_catch_throw)\s*(?:\([^\)]*\))?\s*;\s*(case\s+\w+|default)\s*:' *
diff --git a/fs/nfs/super.c b/fs/nfs/super.c index d20326ee0475..eb2401079b04 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -889,7 +889,7 @@ static struct nfs_server *nfs_try_mount_request(struct fs_context *fc) default: if (rpcauth_get_gssinfo(flavor, &info) != 0) continue; - fallthrough; + break; } dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor); ctx->selected_flavor = flavor;
There is no case after the default from which to fallthrough to. Clang will error in this case (unhelpfully without context, see link below) and GCC will with -Wswitch-unreachable. The previous commit should have just replaced the comment with a break statement. If we consider implicit fallthrough to be a design mistake of C, then all case statements should be terminated with one of the following statements: * break * continue * return * __attribute__(__fallthrough__) * goto (plz no) * (call of function with __attribute__(__noreturn__)) Fixes: 2a1390c95a69 ("nfs: Convert to use the preferred fallthrough macro") Link: https://bugs.llvm.org/show_bug.cgi?id=47539 Suggested-by: Joe Perches <joe@perches.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes v2: * add break rather than no terminating statement as per Joe. * add Joe's suggested by tag. * add blurb about acceptable terminal statements. fs/nfs/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)