[PATCH] net: sctp: Rename fallthrough label to unhandled

[PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

fallthrough may become a pseudo reserved keyword so this only use of
fallthrough is better renamed to allow it.

Signed-off-by: Joe Perches <joe@perches.com>
---
 net/sctp/sm_make_chunk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 36bd8a6e82df..3fdcaa2fbf12 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2152,7 +2152,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
 	case SCTP_PARAM_SET_PRIMARY:
 		if (net->sctp.addip_enable)
 			break;
-		goto fallthrough;
+		goto unhandled;
 
 	case SCTP_PARAM_HOST_NAME_ADDRESS:
 		/* Tell the peer, we won't support this param.  */
@@ -2163,11 +2163,11 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
 	case SCTP_PARAM_FWD_TSN_SUPPORT:
 		if (ep->prsctp_enable)
 			break;
-		goto fallthrough;
+		goto unhandled;
 
 	case SCTP_PARAM_RANDOM:
 		if (!ep->auth_enable)
-			goto fallthrough;
+			goto unhandled;
 
 		/* SCTP-AUTH: Secion 6.1
 		 * If the random number is not 32 byte long the association
@@ -2184,7 +2184,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
 
 	case SCTP_PARAM_CHUNKS:
 		if (!ep->auth_enable)
-			goto fallthrough;
+			goto unhandled;
 
 		/* SCTP-AUTH: Section 3.2
 		 * The CHUNKS parameter MUST be included once in the INIT or
@@ -2200,7 +2200,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
 
 	case SCTP_PARAM_HMAC_ALGO:
 		if (!ep->auth_enable)
-			goto fallthrough;
+			goto unhandled;
 
 		hmacs = (struct sctp_hmac_algo_param *)param.p;
 		n_elt = (ntohs(param.p->length) -
@@ -2223,7 +2223,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
 			retval = SCTP_IERROR_ABORT;
 		}
 		break;
-fallthrough:
+unhandled:
 	default:
 		pr_debug("%s: unrecognized param:%d for chunk:%d\n",
 			 __func__, ntohs(param.p->type), cid);
-- 
2.15.0

[RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

Reserve the pseudo keyword 'fallthrough' for the ability to convert the
various case block /* fallthrough */ style comments to appear to be an
actual reserved word with the same gcc case block missing fallthrough
warning capability.

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

	break;
	fallthrough;
	goto <label>;
	return [expression];

fallthough is gcc's __attribute__((__fallthrough__)) which was introduced
in gcc version 7..

fallthrough devolves to an empty "do {} while (0)" if the compiler version
(any version less than gcc 7) does not support the attribute.

Signed-off-by: Joe Perches <joe@perches.com>
---

For allow compilation of a kernel that includes net/sctp, this patch
also requires a rename of the use of fallthrough as a label in
net/sctp/sm_make_chunk.c that was submitted as:

https://lore.kernel.org/lkml/e0dd3af448e38e342c1ac6e7c0c802696eb77fd6.1564549413.git.joe@perches.com/

 include/linux/compiler_attributes.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 6b318efd8a74..cdf016596659 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -40,6 +40,7 @@
 # define __GCC4_has_attribute___noclone__             1
 # define __GCC4_has_attribute___nonstring__           0
 # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)
+# define __GCC4_has_attribute___fallthrough__         0
 #endif
 
 /*
@@ -185,6 +186,22 @@
 # define __noclone
 #endif
 
+/*
+ * Add the pseudo keyword 'fallthrough' so case statement blocks
+ * must end with any of these keywords:
+ *   break;
+ *   fallthrough;
+ *   goto <label>;
+ *   return [expression];
+ *
+ *  gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
+ */
+#if __has_attribute(__fallthrough__)
+# define fallthrough                    __attribute__((__fallthrough__))
+#else
+# define fallthrough                    do {} while (0)  /* fallthrough */
+#endif
+
 /*
  * Note the missing underscores.
  *
-- 
2.15.0

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Peter Zijlstra]

On Tue, Jul 30, 2019 at 10:35:18PM -0700, Joe Perches wrote:
> Reserve the pseudo keyword 'fallthrough' for the ability to convert the
> various case block /* fallthrough */ style comments to appear to be an
> actual reserved word with the same gcc case block missing fallthrough
> warning capability.
> 
> All switch/case blocks now must end in one of:
> 
> 	break;
> 	fallthrough;
> 	goto <label>;
> 	return [expression];
> 
> fallthough is gcc's __attribute__((__fallthrough__)) which was introduced
> in gcc version 7..
> 
> fallthrough devolves to an empty "do {} while (0)" if the compiler version
> (any version less than gcc 7) does not support the attribute.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

_MUCH_ better than that silly comment, thanks for doing this!

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Wed, 2019-07-31 at 11:02 +0200, Peter Zijlstra wrote:
> On Tue, Jul 30, 2019 at 10:35:18PM -0700, Joe Perches wrote:
> > Reserve the pseudo keyword 'fallthrough' for the ability to convert the
> > various case block /* fallthrough */ style comments to appear to be an
> > actual reserved word with the same gcc case block missing fallthrough
> > warning capability.
> > 
> > All switch/case blocks now must end in one of:
> > 
> > 	break;
> > 	fallthrough;
> > 	goto <label>;
> > 	return [expression];
> > 
> > fallthough is gcc's __attribute__((__fallthrough__)) which was introduced
> > in gcc version 7..
> > 
> > fallthrough devolves to an empty "do {} while (0)" if the compiler version
> > (any version less than gcc 7) does not support the attribute.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> _MUCH_ better than that silly comment, thanks for doing this!
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

I also hacked up a little perl script to do most of the
conversions and realignments for the 4200+ current comments.

I'll work on it a bit more and then post it when it's
presentable.

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Neil Horman]

On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> fallthrough may become a pseudo reserved keyword so this only use of
> fallthrough is better renamed to allow it.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
Are you referring to the __attribute__((fallthrough)) statement that gcc
supports?  If so the compiler should by all rights be able to differentiate
between a null statement attribute and a explicit goto and label without the
need for renaming here.  Or are you referring to something else?

Neil

> ---
>  net/sctp/sm_make_chunk.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 36bd8a6e82df..3fdcaa2fbf12 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2152,7 +2152,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  	case SCTP_PARAM_SET_PRIMARY:
>  		if (net->sctp.addip_enable)
>  			break;
> -		goto fallthrough;
> +		goto unhandled;
>  
>  	case SCTP_PARAM_HOST_NAME_ADDRESS:
>  		/* Tell the peer, we won't support this param.  */
> @@ -2163,11 +2163,11 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  	case SCTP_PARAM_FWD_TSN_SUPPORT:
>  		if (ep->prsctp_enable)
>  			break;
> -		goto fallthrough;
> +		goto unhandled;
>  
>  	case SCTP_PARAM_RANDOM:
>  		if (!ep->auth_enable)
> -			goto fallthrough;
> +			goto unhandled;
>  
>  		/* SCTP-AUTH: Secion 6.1
>  		 * If the random number is not 32 byte long the association
> @@ -2184,7 +2184,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  
>  	case SCTP_PARAM_CHUNKS:
>  		if (!ep->auth_enable)
> -			goto fallthrough;
> +			goto unhandled;
>  
>  		/* SCTP-AUTH: Section 3.2
>  		 * The CHUNKS parameter MUST be included once in the INIT or
> @@ -2200,7 +2200,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  
>  	case SCTP_PARAM_HMAC_ALGO:
>  		if (!ep->auth_enable)
> -			goto fallthrough;
> +			goto unhandled;
>  
>  		hmacs = (struct sctp_hmac_algo_param *)param.p;
>  		n_elt = (ntohs(param.p->length) -
> @@ -2223,7 +2223,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  			retval = SCTP_IERROR_ABORT;
>  		}
>  		break;
> -fallthrough:
> +unhandled:
>  	default:
>  		pr_debug("%s: unrecognized param:%d for chunk:%d\n",
>  			 __func__, ntohs(param.p->type), cid);
> -- 
> 2.15.0
> 
> 

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> > fallthrough may become a pseudo reserved keyword so this only use of
> > fallthrough is better renamed to allow it.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> Are you referring to the __attribute__((fallthrough)) statement that gcc
> supports?  If so the compiler should by all rights be able to differentiate
> between a null statement attribute and a explicit goto and label without the
> need for renaming here.  Or are you referring to something else?

Hi.

I sent after this a patch that adds

# define fallthrough                    __attribute__((__fallthrough__))

https://lore.kernel.org/patchwork/patch/1108577/

So this rename is a prerequisite to adding this #define.

> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
[]
> > @@ -2152,7 +2152,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
> >  	case SCTP_PARAM_SET_PRIMARY:
> >  		if (net->sctp.addip_enable)
> >  			break;
> > -		goto fallthrough;
> > +		goto unhandled;

etc...

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Neil Horman]

On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> > > fallthrough may become a pseudo reserved keyword so this only use of
> > > fallthrough is better renamed to allow it.
> > > 
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > Are you referring to the __attribute__((fallthrough)) statement that gcc
> > supports?  If so the compiler should by all rights be able to differentiate
> > between a null statement attribute and a explicit goto and label without the
> > need for renaming here.  Or are you referring to something else?
> 
> Hi.
> 
> I sent after this a patch that adds
> 
> # define fallthrough                    __attribute__((__fallthrough__))
> 
> https://lore.kernel.org/patchwork/patch/1108577/
> 
> So this rename is a prerequisite to adding this #define.
> 
why not just define __fallthrough instead, like we do for all the other
attributes we alias (i.e. __read_mostly, __protected_by, __unused, __exception,
etc)

Neil

> > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> []
> > > @@ -2152,7 +2152,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
> > >  	case SCTP_PARAM_SET_PRIMARY:
> > >  		if (net->sctp.addip_enable)
> > >  			break;
> > > -		goto fallthrough;
> > > +		goto unhandled;
> 
> etc...
> 
> 
> 

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> > > > fallthrough may become a pseudo reserved keyword so this only use of
> > > > fallthrough is better renamed to allow it.
> > > > 
> > > > Signed-off-by: Joe Perches <joe@perches.com>
> > > Are you referring to the __attribute__((fallthrough)) statement that gcc
> > > supports?  If so the compiler should by all rights be able to differentiate
> > > between a null statement attribute and a explicit goto and label without the
> > > need for renaming here.  Or are you referring to something else?
> > 
> > Hi.
> > 
> > I sent after this a patch that adds
> > 
> > # define fallthrough                    __attribute__((__fallthrough__))
> > 
> > https://lore.kernel.org/patchwork/patch/1108577/
> > 
> > So this rename is a prerequisite to adding this #define.
> > 
> why not just define __fallthrough instead, like we do for all the other
> attributes we alias (i.e. __read_mostly, __protected_by, __unused, __exception,
> etc)

Because it's not as intelligible when used as a statement.

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On Tue 2019-07-30 22:35:18, Joe Perches wrote:
> Reserve the pseudo keyword 'fallthrough' for the ability to convert the
> various case block /* fallthrough */ style comments to appear to be an
> actual reserved word with the same gcc case block missing fallthrough
> warning capability.

Acked-by: Pavel Machek <pavel@ucw.cz>

> +/*
> + * Add the pseudo keyword 'fallthrough' so case statement blocks
> + * must end with any of these keywords:
> + *   break;
> + *   fallthrough;
> + *   goto <label>;
> + *   return [expression];
> + *
> + *  gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> + */
> +#if __has_attribute(__fallthrough__)
> +# define fallthrough                    __attribute__((__fallthrough__))
> +#else
> +# define fallthrough                    do {} while (0)  /* fallthrough */
> +#endif
> +

Will various checkers (and gcc) recognize, that comment in a macro,
and disable the warning accordingly?

Explanation that the comment is "magic" might not be a bad idea.

Thanks,

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Wed, 2019-07-31 at 19:14 +0200, Pavel Machek wrote:
> On Tue 2019-07-30 22:35:18, Joe Perches wrote:
> > Reserve the pseudo keyword 'fallthrough' for the ability to convert the
> > various case block /* fallthrough */ style comments to appear to be an
> > actual reserved word with the same gcc case block missing fallthrough
> > warning capability.
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> > +/*
> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> > + * must end with any of these keywords:
> > + *   break;
> > + *   fallthrough;
> > + *   goto <label>;
> > + *   return [expression];
> > + *
> > + *  gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> > + */
> > +#if __has_attribute(__fallthrough__)
> > +# define fallthrough                    __attribute__((__fallthrough__))
> > +#else
> > +# define fallthrough                    do {} while (0)  /* fallthrough */
> > +#endif
> > +
> 
> Will various checkers (and gcc) recognize, that comment in a macro,
> and disable the warning accordingly?

Current non-gcc tools:  I doubt it.

And that's unlikely as the comments are supposed to be stripped
before the macro expansion phase.

gcc 7+, which by now probably most developers actually use, will
though
and likely that's sufficient.

> Explanation that the comment is "magic" might not be a bad idea.

The comment was more for the reader.

cheers, Joe

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[hpa]

On July 31, 2019 10:51:37 AM PDT, Joe Perches <joe@perches.com> wrote:
>On Wed, 2019-07-31 at 19:14 +0200, Pavel Machek wrote:
>> On Tue 2019-07-30 22:35:18, Joe Perches wrote:
>> > Reserve the pseudo keyword 'fallthrough' for the ability to convert
>the
>> > various case block /* fallthrough */ style comments to appear to be
>an
>> > actual reserved word with the same gcc case block missing
>fallthrough
>> > warning capability.
>> 
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> 
>> > +/*
>> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
>> > + * must end with any of these keywords:
>> > + *   break;
>> > + *   fallthrough;
>> > + *   goto <label>;
>> > + *   return [expression];
>> > + *
>> > + *  gcc:
>https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
>> > + */
>> > +#if __has_attribute(__fallthrough__)
>> > +# define fallthrough                   
>__attribute__((__fallthrough__))
>> > +#else
>> > +# define fallthrough                    do {} while (0)  /*
>fallthrough */
>> > +#endif
>> > +
>> 
>> Will various checkers (and gcc) recognize, that comment in a macro,
>> and disable the warning accordingly?
>
>Current non-gcc tools:  I doubt it.
>
>And that's unlikely as the comments are supposed to be stripped
>before the macro expansion phase.
>
>gcc 7+, which by now probably most developers actually use, will
>though
>and likely that's sufficient.
>
>> Explanation that the comment is "magic" might not be a bad idea.
>
>The comment was more for the reader.
>
>cheers, Joe

If the comments are stripped, how would the compiler see them to be able to issue a warning? I would guess that it is retained or replaced with some other magic token.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Peter Zijlstra]

On Wed, Jul 31, 2019 at 11:24:36AM -0700, hpa@zytor.com wrote:
> >> > +/*
> >> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> >> > + * must end with any of these keywords:
> >> > + *   break;
> >> > + *   fallthrough;
> >> > + *   goto <label>;
> >> > + *   return [expression];
> >> > + *
> >> > + *  gcc: >https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> >> > + */
> >> > +#if __has_attribute(__fallthrough__)
> >> > +# define fallthrough                   __attribute__((__fallthrough__))
> >> > +#else
> >> > +# define fallthrough                    do {} while (0)  /* fallthrough */
> >> > +#endif
> >> > +

> If the comments are stripped, how would the compiler see them to be
> able to issue a warning? I would guess that it is retained or replaced
> with some other magic token.

Everything that has the warning (GCC-7+/CLANG-9) has that attribute.

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Kees Cook]

On Wed, Jul 31, 2019 at 08:48:32PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 31, 2019 at 11:24:36AM -0700, hpa@zytor.com wrote:
> > >> > +/*
> > >> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> > >> > + * must end with any of these keywords:
> > >> > + *   break;
> > >> > + *   fallthrough;
> > >> > + *   goto <label>;
> > >> > + *   return [expression];
> > >> > + *
> > >> > + *  gcc: >https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> > >> > + */
> > >> > +#if __has_attribute(__fallthrough__)
> > >> > +# define fallthrough                   __attribute__((__fallthrough__))
> > >> > +#else
> > >> > +# define fallthrough                    do {} while (0)  /* fallthrough */
> > >> > +#endif
> > >> > +
> 
> > If the comments are stripped, how would the compiler see them to be
> > able to issue a warning? I would guess that it is retained or replaced
> > with some other magic token.
> 
> Everything that has the warning (GCC-7+/CLANG-9) has that attribute.

I'd like to make sure we don't regress Coverity most of all. If the
recent updates to the Coverity scanner include support for the attribute
now, then I'm all for it. :)

-- 
Kees Cook

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Neil Horman]

On Wed, Jul 31, 2019 at 09:35:31AM -0700, Joe Perches wrote:
> On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> > On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> > > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> > > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> > > > > fallthrough may become a pseudo reserved keyword so this only use of
> > > > > fallthrough is better renamed to allow it.
> > > > > 
> > > > > Signed-off-by: Joe Perches <joe@perches.com>
> > > > Are you referring to the __attribute__((fallthrough)) statement that gcc
> > > > supports?  If so the compiler should by all rights be able to differentiate
> > > > between a null statement attribute and a explicit goto and label without the
> > > > need for renaming here.  Or are you referring to something else?
> > > 
> > > Hi.
> > > 
> > > I sent after this a patch that adds
> > > 
> > > # define fallthrough                    __attribute__((__fallthrough__))
> > > 
> > > https://lore.kernel.org/patchwork/patch/1108577/
> > > 
> > > So this rename is a prerequisite to adding this #define.
> > > 
> > why not just define __fallthrough instead, like we do for all the other
> > attributes we alias (i.e. __read_mostly, __protected_by, __unused, __exception,
> > etc)
> 
> Because it's not as intelligible when used as a statement.
I think thats somewhat debatable.  __fallthrough to me looks like an internal
macro, whereas fallthrough looks like a comment someone forgot to /* */

Neil

> 
> 
> 
> 

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Miguel Ojeda]

On Wed, Jul 31, 2019 at 10:02 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jul 31, 2019 at 08:48:32PM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 31, 2019 at 11:24:36AM -0700, hpa@zytor.com wrote:
> > > >> > +/*
> > > >> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> > > >> > + * must end with any of these keywords:
> > > >> > + *   break;
> > > >> > + *   fallthrough;
> > > >> > + *   goto <label>;
> > > >> > + *   return [expression];
> > > >> > + *
> > > >> > + *  gcc: >https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> > > >> > + */
> > > >> > +#if __has_attribute(__fallthrough__)
> > > >> > +# define fallthrough                   __attribute__((__fallthrough__))
> > > >> > +#else
> > > >> > +# define fallthrough                    do {} while (0)  /* fallthrough */
> > > >> > +#endif
> > > >> > +
> >
> > > If the comments are stripped, how would the compiler see them to be
> > > able to issue a warning? I would guess that it is retained or replaced
> > > with some other magic token.
> >
> > Everything that has the warning (GCC-7+/CLANG-9) has that attribute.
>
> I'd like to make sure we don't regress Coverity most of all. If the
> recent updates to the Coverity scanner include support for the attribute
> now, then I'm all for it. :)

We had this discussion a while ago (October) and the consensus was
that we would like to wait for a while until all tools were ready to
use the attribute and everyone's would be happy:

  https://lore.kernel.org/lkml/20181021171414.22674-1-miguel.ojeda.sandonis@gmail.com/

Is everyone happy this time around? :-)

Cheers,
Miguel

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[hpa]

On July 31, 2019 11:48:32 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>On Wed, Jul 31, 2019 at 11:24:36AM -0700, hpa@zytor.com wrote:
>> >> > +/*
>> >> > + * Add the pseudo keyword 'fallthrough' so case statement
>blocks
>> >> > + * must end with any of these keywords:
>> >> > + *   break;
>> >> > + *   fallthrough;
>> >> > + *   goto <label>;
>> >> > + *   return [expression];
>> >> > + *
>> >> > + *  gcc:
>>https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
>> >> > + */
>> >> > +#if __has_attribute(__fallthrough__)
>> >> > +# define fallthrough                  
>__attribute__((__fallthrough__))
>> >> > +#else
>> >> > +# define fallthrough                    do {} while (0)  /*
>fallthrough */
>> >> > +#endif
>> >> > +
>
>> If the comments are stripped, how would the compiler see them to be
>> able to issue a warning? I would guess that it is retained or
>replaced
>> with some other magic token.
>
>Everything that has the warning (GCC-7+/CLANG-9) has that attribute.

The standard is moving toward adding this as an attribute with the [[fallthrough]] syntax; it is in C++17, not sure when it will be in C be if it isn't already.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Wed, 2019-07-31 at 22:59 +0200, Miguel Ojeda wrote:
> On Wed, Jul 31, 2019 at 10:02 PM Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Jul 31, 2019 at 08:48:32PM +0200, Peter Zijlstra wrote:
> > > On Wed, Jul 31, 2019 at 11:24:36AM -0700, hpa@zytor.com wrote:
> > > > > > > +/*
> > > > > > > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> > > > > > > + * must end with any of these keywords:
> > > > > > > + *   break;
> > > > > > > + *   fallthrough;
> > > > > > > + *   goto <label>;
> > > > > > > + *   return [expression];
> > > > > > > + *
> > > > > > > + *  gcc: >https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> > > > > > > + */
> > > > > > > +#if __has_attribute(__fallthrough__)
> > > > > > > +# define fallthrough                   __attribute__((__fallthrough__))
> > > > > > > +#else
> > > > > > > +# define fallthrough                    do {} while (0)  /* fallthrough */
> > > > > > > +#endif
> > > > > > > +
> > > > If the comments are stripped, how would the compiler see them to be
> > > > able to issue a warning? I would guess that it is retained or replaced
> > > > with some other magic token.
> > > 
> > > Everything that has the warning (GCC-7+/CLANG-9) has that attribute.
> > 
> > I'd like to make sure we don't regress Coverity most of all. If the
> > recent updates to the Coverity scanner include support for the attribute
> > now, then I'm all for it. :)
> 
> We had this discussion a while ago (October) and the consensus was
> that we would like to wait for a while until all tools were ready to
> use the attribute and everyone's would be happy:
> 
>   https://lore.kernel.org/lkml/20181021171414.22674-1-miguel.ojeda.sandonis@gmail.com/
> 
> Is everyone happy this time around? :-)

Note also that this doesn't actually _use_ fallthrough
it just reserves it.

Patches that convert /* fallthrough */ et al, to fallthrough
would only be published when everyone's happy enough.

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

On Wed, 2019-07-31 at 16:58 -0400, Neil Horman wrote:
> On Wed, Jul 31, 2019 at 09:35:31AM -0700, Joe Perches wrote:
> > On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> > > On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> > > > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> > > > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> > > > > > fallthrough may become a pseudo reserved keyword so this only use of
> > > > > > fallthrough is better renamed to allow it.
> > > > > > 
> > > > > > Signed-off-by: Joe Perches <joe@perches.com>
> > > > > Are you referring to the __attribute__((fallthrough)) statement that gcc
> > > > > supports?  If so the compiler should by all rights be able to differentiate
> > > > > between a null statement attribute and a explicit goto and label without the
> > > > > need for renaming here.  Or are you referring to something else?
> > > > 
> > > > Hi.
> > > > 
> > > > I sent after this a patch that adds
> > > > 
> > > > # define fallthrough                    __attribute__((__fallthrough__))
> > > > 
> > > > https://lore.kernel.org/patchwork/patch/1108577/
> > > > 
> > > > So this rename is a prerequisite to adding this #define.
> > > > 
> > > why not just define __fallthrough instead, like we do for all the other
> > > attributes we alias (i.e. __read_mostly, __protected_by, __unused, __exception,
> > > etc)
> > 
> > Because it's not as intelligible when used as a statement.
> I think thats somewhat debatable.  __fallthrough to me looks like an internal
> macro, whereas fallthrough looks like a comment someone forgot to /* */


I'd rather see:

	switch (foo) {
	case FOO:
		bar |= baz;
		fallthrough;
	case BAR:
		bar |= qux;
		break;
	default:
		error();
	}

than

	switch (foo) {
	case FOO:
		bar |= baz;
		__fallthrough;
	case BAR:
		bar |= qux;
		break;
	default:
		error();
	}

or esoecially

	switch (foo) {
	case FOO:
		bar |= baz;
		/* fallthrough
*/;
	case BAR:
		bar |= qux;
		break;
	default:
		error();
	}

but <shrug>, bikeshed ahoy!...

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Miguel Ojeda]

On Wed, Jul 31, 2019 at 11:01 PM <hpa@zytor.com> wrote:
>
> The standard is moving toward adding this as an attribute with the [[fallthrough]] syntax; it is in C++17, not sure when it will be in C be if it isn't already.

Not yet, but it seems to be coming:

  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf

However, even if C2x gets it, it will be quite a while until the GCC
minimum version gets bumped up to that, so...

Cheers,
Miguel

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Miguel Ojeda]

On Thu, Aug 1, 2019 at 12:07 AM Joe Perches <joe@perches.com> wrote:
>
> Note also that this doesn't actually _use_ fallthrough
> it just reserves it.

If we are reserving it, we should be giving a compile error instead. I
don't see how users will understand they shouldn't use it just yet (it
is the same as adding it, which actually looks more like
encouragement!).

Cheers,
Miguel

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[hpa]

On July 31, 2019 4:55:47 PM PDT, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>On Wed, Jul 31, 2019 at 11:01 PM <hpa@zytor.com> wrote:
>>
>> The standard is moving toward adding this as an attribute with the
>[[fallthrough]] syntax; it is in C++17, not sure when it will be in C
>be if it isn't already.
>
>Not yet, but it seems to be coming:
>
>  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf
>
>However, even if C2x gets it, it will be quite a while until the GCC
>minimum version gets bumped up to that, so...
>
>Cheers,
>Miguel

The point was that we should plan ahead in whatever we end up doing.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Wed, 2019-07-31 at 23:10 -0700, hpa@zytor.com wrote:
> On July 31, 2019 4:55:47 PM PDT, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> > On Wed, Jul 31, 2019 at 11:01 PM <hpa@zytor.com> wrote:
> > > The standard is moving toward adding this as an attribute with the
> > [[fallthrough]] syntax; it is in C++17, not sure when it will be in C
> > be if it isn't already.
> > 
> > Not yet, but it seems to be coming:
> > 
> >  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf
> > 
> > However, even if C2x gets it, it will be quite a while until the GCC
> > minimum version gets bumped up to that, so...
> > 
> > Cheers,
> > Miguel
> 
> The point was that we should plan ahead in whatever we end up doing.

Using a local keyword will allow any compiler to work successfully.

It's pretty simple to modify this to something like

#define fallback [[fallback]]

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Neil Horman]

On Wed, Jul 31, 2019 at 03:23:46PM -0700, Joe Perches wrote:
> On Wed, 2019-07-31 at 16:58 -0400, Neil Horman wrote:
> > On Wed, Jul 31, 2019 at 09:35:31AM -0700, Joe Perches wrote:
> > > On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> > > > On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> > > > > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> > > > > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> > > > > > > fallthrough may become a pseudo reserved keyword so this only use of
> > > > > > > fallthrough is better renamed to allow it.
> > > > > > > 
> > > > > > > Signed-off-by: Joe Perches <joe@perches.com>
> > > > > > Are you referring to the __attribute__((fallthrough)) statement that gcc
> > > > > > supports?  If so the compiler should by all rights be able to differentiate
> > > > > > between a null statement attribute and a explicit goto and label without the
> > > > > > need for renaming here.  Or are you referring to something else?
> > > > > 
> > > > > Hi.
> > > > > 
> > > > > I sent after this a patch that adds
> > > > > 
> > > > > # define fallthrough                    __attribute__((__fallthrough__))
> > > > > 
> > > > > https://lore.kernel.org/patchwork/patch/1108577/
> > > > > 
> > > > > So this rename is a prerequisite to adding this #define.
> > > > > 
> > > > why not just define __fallthrough instead, like we do for all the other
> > > > attributes we alias (i.e. __read_mostly, __protected_by, __unused, __exception,
> > > > etc)
> > > 
> > > Because it's not as intelligible when used as a statement.
> > I think thats somewhat debatable.  __fallthrough to me looks like an internal
> > macro, whereas fallthrough looks like a comment someone forgot to /* */
> 
> 
> I'd rather see:
> 
> 	switch (foo) {
> 	case FOO:
> 		bar |= baz;
> 		fallthrough;
> 	case BAR:
> 		bar |= qux;
> 		break;
> 	default:
> 		error();
> 	}
> 
> than
> 
> 	switch (foo) {
> 	case FOO:
> 		bar |= baz;
> 		__fallthrough;
> 	case BAR:
> 		bar |= qux;
> 		break;
> 	default:
> 		error();
> 	}
> 
> or esoecially
> 
> 	switch (foo) {
> 	case FOO:
> 		bar |= baz;
> 		/* fallthrough
> */;
> 	case BAR:
> 		bar |= qux;
> 		break;
> 	default:
> 		error();
> 	}
> 
> but <shrug>, bikeshed ahoy!...
You can say that if you want, but you made the point that your think the macro
as you have written is more readable.  You example illustrates though that /*
fallthrough */ is a pretty common comment, and not prefixing it makes it look
like someone didn't add a comment that they meant to.  The __ prefix is standard
practice for defining macros to attributes (212 instances of it by my count).  I
don't mind rewriting the goto labels at all, but I think consistency is
valuable.

Neil

> 
> 
> 

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Peter Zijlstra]

On Wed, Jul 31, 2019 at 11:10:36PM -0700, hpa@zytor.com wrote:
> On July 31, 2019 4:55:47 PM PDT, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> >On Wed, Jul 31, 2019 at 11:01 PM <hpa@zytor.com> wrote:
> >>
> >> The standard is moving toward adding this as an attribute with the
> >[[fallthrough]] syntax; it is in C++17, not sure when it will be in C
> >be if it isn't already.
> >
> >Not yet, but it seems to be coming:
> >
> >  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf
> >
> >However, even if C2x gets it, it will be quite a while until the GCC
> >minimum version gets bumped up to that, so...
> >
> >Cheers,
> >Miguel
> 
> The point was that we should plan ahead in whatever we end up doing.

By reserving 'fallthrough' as a keyword we do exactly that. We can then
define it to whatever the compiler/tool at hand requires.

Once GCC gains support for that [[attribute]] nonsense, we can detector
that and use that over the __attribute__(())

[ Also the Cxx attribute syntax is an abomination -- just a lesser one
than reading actual comments :-) ]

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Peter Zijlstra]

On Wed, Jul 31, 2019 at 01:02:07PM -0700, Kees Cook wrote:
> On Wed, Jul 31, 2019 at 08:48:32PM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 31, 2019 at 11:24:36AM -0700, hpa@zytor.com wrote:
> > > >> > +/*
> > > >> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> > > >> > + * must end with any of these keywords:
> > > >> > + *   break;
> > > >> > + *   fallthrough;
> > > >> > + *   goto <label>;
> > > >> > + *   return [expression];
> > > >> > + *
> > > >> > + *  gcc: >https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> > > >> > + */
> > > >> > +#if __has_attribute(__fallthrough__)
> > > >> > +# define fallthrough                   __attribute__((__fallthrough__))
> > > >> > +#else
> > > >> > +# define fallthrough                    do {} while (0)  /* fallthrough */
> > > >> > +#endif
> > > >> > +
> > 
> > > If the comments are stripped, how would the compiler see them to be
> > > able to issue a warning? I would guess that it is retained or replaced
> > > with some other magic token.
> > 
> > Everything that has the warning (GCC-7+/CLANG-9) has that attribute.
> 
> I'd like to make sure we don't regress Coverity most of all. If the
> recent updates to the Coverity scanner include support for the attribute
> now, then I'm all for it. :)

IMO Coverity can go pound sand, I never see its output, while I get to
look at the code and GCC output daily.

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

On Thu, 2019-08-01 at 06:50 -0400, Neil Horman wrote:
> On Wed, Jul 31, 2019 at 03:23:46PM -0700, Joe Perches wrote:
[]
> You can say that if you want, but you made the point that your think the macro
> as you have written is more readable.  You example illustrates though that /*
> fallthrough */ is a pretty common comment, and not prefixing it makes it look
> like someone didn't add a comment that they meant to.  The __ prefix is standard
> practice for defining macros to attributes (212 instances of it by my count).  I
> don't mind rewriting the goto labels at all, but I think consistency is
> valuable.

Hey Neil.

Perhaps you want to make this argument on the RFC patch thread
that introduces the fallthrough pseudo-keyword.

https://lore.kernel.org/patchwork/patch/1108577/

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[hpa]

On August 1, 2019 5:24:29 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>On Wed, Jul 31, 2019 at 11:10:36PM -0700, hpa@zytor.com wrote:
>> On July 31, 2019 4:55:47 PM PDT, Miguel Ojeda
><miguel.ojeda.sandonis@gmail.com> wrote:
>> >On Wed, Jul 31, 2019 at 11:01 PM <hpa@zytor.com> wrote:
>> >>
>> >> The standard is moving toward adding this as an attribute with the
>> >[[fallthrough]] syntax; it is in C++17, not sure when it will be in
>C
>> >be if it isn't already.
>> >
>> >Not yet, but it seems to be coming:
>> >
>> >  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf
>> >
>> >However, even if C2x gets it, it will be quite a while until the GCC
>> >minimum version gets bumped up to that, so...
>> >
>> >Cheers,
>> >Miguel
>> 
>> The point was that we should plan ahead in whatever we end up doing.
>
>By reserving 'fallthrough' as a keyword we do exactly that. We can then
>define it to whatever the compiler/tool at hand requires.
>
>Once GCC gains support for that [[attribute]] nonsense, we can detector
>that and use that over the __attribute__(())
>
>[ Also the Cxx attribute syntax is an abomination -- just a lesser one
>than reading actual comments :-) ]

I'm not disagreeing... I think using a macro makes sense.

Not sure if I agree about the syntax... I think it's rather friendly compared to gcc's ;)
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Miguel Ojeda]

On Thu, Aug 1, 2019 at 10:10 PM <hpa@zytor.com> wrote:
>
> I'm not disagreeing... I think using a macro makes sense.

It is either a macro or waiting for 5+ years (while we keep using the
comment style) :-)

In case it helps to make one's mind about whether to go for it or not,
I summarized the advantages and a few other details in the patch I
sent in October:

  https://github.com/ojeda/linux/commit/668f011a2706ea555987e263f609a5deba9c7fc4

It would be nice, however, to discuss whether we want __fallthrough or
fallthrough. The former is consistent with the rest of compiler
attributes and makes it clear it is not a keyword, the latter is
consistent with "break", "goto" and "return", as Joe's patch explains.

Cheers,
Miguel

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Thu, 2019-08-01 at 22:26 +0200, Miguel Ojeda wrote:
> On Thu, Aug 1, 2019 at 10:10 PM <hpa@zytor.com> wrote:
> > I'm not disagreeing... I think using a macro makes sense.
> 
> It is either a macro or waiting for 5+ years (while we keep using the
> comment style) :-)
> 
> In case it helps to make one's mind about whether to go for it or not,
> I summarized the advantages and a few other details in the patch I
> sent in October:
> 
>   https://github.com/ojeda/linux/commit/668f011a2706ea555987e263f609a5deba9c7fc4
> 
> It would be nice, however, to discuss whether we want __fallthrough or
> fallthrough.

I was happy enough with either though after a bit of
playing around with both, I think the pseudo-keyword
reads quite a bit better.

linux-kernel is a constrained code base and using
underscored prefixes is not really necessary.

For instance:

_Bool is c99, but the kernel uses bool
uint8_t also, but the kernel uses u8, etc...

Yes it requires a bit of 'local knowledge',
but simpler is better.

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Neil Horman]

On Thu, Aug 01, 2019 at 10:42:31AM -0700, Joe Perches wrote:
> On Thu, 2019-08-01 at 06:50 -0400, Neil Horman wrote:
> > On Wed, Jul 31, 2019 at 03:23:46PM -0700, Joe Perches wrote:
> []
> > You can say that if you want, but you made the point that your think the macro
> > as you have written is more readable.  You example illustrates though that /*
> > fallthrough */ is a pretty common comment, and not prefixing it makes it look
> > like someone didn't add a comment that they meant to.  The __ prefix is standard
> > practice for defining macros to attributes (212 instances of it by my count).  I
> > don't mind rewriting the goto labels at all, but I think consistency is
> > valuable.
> 
> Hey Neil.
> 
> Perhaps you want to make this argument on the RFC patch thread
> that introduces the fallthrough pseudo-keyword.
> 
> https://lore.kernel.org/patchwork/patch/1108577/
> 
> 
> 
Sure, but it will have to wait for tomorrow at this point, as I need to run to
an appointment.

Best
Neil

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Neil Horman]

On Thu, Aug 01, 2019 at 10:26:29PM +0200, Miguel Ojeda wrote:
> On Thu, Aug 1, 2019 at 10:10 PM <hpa@zytor.com> wrote:
> >
> > I'm not disagreeing... I think using a macro makes sense.
> 
> It is either a macro or waiting for 5+ years (while we keep using the
> comment style) :-)
> 
> In case it helps to make one's mind about whether to go for it or not,
> I summarized the advantages and a few other details in the patch I
> sent in October:
> 
>   https://github.com/ojeda/linux/commit/668f011a2706ea555987e263f609a5deba9c7fc4
> 
> It would be nice, however, to discuss whether we want __fallthrough or
> fallthrough. The former is consistent with the rest of compiler
> attributes and makes it clear it is not a keyword, the latter is
> consistent with "break", "goto" and "return", as Joe's patch explains.
> 
I was having this conversation with Joe, and I agree, I like the idea of
macroing up the fall through attribute, but naming it __fallthrough seems more
consistent to me with the other attribute macros.  I also feel like its more
recognizable as a macro.  Naming it fallthrough just makes it look like someone
forgot to put /**/'s around it to me.

Neil

> Cheers,
> Miguel
> 

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Fri 2019-08-02 07:00:42, Neil Horman wrote:
> On Thu, Aug 01, 2019 at 10:26:29PM +0200, Miguel Ojeda wrote:
> > On Thu, Aug 1, 2019 at 10:10 PM <hpa@zytor.com> wrote:
> > >
> > > I'm not disagreeing... I think using a macro makes sense.
> > 
> > It is either a macro or waiting for 5+ years (while we keep using the
> > comment style) :-)
> > 
> > In case it helps to make one's mind about whether to go for it or not,
> > I summarized the advantages and a few other details in the patch I
> > sent in October:
> > 
> >   https://github.com/ojeda/linux/commit/668f011a2706ea555987e263f609a5deba9c7fc4
> > 
> > It would be nice, however, to discuss whether we want __fallthrough or
> > fallthrough. The former is consistent with the rest of compiler
> > attributes and makes it clear it is not a keyword, the latter is
> > consistent with "break", "goto" and "return", as Joe's patch explains.
> > 
> I was having this conversation with Joe, and I agree, I like the idea of
> macroing up the fall through attribute, but naming it __fallthrough seems more
> consistent to me with the other attribute macros.  I also feel like its more
> recognizable as a macro.  Naming it fallthrough just makes it look like someone
> forgot to put /**/'s around it to me.

I like the "fallthrough". It looks like "return" and it should, no
need to have __'s there..
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Kees Cook]

On Fri, Aug 02, 2019 at 02:34:18PM +0200, Pavel Machek wrote:
> I like the "fallthrough". It looks like "return" and it should, no
> need to have __'s there..

Yeah, it would have the same feel as "break", "continue", "return"...

The only place I see this already used is in net/sctp/sm_make_chunk.c,
as a label, which would be trivial to adjust...

-- 
Kees Cook

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Fri, 2019-08-02 at 09:09 -0700, Kees Cook wrote:
> On Fri, Aug 02, 2019 at 02:34:18PM +0200, Pavel Machek wrote:
> > I like the "fallthrough". It looks like "return" and it should, no
> > need to have __'s there..
> 
> Yeah, it would have the same feel as "break", "continue", "return"...
> 
> The only place I see this already used is in net/sctp/sm_make_chunk.c,
> as a label, which would be trivial to adjust...

Yeah.

From the RFC patch:

For allow compilation of a kernel that includes net/sctp, this patch
also requires a rename of the use of fallthrough as a label in
net/sctp/sm_make_chunk.c that was submitted as:

https://lore.kernel.org/lkml/e0dd3af448e38e342c1ac6e7c0c802696eb77fd6.1564549413.git.joe@perches.com/

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> > > > fallthrough may become a pseudo reserved keyword so this only use of
> > > > fallthrough is better renamed to allow it.

Can you or any other maintainer apply this patch
or ack it so David Miller can apply it?

Thanks.

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Neil Horman]

On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> fallthrough may become a pseudo reserved keyword so this only use of
> fallthrough is better renamed to allow it.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  net/sctp/sm_make_chunk.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 36bd8a6e82df..3fdcaa2fbf12 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2152,7 +2152,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  	case SCTP_PARAM_SET_PRIMARY:
>  		if (net->sctp.addip_enable)
>  			break;
> -		goto fallthrough;
> +		goto unhandled;
>  
>  	case SCTP_PARAM_HOST_NAME_ADDRESS:
>  		/* Tell the peer, we won't support this param.  */
> @@ -2163,11 +2163,11 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  	case SCTP_PARAM_FWD_TSN_SUPPORT:
>  		if (ep->prsctp_enable)
>  			break;
> -		goto fallthrough;
> +		goto unhandled;
>  
>  	case SCTP_PARAM_RANDOM:
>  		if (!ep->auth_enable)
> -			goto fallthrough;
> +			goto unhandled;
>  
>  		/* SCTP-AUTH: Secion 6.1
>  		 * If the random number is not 32 byte long the association
> @@ -2184,7 +2184,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  
>  	case SCTP_PARAM_CHUNKS:
>  		if (!ep->auth_enable)
> -			goto fallthrough;
> +			goto unhandled;
>  
>  		/* SCTP-AUTH: Section 3.2
>  		 * The CHUNKS parameter MUST be included once in the INIT or
> @@ -2200,7 +2200,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  
>  	case SCTP_PARAM_HMAC_ALGO:
>  		if (!ep->auth_enable)
> -			goto fallthrough;
> +			goto unhandled;
>  
>  		hmacs = (struct sctp_hmac_algo_param *)param.p;
>  		n_elt = (ntohs(param.p->length) -
> @@ -2223,7 +2223,7 @@ static enum sctp_ierror sctp_verify_param(struct net *net,
>  			retval = SCTP_IERROR_ABORT;
>  		}
>  		break;
> -fallthrough:
> +unhandled:
>  	default:
>  		pr_debug("%s: unrecognized param:%d for chunk:%d\n",
>  			 __func__, ntohs(param.p->type), cid);
> -- 
> 2.15.0
> 
> 
Yeah, it seems reasonable to me, though I'm still not comfortable with defining
fallthrough as a macrotized keyword, but thats a debate for another thread

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 02 Aug 2019 10:47:34 -0700

> On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
>> On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
>> > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
>> > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
>> > > > fallthrough may become a pseudo reserved keyword so this only use of
>> > > > fallthrough is better renamed to allow it.
> 
> Can you or any other maintainer apply this patch
> or ack it so David Miller can apply it?

I, like others, don't like the lack of __ in the keyword.  It's kind of
rediculous the problems it creates to pollute the global namespace like
that and yes also inconsistent with other shorthands for builtins.

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

On Fri, 2019-08-02 at 16:19 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Fri, 02 Aug 2019 10:47:34 -0700
> 
> > On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> >> On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> >> > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> >> > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> >> > > > fallthrough may become a pseudo reserved keyword so this only use of
> >> > > > fallthrough is better renamed to allow it.
> > 
> > Can you or any other maintainer apply this patch
> > or ack it so David Miller can apply it?
> 
> I, like others, don't like the lack of __ in the keyword.  It's kind of
> rediculous the problems it creates to pollute the global namespace like
> that and yes also inconsistent with other shorthands for builtins.

Please add that to the conversation on the RFC thread.
https://lore.kernel.org/patchwork/patch/1108577/

In any case, fallthrough is not really a good label
name for this use.

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Joe Perches]

On Fri, 2019-08-02 at 16:19 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Fri, 02 Aug 2019 10:47:34 -0700
> 
> > On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> >> On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> >> > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> >> > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> >> > > > fallthrough may become a pseudo reserved keyword so this only use of
> >> > > > fallthrough is better renamed to allow it.
> > 
> > Can you or any other maintainer apply this patch
> > or ack it so David Miller can apply it?
> 
> I, like others, don't like the lack of __ in the keyword.  It's kind of
> rediculous the problems it creates to pollute the global namespace like
> that and yes also inconsistent with other shorthands for builtins.

Rejected?

I think that's inappropriate.

As coded, it's nothing like a fallthrough and
the rename to unhandled is more descriptive.

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Tue, 2019-07-30 at 22:35 -0700, Joe Perches wrote:
> Reserve the pseudo keyword 'fallthrough' for the ability to convert the
> various case block /* fallthrough */ style comments to appear to be an
> actual reserved word with the same gcc case block missing fallthrough
> warning capability.

Linus?  Do you have an opinion about this RFC/patch?

> All switch/case blocks now must end in one of:
> 
> 	break;
> 	fallthrough;
> 	goto <label>;
> 	return [expression];
> 
> fallthough is gcc's __attribute__((__fallthrough__)) which was introduced
> in gcc version 7..
> 
> fallthrough devolves to an empty "do {} while (0)" if the compiler version
> (any version less than gcc 7) does not support the attribute.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> 
> For allow compilation of a kernel that includes net/sctp, this patch
> also requires a rename of the use of fallthrough as a label in
> net/sctp/sm_make_chunk.c that was submitted as:
> 
> https://lore.kernel.org/lkml/e0dd3af448e38e342c1ac6e7c0c802696eb77fd6.1564549413.git.joe@perches.com/
> 
>  include/linux/compiler_attributes.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index 6b318efd8a74..cdf016596659 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -40,6 +40,7 @@
>  # define __GCC4_has_attribute___noclone__             1
>  # define __GCC4_has_attribute___nonstring__           0
>  # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)
> +# define __GCC4_has_attribute___fallthrough__         0
>  #endif
>  
>  /*
> @@ -185,6 +186,22 @@
>  # define __noclone
>  #endif
>  
> +/*
> + * Add the pseudo keyword 'fallthrough' so case statement blocks
> + * must end with any of these keywords:
> + *   break;
> + *   fallthrough;
> + *   goto <label>;
> + *   return [expression];
> + *
> + *  gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> + */
> +#if __has_attribute(__fallthrough__)
> +# define fallthrough                    __attribute__((__fallthrough__))
> +#else
> +# define fallthrough                    do {} while (0)  /* fallthrough */
> +#endif
> +
>  /*
>   * Note the missing underscores.
>   *

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Linus Torvalds]

On Sun, Aug 4, 2019 at 11:01 AM Joe Perches <joe@perches.com> wrote:
>
> Linus?  Do you have an opinion about this RFC/patch?

So my only real concern is that the comment approach has always been
the really traditional one, going back all the way to 'lint' days.

And you obviously cannot use a #define to create a comment, so this
whole keyword model will never be able to do that.

At the same time, all the modern tools we care about do seem to be
happy with it, either through the gcc attribute, the clang
[[clang:fallthrough]] or the (eventual) standard C [[fallthrough]]
model.

So I'm ok with just saying "the comment model may be traditional, but
it's not very good".

I didn't look at all the patches, but the one I *did* see had a few issues:

 - it didn't seem to handle clang

 - we'd need to make -Wimplicit-fallthrough be dependent on the
compiler actually supporting the attribute, not just on supporting the
flag.

without those changes, nobody can actually start doing any
conversions. But I assume such patches exist somewhere, and I've just
missed them.

               Linus

Re: [PATCH] net: sctp: Rename fallthrough label to unhandled

[Neil Horman]

On Fri, Aug 02, 2019 at 04:19:32PM -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Fri, 02 Aug 2019 10:47:34 -0700
> 
> > On Wed, 2019-07-31 at 08:16 -0400, Neil Horman wrote:
> >> On Wed, Jul 31, 2019 at 04:32:43AM -0700, Joe Perches wrote:
> >> > On Wed, 2019-07-31 at 07:19 -0400, Neil Horman wrote:
> >> > > On Tue, Jul 30, 2019 at 10:04:37PM -0700, Joe Perches wrote:
> >> > > > fallthrough may become a pseudo reserved keyword so this only use of
> >> > > > fallthrough is better renamed to allow it.
> > 
> > Can you or any other maintainer apply this patch
> > or ack it so David Miller can apply it?
> 
> I, like others, don't like the lack of __ in the keyword.  It's kind of
> rediculous the problems it creates to pollute the global namespace like
> that and yes also inconsistent with other shorthands for builtins.
> 
FWIW, I acked the sctp patch, because the use of the word fallthrough as a
label, isn't that important to me, unhendled is just as good, so I'm ok with
that change.

But, as I stated in the other thread, I agree, making a macro out of fallthrough
without clearly naming it using a macro convention like __ is not something I'm
ok with
Neil

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Miguel Ojeda]

On Sun, Aug 4, 2019 at 8:09 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So my only real concern is that the comment approach has always been
> the really traditional one, going back all the way to 'lint' days.
>
> And you obviously cannot use a #define to create a comment, so this
> whole keyword model will never be able to do that.
>
> At the same time, all the modern tools we care about do seem to be
> happy with it, either through the gcc attribute, the clang
> [[clang:fallthrough]] or the (eventual) standard C [[fallthrough]]
> model.

Quick note regarding C2x: fallthrough didn't make it yet into the
draft (other attributes already did), but it may still be added.

> So I'm ok with just saying "the comment model may be traditional, but
> it's not very good".
>
> I didn't look at all the patches, but the one I *did* see had a few issues:
>
>  - it didn't seem to handle clang

Hmm... Not sure what this refers to, but note that Clang will likely
get support in the next release (they are working on it at the
moment). As far as I understand, we do not yet establish a minimum
version for Clang since they still solving several issues, no?

>  - we'd need to make -Wimplicit-fallthrough be dependent on the
> compiler actually supporting the attribute, not just on supporting the
> flag.

If the above is correct (i.e. if we do not care about a specific
version of Clang just yet), and since gcc got support for the warning
at the same time as the attribute (7.1), I think we don't need to do
special handing for the warning; but if someone knows about a case
where we do have an issue with it either with GCC or Clang, please let
me know.

> without those changes, nobody can actually start doing any
> conversions. But I assume such patches exist somewhere, and I've just
> missed them.

I think the primary concern has always been the tooling (e.g. Coverity
was mentioned in October and also this time, and well as some
IDEs/text editors), not the compilers themselves.

Cheers,
Miguel

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Sun, 2019-08-04 at 11:09 -0700, Linus Torvalds wrote:
> On Sun, Aug 4, 2019 at 11:01 AM Joe Perches <joe@perches.com> wrote:
> > Linus?  Do you have an opinion about this RFC/patch?
> 
> So my only real concern is that the comment approach has always been
> the really traditional one, going back all the way to 'lint' days.
> 
> And you obviously cannot use a #define to create a comment, so this
> whole keyword model will never be able to do that.
> 
> At the same time, all the modern tools we care about do seem to be
> happy with it, either through the gcc attribute, the clang
> [[clang:fallthrough]] or the (eventual) standard C [[fallthrough]]
> model.

(adding Nick Desaulniers and clang-built-linux to cc's)

As far as I can tell, clang 10 (and it took hours to compile
and link the most current version here) does not support
	-Wimplicit-fallthrough=3
and using just -Wimplicit-fallthrough with clang 10 does not emit
a fallthrough warning even with -Wextra and -Wimplicit-fallthrough
using switch / case code blocks like:
---
 lib/test_module.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lib/test_module.c b/lib/test_module.c
index debd19e35198..30c835178c7c 100644
--- a/lib/test_module.c
+++ b/lib/test_module.c
@@ -14,6 +14,21 @@
 #include <linux/module.h>
 #include <linux/printk.h>
 
+static int switch_case(int val)
+{
+	int i = 0;
+
+	switch (val) {
+	case 1:
+		i |= 1;
+	case 2:
+		i |= 2;
+		break;
+	}
+
+	return i;
+}
+
 static int __init test_module_init(void)
 {
 	pr_warn("Hello, world\n");
---

Given:

$ clang -v
clang version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72)

and the compilation command line:
$ clang -Wp,-MD,lib/.test_module.o.d  -nostdinc -isystem /usr/local/lib/clang/10.0.0/include -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security -std=gnu89 -no-integrated-as -Werror=unknown-warning-option -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -m64 -mno-80387 -mstack-alignment=8 -mtune=generic -mno-red-zone -mcmodel=kernel -DCONFIG_X86_X32_ABI -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -DCONFIG_AS_SSSE3=1 -DCONFIG_AS_AVX=1 -DCONFIG_AS_AVX2=1 -DCONFIG_AS_AVX512=1 -DCONFIG_AS_SHA1_NI=1 -DCONFIG_AS_SHA256_NI=1 -Wno-sign-compare -fno-a
 synchronous-unwind-tables -mretpoline-external-thunk -fno-delete-null-pointer-checks -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-format-invalid-specifier -Wno-gnu -Wno-tautological-compare -mno-global-merge -Wno-unused-const-variable -DCC_USING_FENTRY -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -fno-strict-overflow -fno-merge-all-constants -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -fcf-protection=none -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length     -fsanitize=kernel-address -mllvm -asan-mapping-offset=0xdffffc0000000000  -mllvm -asan-globals=1  -mllvm -asan-instrumentation-with-call-threshold=0  -mllvm -asan-stack=0   --param asan-instrument-allocas=1   -fsanitize-coverage=trace-pc -fsanitize-coverage=trace-cmp  -DMODULE  -DKBUILD_BASENAME='"test_module"' -DKBUILD_MODNAME='"test_module"' -Wextra -Wimplicit-fallthrough -c -o lib/test_module.o lib/test_module.c

> So I'm ok with just saying "the comment model may be traditional, but
> it's not very good".
> 
> I didn't look at all the patches, but the one I *did* see had a few issues:
> 
>  - it didn't seem to handle clang

The __has_attribute use is at least clang compatible.
https://releases.llvm.org/3.7.0/tools/clang/docs/LanguageExtensions.html
even if it doesn't (seem to?) work.

>  - we'd need to make -Wimplicit-fallthrough be dependent on the
> compiler actually supporting the attribute, not just on supporting the
> flag.

I believe that also needs work if ever clang works,

Makefile:KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,)

this will have to be changed for clang as the =<val> isn't (yet?) supported.

> without those changes, nobody can actually start doing any
> conversions. But I assume such patches exist somewhere, and I've just
> missed them.

I haven't sent any patches for any comment conversions.
nor would I until the RFC is acceptable.

Just this RFC and the necessary conversion of the one use
of fallthrough as a label (which David Miller did not apply)

Some people reasonably feel that Coverity should recognize
fallthrough; style annotations before changing the existing
/* fallthrough */ comment uses.  I think lint doesn't matter
much.

I do have a script that does a reasonable job of converting
most of the /* fallthrough */ style comments to fallthrough;
while realigning to the last indentation.

That script still needs more work before I will post it.

Lastly:

I think using the pseudo-keyword
	fallthrough;
reads better than
	__fallthrough;
to end case blocks.

Do you have an opinion here?

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Nathan Chancellor]

Hi Joe,

On Sun, Aug 04, 2019 at 05:39:28PM -0700, Joe Perches wrote:
> On Sun, 2019-08-04 at 11:09 -0700, Linus Torvalds wrote:
> > On Sun, Aug 4, 2019 at 11:01 AM Joe Perches <joe@perches.com> wrote:
> > > Linus?  Do you have an opinion about this RFC/patch?
> > 
> > So my only real concern is that the comment approach has always been
> > the really traditional one, going back all the way to 'lint' days.
> > 
> > And you obviously cannot use a #define to create a comment, so this
> > whole keyword model will never be able to do that.
> > 
> > At the same time, all the modern tools we care about do seem to be
> > happy with it, either through the gcc attribute, the clang
> > [[clang:fallthrough]] or the (eventual) standard C [[fallthrough]]
> > model.
> 
> (adding Nick Desaulniers and clang-built-linux to cc's)

Thanks for adding us.

> As far as I can tell, clang 10 (and it took hours to compile
> and link the most current version here) does not support

Just a heads up in case you want to mess around with clang in the
future, I wrote a toolchain build script for ClangBuiltLinux to help
with the long compile times by cutting as much cruft as possible (and
it is self contained by default, won't install anything outside of the
repository).

https://github.com/ClangBuiltLinux/tc-build

The slimmest working configuration for testing what you did would probably
be from the following command:

./build-llvm.py --build-stage1-only --projects clang --targets X86

> 	-Wimplicit-fallthrough=3
> and using just -Wimplicit-fallthrough with clang 10 does not emit
> a fallthrough warning even with -Wextra and -Wimplicit-fallthrough
> using switch / case code blocks like:

Unfortunately, -Wimplicit-fallthrough does not work for C right now
(only C++), as pointed out by Nick on LLVM's bug tracker.

https://bugs.llvm.org/show_bug.cgi?id=39382

This patch resolves that while adding support for the attribute.

https://reviews.llvm.org/D64838

Your example properly works when that patch is applied and
-Wimplicit-fallthrough is added to the list of flags.

../lib/test_module.c:24:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
        case 2:
        ^
../lib/test_module.c:24:2: note: insert '__attribute__((fallthrough));' to silence this warning
        case 2:
        ^
        __attribute__((fallthrough)); 
../lib/test_module.c:24:2: note: insert 'break;' to avoid fall-through
        case 2:
        ^
        break; 

Hopefully it can get merged soon. I am sure Nathan or Nick can speak
to further progress on that.

> The __has_attribute use is at least clang compatible.
> https://releases.llvm.org/3.7.0/tools/clang/docs/LanguageExtensions.html
> even if it doesn't (seem to?) work.

I was trying to follow along with this thread through the web interface
and kind of got lost, how does it not work? If I apply your compiler
attributes patch with D64838, I see fallthrough get expanded to
__attribute__((__fallthrough__)) by the preprocessor.

> >  - we'd need to make -Wimplicit-fallthrough be dependent on the
> > compiler actually supporting the attribute, not just on supporting the
> > flag.
> 
> I believe that also needs work if ever clang works,
> 
> Makefile:KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,)
> 
> this will have to be changed for clang as the =<val> isn't (yet?) supported.

GCC's documentation says that -Wimplicit-fallthrough is equivalent to
-Wimplicit-fallthrough=3 so it seems like just making that change would
be all that is needed to support Clang:

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-fallthrough

Cheers,
Nathan

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Joe Perches]

On Sun, 2019-08-04 at 18:18 -0700, Nathan Chancellor wrote:
> On Sun, Aug 04, 2019 at 05:39:28PM -0700, Joe Perches wrote:
> Hi Joe,

Hi Nathan.

> This patch resolves that while adding support for the attribute.
> https://reviews.llvm.org/D64838
[]
> > The __has_attribute use is at least clang compatible.
> > https://releases.llvm.org/3.7.0/tools/clang/docs/LanguageExtensions.html
> > even if it doesn't (seem to?) work.
> 
> I was trying to follow along with this thread through the web interface
> and kind of got lost, how does it not work?

It does not work in llvm/clang mainline through
commit 305b961f64b75e73110e309341535f6d5a48ed72.

> If I apply your compiler attributes patch with D64838,
> I see fallthrough get expanded to
> __attribute__((__fallthrough__)) by the preprocessor.

Well, great.
I hope D64838 or something like it gets applied soon.
But all I could and did test was the current version.

cheers, Joe

RE: [PATCH] net: sctp: Rename fallthrough label to unhandled

[David Laight]

From: Joe Perches
> Sent: 01 August 2019 18:43
> On Thu, 2019-08-01 at 06:50 -0400, Neil Horman wrote:
> > On Wed, Jul 31, 2019 at 03:23:46PM -0700, Joe Perches wrote:
> []
> > You can say that if you want, but you made the point that your think the macro
> > as you have written is more readable.  You example illustrates though that /*
> > fallthrough */ is a pretty common comment, and not prefixing it makes it look
> > like someone didn't add a comment that they meant to.  The __ prefix is standard
> > practice for defining macros to attributes (212 instances of it by my count).  I
> > don't mind rewriting the goto labels at all, but I think consistency is
> > valuable.
> 
> Hey Neil.
> 
> Perhaps you want to make this argument on the RFC patch thread
> that introduces the fallthrough pseudo-keyword.
> 
> https://lore.kernel.org/patchwork/patch/1108577/

ISTM that the only place where you need something other than the
traditional comment is inside a #define where (almost certainly)
the comments have to get stripped too early.

Adding a 'fallthough' as unknown C keyword sucks...


	David

 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Kees Cook]

On Wed, Jul 31, 2019 at 01:02:07PM -0700, Kees Cook wrote:
> On Wed, Jul 31, 2019 at 08:48:32PM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 31, 2019 at 11:24:36AM -0700, hpa@zytor.com wrote:
> > > >> > +/*
> > > >> > + * Add the pseudo keyword 'fallthrough' so case statement blocks
> > > >> > + * must end with any of these keywords:
> > > >> > + *   break;
> > > >> > + *   fallthrough;
> > > >> > + *   goto <label>;
> > > >> > + *   return [expression];
> > > >> > + *
> > > >> > + *  gcc: >https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
> > > >> > + */
> > > >> > +#if __has_attribute(__fallthrough__)
> > > >> > +# define fallthrough                   __attribute__((__fallthrough__))
> > > >> > +#else
> > > >> > +# define fallthrough                    do {} while (0)  /* fallthrough */
> > > >> > +#endif
> > > >> > +
> > 
> > > If the comments are stripped, how would the compiler see them to be
> > > able to issue a warning? I would guess that it is retained or replaced
> > > with some other magic token.
> > 
> > Everything that has the warning (GCC-7+/CLANG-9) has that attribute.
> 
> I'd like to make sure we don't regress Coverity most of all. If the
> recent updates to the Coverity scanner include support for the attribute
> now, then I'm all for it. :)

I want to recant my position on Coverity coverage being a requirement
here. While I was originally concerned about suddenly adding thousands
more warnings to Coverity scans (if it doesn't support the flag --
I should know soon), it's been made clear to me we're now at the point
where this is about to happen for Clang instead (since _it_ doesn't
support the comment-style marking and never will but is about to gain
C support[1] for the detection -- it only had C++ before).

With that out of the way, yes, let's do a mass conversion. As mentioned
before, I think "fallthrough;" should be used here (to match "break;").
Let's fork the C language. :)

-Kees

[1] https://reviews.llvm.org/D64838

-- 
Kees Cook

Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Kees Cook]

On Thu, Aug 15, 2019 at 11:15:53AM -0700, Kees Cook wrote:
> I want to recant my position on Coverity coverage being a requirement
> here. While I was originally concerned about suddenly adding thousands
> more warnings to Coverity scans (if it doesn't support the flag --
> I should know soon), it's been made clear to me we're now at the point

For the record, Coverity *does*[1] support the attribute flag.

-Kees

[1] This should be visible with a Coverity account:
    https://scan3.coverity.com/reports.htm#v39370/p12360/fileInstanceId=27181923&defectInstanceId=7915635&mergedDefectId=220491

-- 
Kees Cook

treewide replacement of fallthrough comments with "fallthrough" macro (was Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use)

[Kees Cook]

On Thu, Aug 15, 2019 at 11:15:53AM -0700, Kees Cook wrote:
> With that out of the way, yes, let's do a mass conversion. As mentioned
> before, I think "fallthrough;" should be used here (to match "break;").
> Let's fork the C language. :)

FWIW, last week I asked Linus at the maintainer's summit which he
preferred ("__fallthrough" or "fallthrough") and he said "fallthrough".

Joe, if you've still got the series ready, do you want to send it for
this merge window before -rc1 gets cut?

-- 
Kees Cook

Re: treewide replacement of fallthrough comments with "fallthrough" macro (was Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use)

[Joe Perches]

On Mon, 2019-09-16 at 15:19 -0700, Kees Cook wrote:
> On Thu, Aug 15, 2019 at 11:15:53AM -0700, Kees Cook wrote:
> > With that out of the way, yes, let's do a mass conversion. As mentioned
> > before, I think "fallthrough;" should be used here (to match "break;").
> > Let's fork the C language. :)
> 
> FWIW, last week I asked Linus at the maintainer's summit which he
> preferred ("__fallthrough" or "fallthrough") and he said "fallthrough".

Nice.  I think that's better style/taste.

> Joe, if you've still got the series ready, do you want to send it for
> this merge window before -rc1 gets cut?

The first bits that add fallthrough and converts the one existing
use of fallthrough as a label can be sent now.  I'll do that in
a few days as I'm not able to do that right now.

Sending any actual comment conversion patches before -rc1 might
require changes in already queued up and tested patches

I do think a scripted conversion by major subsystem/directory
might be a better mechanism than individual patches, especially
if applied directly before releasing what will be -rc1.

Re: treewide replacement of fallthrough comments with "fallthrough" macro (was Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use)

[Kees Cook]

On Tue, Sep 17, 2019 at 03:26:32PM -0700, Joe Perches wrote:
> On Mon, 2019-09-16 at 15:19 -0700, Kees Cook wrote:
> > On Thu, Aug 15, 2019 at 11:15:53AM -0700, Kees Cook wrote:
> > > With that out of the way, yes, let's do a mass conversion. As mentioned
> > > before, I think "fallthrough;" should be used here (to match "break;").
> > > Let's fork the C language. :)
> > 
> > FWIW, last week I asked Linus at the maintainer's summit which he
> > preferred ("__fallthrough" or "fallthrough") and he said "fallthrough".
> 
> Nice.  I think that's better style/taste.
> 
> > Joe, if you've still got the series ready, do you want to send it for
> > this merge window before -rc1 gets cut?
> 
> The first bits that add fallthrough and converts the one existing
> use of fallthrough as a label can be sent now.  I'll do that in
> a few days as I'm not able to do that right now.

Okay, sounds good.

> Sending any actual comment conversion patches before -rc1 might
> require changes in already queued up and tested patches
> 
> I do think a scripted conversion by major subsystem/directory
> might be a better mechanism than individual patches, especially
> if applied directly before releasing what will be -rc1.

Yeah, as long as we've got the infrastructure in place, the scripted
conversion can land whenever is easiest.

Thanks!

-- 
Kees Cook