On Sun, Oct 21, 2018 at 10:14 AM Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> wrote: > > From the GCC manual: > > fallthrough > > The fallthrough attribute with a null statement serves as a > fallthrough statement. It hints to the compiler that a statement > that falls through to another case label, or user-defined label > in a switch statement is intentional and thus the -Wimplicit-fallthrough > warning must not trigger. The fallthrough attribute may appear > at most once in each attribute list, and may not be mixed with > other attributes. It can only be used in a switch statement > (the compiler will issue an error otherwise), after a preceding > statement and before a logically succeeding case label, > or user-defined label. > > https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html > > Currently, most of the kernel uses fallthrough comments to silence > the -Wimplicit-fallthrough warnings (enabled by -Wextra, in W=1). > However, C++17 standarized an "statement attribute" (the first s/standarized/standardized/ > of its kind) to deal with this: [[fallthrough]] is meant to be > a new control keyword in the form of an extension. I think we can leave the details about the [[]] notation out. IIUC, it's only applicable to C++. > > In C mode, GCC supports the __fallthrough__ attribute since 7.1, > the same time the warning and the comment parsing were introduced. > > While comment parsing is a good idea to deal with old codebases > that used such a comment as documentation for humans, the best > solution is to use the attribute: > > * It is a "real" part of the AST (=> better for tooling). +1 > > * It does not follow arbitrary rules for parsing (e.g. regexps > for the comment parsing). +2 > > * It may even become standarized in C as well: there are ongoing s/standarized/standardized/ > proposals to import some C++ standard attributes into > the C standard, e.g. for fallthrough: > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf > > On top of that, it is also a better solution for the kernel, because: > > * We can actually use a #define for it like for the rest of > attributes/extensions, which is not possible with a comment, > so that its naming/usage is consistent across the entire kernel. +3 > > * Whenever the migration from the comments to the attribute > is complete, we may increase the level of the GCC warning up to 5, > i.e. comments will not longer be considered for warning > surpression: only the attribute must be used. This would enforce s/surpression/suppression/ > consistency by leveraging the compiler directly (instead of > enforcing it with other tools). > > * Further into the future, we can consider moving the warning > up to W=0 or even making it an error. > > It is worth noting that clang >= 3.2 supports the warning and > the attribute, but only in C++ mode (and it is not enabled by > -Wall/-Wextra/-Wpedantic like in gcc). Hopefully, they will also > support it for C as well. https://bugs.llvm.org/show_bug.cgi?id=39382 > > Further, icc >= 18 does not seem to know anything about the warning; > except that it accepts (i.e. ignores) [[fallthrough]] in C++17 mode > (to be conformant, probably). > > Link: https://lore.kernel.org/lkml/20181017062255.oiu44y4zuuwilan3@mwanda/ > Suggested-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> > --- > include/linux/compiler_attributes.h | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h > index 6b28c1b7310c..9e2153f85462 100644 > --- a/include/linux/compiler_attributes.h > +++ b/include/linux/compiler_attributes.h > @@ -32,6 +32,7 @@ > # define __GCC4_has_attribute___assume_aligned__ (__GNUC_MINOR__ >= 9) > # define __GCC4_has_attribute___designated_init__ 0 > # define __GCC4_has_attribute___externally_visible__ 1 > +# define __GCC4_has_attribute___fallthrough__ 0 > # define __GCC4_has_attribute___noclone__ 1 > # define __GCC4_has_attribute___optimize__ 1 > # define __GCC4_has_attribute___nonstring__ 0 > @@ -133,6 +134,23 @@ > # define __visible > #endif > > +/* > + * Currently, most of the kernel uses fallthrough comments to silence > + * the -Wimplicit-fallthrough warnings (enabled by -Wextra, in W=1). > + * For new instances, please use this attribute instead. > + * > + * Optional: only supported since gcc >= 7.1 > + * Optional: not supported by clang > + * Optional: not supported by icc > + * > + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#index-fallthrough-statement-attribute > + */ > +#if __has_attribute(__fallthrough__) > +# define __fallthrough __attribute__((__fallthrough__)) > +#else > +# define __fallthrough While this is in the correct format as the other attributes in this file, I think this particular attribute is a special case, because of the variety of fallbacks and differing support for them. I'd like to see in the commit message maybe a list of tools we'd like to support and links to the feature requests/bug reports for them. I acknowledge it's more work to file bugs, but it's being a good open source citizen, IMO. I'm also curious which is the first version of GCC to support the comment format? > +#endif > + > /* > * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-format-function-attribute > * clang: https://clang.llvm.org/docs/AttributeReference.html#format > -- > 2.17.1 > -- Thanks, ~Nick Desaulniers
