Re: [PATCH v2] nfs: remove incorrect fallthrough label

Re: [PATCH v2] nfs: remove incorrect fallthrough label

[linmiaohe]

Nick Desaulniers <ndesaulniers@google.com> 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: Miaohe Lin <linmiaohe@huawei.com>

Re: [PATCH v2] nfs: remove incorrect fallthrough label

[Joe Perches]

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*:' *

Re: [PATCH v2] nfs: remove incorrect fallthrough label

[Nick Desaulniers]

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.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] nfs: remove incorrect fallthrough label

[Nathan Chancellor]

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
> 

Re: [PATCH v2] nfs: remove incorrect fallthrough label

[Joe Perches]

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?

Re: [PATCH v2] nfs: remove incorrect fallthrough label

[Gustavo A. R. Silva]

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
> 

[PATCH v2] nfs: remove incorrect fallthrough label

[Nick Desaulniers]

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(-)

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