| Message ID | 20181021171414.22674-2-miguel.ojeda.sandonis@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Compiler Attributes: __fallthrough | expand |
On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda 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 Do we know if coverity understands the fallthrough attribute? One of the reasons why I started using /* fallthrough */ is because it kept Coverity happy. If the conversion from /* fallthrough */ to the __fallthrough__ attribute means that we start gethting a lot of Coverity warnings, that would be unfortunate. OTOH, if this is getting standardized, maybe we can get Coverity to understand this attribute? - Ted
On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda wrote: > +#if __has_attribute(__fallthrough__) > +# define __fallthrough __attribute__((__fallthrough__)) > +#else > +# define __fallthrough > +#endif Why is the #else not: # define __fallthrough /* fallthrough */ Would this solve the Coverity problem, or does Coverity look at the raw source code before preprocessing?
On Sun, Oct 21, 2018 at 05:42:18PM -0700, Matthew Wilcox wrote: > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda wrote: > > +#if __has_attribute(__fallthrough__) > > +# define __fallthrough __attribute__((__fallthrough__)) > > +#else > > +# define __fallthrough > > +#endif > > Why is the #else not: > > # define __fallthrough /* fallthrough */ > > Would this solve the Coverity problem, or does Coverity look at the raw > source code before preprocessing? Wouldn't the "/* ... */" be eaten by the preprocessor before defining the __fallthrough cpp macro? (e.g., try running the attached script) - Ted #!/bin/bash cat << EOF > /tmp/test$$.c #define foobar quux /* foobar */ foobar EOF gcc -E /tmp/test$$.c rm -f /tmp/test$$.c
On Mon, Oct 22, 2018 at 02:58:00AM -0400, Theodore Y. Ts'o wrote: > On Sun, Oct 21, 2018 at 05:42:18PM -0700, Matthew Wilcox wrote: > > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda wrote: > > > +#if __has_attribute(__fallthrough__) > > > +# define __fallthrough __attribute__((__fallthrough__)) > > > +#else > > > +# define __fallthrough > > > +#endif > > > > Why is the #else not: > > > > # define __fallthrough /* fallthrough */ > > > > Would this solve the Coverity problem, or does Coverity look at the raw > > source code before preprocessing? > > Wouldn't the "/* ... */" be eaten by the preprocessor before defining > the __fallthrough cpp macro? (e.g., try running the attached script) You're right, even on older versions (4.7 here) : $ echo -e '#define foobar quux /* foobar */\nfoobar\n' | gcc -E - # 1 "<stdin>" # 1 "<command-line>" # 1 "<stdin>" quux Willy
On Mon, Oct 22, 2018 at 12:27 AM Theodore Y. Ts'o <tytso@mit.edu> wrote: > > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda 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 > > Do we know if coverity understands the fallthrough attribute? One of > the reasons why I started using /* fallthrough */ is because it kept > Coverity happy. If Coverity is like gcc, they should be doing both (i.e. I see the comment parsing as an "extra" that gcc did, but the "basic stuff" is the attribute -- and I would guess it is way easier for them to support than the comment parsing). But I cannot test it myself :-( Someone, please? However, if I understood Greg correctly in his reply to the cover letter, he replied that Coverity knows about it (?). > > If the conversion from /* fallthrough */ to the __fallthrough__ > attribute means that we start gethting a lot of Coverity warnings, > that would be unfortunate. OTOH, if this is getting standardized, > maybe we can get Coverity to understand this attribute? Indeed! That would be the best for everyone, including Coverity customers. Cheers, Miguel
On Mon, Oct 22, 2018 at 2:42 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda wrote: > > +#if __has_attribute(__fallthrough__) > > +# define __fallthrough __attribute__((__fallthrough__)) > > +#else > > +# define __fallthrough > > +#endif > > Why is the #else not: > > # define __fallthrough /* fallthrough */ > > Would this solve the Coverity problem, or does Coverity look at the raw > source code before preprocessing? That wouldn't work if Coverity follows the standard, because it is required that comments are removed right before the preprocessing phase. That is one of the advantages vs. the attribute that I mentioned: """ We can actually use a #define for it like for the rest of attributes/extensions, which is not possible with a comment, (...) """ Cheers, Miguel
On Mon, Oct 22, 2018 at 2:26 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Mon, Oct 22, 2018 at 12:27 AM Theodore Y. Ts'o <tytso@mit.edu> wrote: >> >> On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda 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 Please CC Gustavo on these kinds of things -- he's been driving the bulk of the fall through coverage. >> Do we know if coverity understands the fallthrough attribute? One of >> the reasons why I started using /* fallthrough */ is because it kept >> Coverity happy. > > If Coverity is like gcc, they should be doing both (i.e. I see the > comment parsing as an "extra" that gcc did, but the "basic stuff" is > the attribute -- and I would guess it is way easier for them to > support than the comment parsing). > > But I cannot test it myself :-( Someone, please? > > However, if I understood Greg correctly in his reply to the cover > letter, he replied that Coverity knows about it (?). > >> >> If the conversion from /* fallthrough */ to the __fallthrough__ >> attribute means that we start gethting a lot of Coverity warnings, >> that would be unfortunate. OTOH, if this is getting standardized, >> maybe we can get Coverity to understand this attribute? > > Indeed! That would be the best for everyone, including Coverity customers. We need to make sure the static analyzers are happy with either method. Additionally, when was -Wimplicit-fallthrough added to GCC? If it was added _before_ the attribute, we need to continue using the comment style otherwise we lose coverage even with gcc itself. Additionally, does Clang support this attribute (it supports -Wimplicit-fallthrough). -Kees
On Mon, Oct 22, 2018 at 11:36 AM Kees Cook <keescook@chromium.org> wrote: > > We need to make sure the static analyzers are happy with either > method. Additionally, when was -Wimplicit-fallthrough added to GCC? If > it was added _before_ the attribute, we need to continue using the > comment style otherwise we lose coverage even with gcc itself. > Additionally, does Clang support this attribute (it supports > -Wimplicit-fallthrough). Please take a look at the rationale (also more details at the linked thread): * gcc 7.1 added -Wimplicit-fallthrough at the same time as the attribute and the comment parsing. * clang does *not* support the attribute in C. Cheers, Miguel
On 22/10/2018 00:27, Theodore Y. Ts'o wrote: > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda 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 > > Do we know if coverity understands the fallthrough attribute? One of > the reasons why I started using /* fallthrough */ is because it kept > Coverity happy. FWIW, current "eclipse" has the same "problem". > If the conversion from /* fallthrough */ to the __fallthrough__ > attribute means that we start gethting a lot of Coverity warnings, We could keep both. MfG, Bernd
On Mon, Oct 22, 2018 at 11:41 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > * clang does *not* support the attribute in C. ...nor the warning at all, by the way (which is why this is OK). Cheers, Miguel
On Mon, Oct 22, 2018 at 11:41:56AM +0200, Bernd Petrovitsch wrote: > On 22/10/2018 00:27, Theodore Y. Ts'o wrote: > > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda 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 > > > > Do we know if coverity understands the fallthrough attribute? One of > > the reasons why I started using /* fallthrough */ is because it kept > > Coverity happy. > > FWIW, current "eclipse" has the same "problem". > > > If the conversion from /* fallthrough */ to the __fallthrough__ > > attribute means that we start gethting a lot of Coverity warnings, > > We could keep both. What does that even mean? Use both the attribute and the comment until Eclipse is updated? case 3: frob(); __fall_through; /* fall through */ case 4: That seems like a wrong idea... regards, dan carpenter
On 22/10/18 12:27, Dan Carpenter wrote: [...] > What does that even mean? Use both the attribute and the comment until > Eclipse is updated? > > case 3: > frob(); > __fall_through; /* fall through */ > case 4: > > That seems like a wrong idea... It's more like ---- snip ---- case 3: frob(); __fall_through; /* no break - fall through */ case 4: ---- snip ---- as "eclipse" doesn't accept anything else. And yes, it's far from "beautiful" but I hadn't time to dig into eclipses innards to fix that. MfG, Bernd
On Mon, Oct 22, 2018 at 2:41 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Mon, Oct 22, 2018 at 11:36 AM Kees Cook <keescook@chromium.org> wrote: >> >> We need to make sure the static analyzers are happy with either >> method. Additionally, when was -Wimplicit-fallthrough added to GCC? If >> it was added _before_ the attribute, we need to continue using the >> comment style otherwise we lose coverage even with gcc itself. >> Additionally, does Clang support this attribute (it supports >> -Wimplicit-fallthrough). > > Please take a look at the rationale (also more details at the linked thread): > > * gcc 7.1 added -Wimplicit-fallthrough at the same time as the > attribute and the comment parsing. Ah, perfect. I missed this. :) > * clang does *not* support the attribute in C. Well that's not good. :) -Kees
On Mon, Oct 22, 2018 at 12:45:03PM +0200, Bernd Petrovitsch wrote: > It's more like > ---- snip ---- > case 3: > frob(); > __fall_through; > /* no break - fall through */ > case 4: > ---- snip ---- > as "eclipse" doesn't accept anything else. > > And yes, it's far from "beautiful" but I hadn't time to dig into > eclipses innards to fix that. > Doing both is super ugly. Let's just do comments until Eclipse gets updated. I had wanted to move to the attribute because that would simplify things in Smatch but it's not a huge deal to delay for another year. regards, dan carpenter
On Mon, Oct 22, 2018 at 12:54 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Doing both is super ugly. Let's just do comments until Eclipse gets > updated. > > I had wanted to move to the attribute because that would simplify things > in Smatch but it's not a huge deal to delay for another year. I can re-send them later on, no problem. On the other hand, doing the changes will push tools to get updated sooner ;-) If tools were doing something as fancy as comment parsing for diagnostics, they should have been updated with the attribute support (either gcc's or C++17's) -- it has been more than a year now since gcc 7.1 and the C++17 final draft. (Note that this does not apply for things like clang, since they weren't doing comment parsing to begin with.) Cheers, Miguel
On Mon, Oct 22, 2018 at 12:53 PM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Oct 22, 2018 at 2:41 AM, Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > Please take a look at the rationale (also more details at the linked thread): > > > > * gcc 7.1 added -Wimplicit-fallthrough at the same time as the > > attribute and the comment parsing. > > Ah, perfect. I missed this. :) No problem! I know the commit message is a bit too long, so I understand :) > > > * clang does *not* support the attribute in C. > > Well that's not good. :) I will see with clang if they plan to add it. (By the way, if the "*not*" sounded rude, sorry; I wanted to emphasize it is surprising that it doesn't -- I also assumed the opposite until I checked it). Cheers, Miguel
On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda wrote: > > 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). This will create a problem for the recent versions of sparse which support __has_attribute() because sparse falsely pretends to support this attribute while, to play nice with -Wdeclaration-after-statement, it needs some adaptation to the parsing (it's actually seen not as a statement but as a declaration). I'll see to fix it this evening. Regards, -- Luc
On Mon, Oct 22, 2018 at 2:07 PM Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda wrote: > > > > 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). > > This will create a problem for the recent versions of sparse which > support __has_attribute() because sparse falsely pretends to support > this attribute while, to play nice with -Wdeclaration-after-statement, > it needs some adaptation to the parsing (it's actually seen not as a > statement but as a declaration). I'll see to fix it this evening. No rush Luc! (I have sent the PR to Linus without this two patches for the moment). And thanks a lot for having being so quick to improve sparse to support all these series! Cheers, Miguel
On Mon, Oct 22, 2018 at 1:24 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Mon, Oct 22, 2018 at 12:53 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Mon, Oct 22, 2018 at 2:41 AM, Miguel Ojeda > > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > > * clang does *not* support the attribute in C. > > > > Well that's not good. :) > > I will see with clang if they plan to add it. > So I have looked in the clang sources and I have seen that clang is already preparing for C2x: since clang >= 6 we can actually enable the C++-like attributes with "-fdouble-square-bracket-attributes" in C mode, which in turn makes the warning work in C mode and can be suppressed with [[fallthrough]]. This would give us the warning also in clang, which would be a win vs. the comments. Nick? However, I don't see a way to enable [[fallthrough]] in C mode for gcc >= 7.1 -- from a quick look, the C parser does not know about [[]] attributes, so I don't think we can use [[fallthrough]] for both compilers (yet). Cheers, Miguel
On Mon, Oct 22, 2018 at 2:43 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Mon, Oct 22, 2018 at 11:41 AM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > * clang does *not* support the attribute in C. > > ...nor the warning at all, by the way (which is why this is OK). > > Cheers, > Miguel Oh? We're going through and annotating all of Android's C++ source right now with it. https://clang.llvm.org/docs/DiagnosticsReference.html#wimplicit-fallthrough Though it looks like the attribute syntax I like from GCC is not yet supported in Clang. https://bugs.llvm.org/show_bug.cgi?id=37135 https://github.com/ClangBuiltLinux/linux/issues/235 I'll take a look at adding support. So Kees sent me this embarrassing example: https://godbolt.org/z/gV_c9_ (try changing the language from C++ to C; it works)! That's embarrassing! https://bugs.llvm.org/show_bug.cgi?id=39382 https://github.com/ClangBuiltLinux/linux/issues/236 Will get these both fixed up.
On Mon, Oct 22, 2018 at 2:26 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Mon, Oct 22, 2018 at 12:27 AM Theodore Y. Ts'o <tytso@mit.edu> wrote: > > > > On Sun, Oct 21, 2018 at 07:14:13PM +0200, Miguel Ojeda 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 > > > > Do we know if coverity understands the fallthrough attribute? One of > > the reasons why I started using /* fallthrough */ is because it kept > > Coverity happy. > > If Coverity is like gcc, they should be doing both (i.e. I see the > comment parsing as an "extra" that gcc did, but the "basic stuff" is > the attribute -- and I would guess it is way easier for them to > support than the comment parsing). > > But I cannot test it myself :-( Someone, please? + Colin, who has been running Coverity on the kernel, and sending patches. > > However, if I understood Greg correctly in his reply to the cover > letter, he replied that Coverity knows about it (?). > > > > > If the conversion from /* fallthrough */ to the __fallthrough__ > > attribute means that we start gethting a lot of Coverity warnings, > > that would be unfortunate. OTOH, if this is getting standardized, > > maybe we can get Coverity to understand this attribute? > > Indeed! That would be the best for everyone, including Coverity customers. > > Cheers, > Miguel
On Mon, Oct 22, 2018 at 3:54 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Mon, Oct 22, 2018 at 12:45:03PM +0200, Bernd Petrovitsch wrote: > > It's more like > > ---- snip ---- > > case 3: > > frob(); > > __fall_through; > > /* no break - fall through */ > > case 4: > > ---- snip ---- > > as "eclipse" doesn't accept anything else. > > > > And yes, it's far from "beautiful" but I hadn't time to dig into > > eclipses innards to fix that. > > > > Doing both is super ugly. Let's just do comments until Eclipse gets > updated. Eclipse as in the IDE? https://bugs.eclipse.org/bugs/ > > I had wanted to move to the attribute because that would simplify things > in Smatch but it's not a huge deal to delay for another year. > > regards, > dan carpenter >
On Sun, Oct 21, 2018 at 10:14 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> 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@oracle.com> > Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> > --- > 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 >
Hi all! On 22/10/18 13:07, Miguel Ojeda wrote: > On Mon, Oct 22, 2018 at 12:54 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: >> >> Doing both is super ugly. Let's just do comments until Eclipse gets >> updated. Yes, "Eclipse" as the IDE. And yes but IMHO better super ugly than loosing the warning - YMMV. For the archives: I have Eclipse Photon/June 2016 here. And "no break" is the (default) string in a comment used by Eclipse (it can be customized and is actually a regexp but it must be in a comment). >> I had wanted to move to the attribute because that would simplify things >> in Smatch but it's not a huge deal to delay for another year. > > I can re-send them later on, no problem. On the other hand, doing the > changes will push tools to get updated sooner ;-) > > If tools were doing something as fancy as comment parsing for > diagnostics, they should have been updated with the attribute support > (either gcc's or C++17's) -- it has been more than a year now since > gcc 7.1 and the C++17 final draft. (Note that this does not apply for > things like clang, since they weren't doing comment parsing to begin > with.) That would be nice. And if they agree on the same texts (or accept per default all somewhat widely used and/or old ones). After stumbling over https://stackoverflow.com/questions/16935935/how-do-i-turn-off-a-static-code-analysis-warning-on-a-line-by-line-warning-in-cd, looking into Eclipses Window -> Preferences -> C/C++ -> Code Analysis -> "No break at the end of case" screen (that's the screenshot there) and I tried various things: Preface: I have ---- snip ---- #define __fallthrough __attribute__((fallthrough)) ---- snip ---- for gcc >= 7 (because clang doesn't know it and I had also older gcc's in use before). So: - Adding a comment to the #define doesn't change anything for Eclipse. - Eclipse looks *only* in comments for the string/regexp given the warnings configuration (and that comment must be on the line directly before the "case"). - Eclipse understands [[fallthrough]] out-of-the-box though (which is C++11 AFAIK) as does g++-7 (I use -std=gnu++17 - most of the sources are C++, but not all) and clang++-6 (all the current standard Ubuntu-18.06/Bionic packages). Eclipse "accepts" [[fallthrough]] only in C++ sources (and not in C sources). - Neither gcc nor clang understand [[fallthrough]] (so it's probably a no-go for the Kernel with C89 anyways). MfG, Bernd PS: clang++ errors with "fallthrough annotation in unreachable code" if [[fallthrough]] is after an assert(). clang-devs there, please, the fallthrough doesn't really generated code (I hope;-). I have lots of switch()es which catch undefined values (for enums et. al.) with "default"+assert() and fall through to the most safe case (for the deployed version).
On Mon, Oct 22, 2018 at 10:50 AM Bernd Petrovitsch <bernd@petrovitsch.priv.at> wrote: > > Hi all! > > On 22/10/18 13:07, Miguel Ojeda wrote: > > On Mon, Oct 22, 2018 at 12:54 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > >> > >> Doing both is super ugly. Let's just do comments until Eclipse gets > >> updated. > > Yes, "Eclipse" as the IDE. > > And yes but IMHO better super ugly than loosing the warning - YMMV. > > For the archives: I have Eclipse Photon/June 2016 here. And "no break" > is the (default) string in a comment used by Eclipse (it can be > customized and is actually a regexp but it must be in a comment). > > >> I had wanted to move to the attribute because that would simplify things > >> in Smatch but it's not a huge deal to delay for another year. > > > > I can re-send them later on, no problem. On the other hand, doing the > > changes will push tools to get updated sooner ;-) > > > > If tools were doing something as fancy as comment parsing for > > diagnostics, they should have been updated with the attribute support > > (either gcc's or C++17's) -- it has been more than a year now since > > gcc 7.1 and the C++17 final draft. (Note that this does not apply for > > things like clang, since they weren't doing comment parsing to begin > > with.) > > That would be nice. And if they agree on the same texts (or accept per > default all somewhat widely used and/or old ones). > > After stumbling over > https://stackoverflow.com/questions/16935935/how-do-i-turn-off-a-static-code-analysis-warning-on-a-line-by-line-warning-in-cd, > looking into Eclipses Window -> Preferences -> C/C++ -> Code Analysis -> > "No break at the end of case" screen (that's the screenshot there) and I > tried various things: > > Preface: > I have > ---- snip ---- > #define __fallthrough __attribute__((fallthrough)) > ---- snip ---- > for gcc >= 7 (because clang doesn't know it and I had also older > gcc's in use before). > > So: > - Adding a comment to the #define doesn't change anything for Eclipse. > - Eclipse looks *only* in comments for the string/regexp given > the warnings configuration (and that comment must be on the line > directly before the "case"). > - Eclipse understands [[fallthrough]] out-of-the-box though (which > is C++11 AFAIK) as does g++-7 (I use -std=gnu++17 - most of the > sources are C++, but not all) and clang++-6 (all the current standard > Ubuntu-18.06/Bionic packages). > Eclipse "accepts" [[fallthrough]] only in C++ sources (and not in C > sources). > - Neither gcc nor clang understand [[fallthrough]] (so it's probably a > no-go for the Kernel with C89 anyways). > > MfG, > Bernd > > PS: clang++ errors with "fallthrough annotation in unreachable code" if > [[fallthrough]] is after an assert(). clang-devs there, please, the > fallthrough doesn't really generated code (I hope;-). > I have lots of switch()es which catch undefined values (for enums > et. al.) with "default"+assert() and fall through to the most safe > case (for the deployed version). Can you send me a link to a simple reproducer in godbolt (godbolt.org) and we'll take a look? > -- > "I dislike type abstraction if it has no real reason. And saving > on typing is not a good reason - if your typing speed is the main > issue when you're coding, you're doing something seriously wrong." > - Linus Torvalds
Hi all! On 22/10/18 19:54, Nick Desaulniers wrote: > On Mon, Oct 22, 2018 at 10:50 AM Bernd Petrovitsch > <bernd@petrovitsch.priv.at> wrote: [...] >> PS: clang++ errors with "fallthrough annotation in unreachable code" if >> [[fallthrough]] is after an assert(). clang-devs there, please, the >> fallthrough doesn't really generated code (I hope;-). [...] > Can you send me a link to a simple reproducer in godbolt (godbolt.org) > and we'll take a look? Does https://godbolt.org/z/2Y4zIo do it - I'm a godbolt-newbie? For ---- snip ---- #include <cassert> int main(void) { switch (1) { default: assert(0); [[fallthrough]]; case 1: ; } return 0; } ---- snip ---- Just "clang++ -Wimplicit-fallthrough -Werror" it ..... MfG, Bernd
On Mon, Oct 22, 2018 at 11:15 AM Bernd Petrovitsch <bernd@petrovitsch.priv.at> wrote: > > Hi all! > > On 22/10/18 19:54, Nick Desaulniers wrote: > > On Mon, Oct 22, 2018 at 10:50 AM Bernd Petrovitsch > > <bernd@petrovitsch.priv.at> wrote: > [...] > >> PS: clang++ errors with "fallthrough annotation in unreachable code" if > >> [[fallthrough]] is after an assert(). clang-devs there, please, the > >> fallthrough doesn't really generated code (I hope;-). > [...] > > Can you send me a link to a simple reproducer in godbolt (godbolt.org) > > and we'll take a look? > > Does https://godbolt.org/z/2Y4zIo do it - I'm a godbolt-newbie? Moving the kernel folks to bcc, since we don't need to be discussing C++ on LKML. https://godbolt.org/z/B1fo9Z shows that this works as intended, for cases that cannot be statically proven. I guess I'm looking for a more realistic code sample to show why putting a `break;` statement there is untenable? > > For > ---- snip ---- > #include <cassert> > > int main(void) > { > switch (1) { > default: > assert(0); > [[fallthrough]]; > case 1: > ; > } > return 0; > } > ---- snip ---- > Just "clang++ -Wimplicit-fallthrough -Werror" it ..... > > MfG, > Bernd > -- > "I dislike type abstraction if it has no real reason. And saving > on typing is not a good reason - if your typing speed is the main > issue when you're coding, you're doing something seriously wrong." > - Linus Torvalds
On Mon, Oct 22, 2018 at 7:17 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Mon, Oct 22, 2018 at 2:43 AM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > On Mon, Oct 22, 2018 at 11:41 AM Miguel Ojeda > > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > > > * clang does *not* support the attribute in C. > > > > ...nor the warning at all, by the way (which is why this is OK). > > > > Cheers, > > Miguel > > Oh? We're going through and annotating all of Android's C++ source > right now with it. > https://clang.llvm.org/docs/DiagnosticsReference.html#wimplicit-fallthrough > Sure, but I am talking about the C mode only. Please read the previous messages in the thread :-) > Though it looks like the attribute syntax I like from GCC is not yet > supported in Clang. Indeed, that is what I explained in the last message. > https://bugs.llvm.org/show_bug.cgi?id=37135 > https://github.com/ClangBuiltLinux/linux/issues/235 > I'll take a look at adding support. > > So Kees sent me this embarrassing example: > https://godbolt.org/z/gV_c9_ > (try changing the language from C++ to C; it works)! That's embarrassing! Yup, that is what I have been testing yesterday -- see the linked thread in the commit message for the summary of the results. > https://bugs.llvm.org/show_bug.cgi?id=39382 > https://github.com/ClangBuiltLinux/linux/issues/236 > Will get these both fixed up. Cheers, Miguel
On Mon, Oct 22, 2018 at 7:36 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Sun, Oct 21, 2018 at 10:14 AM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > 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++. No, because C++ is the driving force for the standard attributes syntax; i.e. C2x is adding them because of the syntax published by C++17; and possibly gcc 7.1 added support for fallthrough (and comment parsing) due to C++17 too. Basically, it is a small paragraph explaining how this came to be. > > +#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 No, is it the correct format because we cannot add support for the other syntax in gcc; so the best way to proceed is to simply wait until clang supports the GNU attribute in C mode. The tooling, of course, is another matter and independent of this. > see in the commit message maybe a list of tools we'd like to support Yes, I already said I would write it in one of the other threads. > 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. Who said we aren't going to do it? :-) I was actually in the process of checking which OSS tools supported what and how easy it was to fix in each of them (gcc's [[...]] syntax, clang's GNU and C++ attrs in C mode, cppcheck's fallthrough support...), but it takes time; I prefer to do the research beforehand; so that the submitted bug reports are better/more precise/more helpful, etc. However, you already sent the LLVM report (without mentioning this thread or me, nor the -fdouble-square-bracket-attributes clang flag that I mentioned). That is a bit rude :-) Please take things a little easier, there is no need to rush stuff. If I didn't have submitted the LLVM bug report is because I hadn't finish looking at the issue. In general, I think it is polite (and also more productive to avoid duplicating efforts) to first ask whoever is working on something before you rush to do it... > > I'm also curious which is the first version of GCC to support the > comment format? gcc 7.1 started everything. It is stated in the commit message and several messages/threads already. Again, please pause, relax and read a bit before sending stuff around :-) Cheers, Miguel
On Mon, Oct 22, 2018 at 7:50 PM Bernd Petrovitsch <bernd@petrovitsch.priv.at> wrote: > > Hi all! > > On 22/10/18 13:07, Miguel Ojeda wrote: > > On Mon, Oct 22, 2018 at 12:54 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > >> > >> Doing both is super ugly. Let's just do comments until Eclipse gets > >> updated. > > Yes, "Eclipse" as the IDE. > > And yes but IMHO better super ugly than loosing the warning - YMMV. I think Dan meant too simply not touch anything (i.e. not losing the warning anywhere). > > For the archives: I have Eclipse Photon/June 2016 here. And "no break" > is the (default) string in a comment used by Eclipse (it can be > customized and is actually a regexp but it must be in a comment). > Hm... that means they don't support (by default) the same regexps as GCC; which is bad: it means that it is only equivalent to the most relaxed level in gcc, 1: """ -Wimplicit-fallthrough=1 matches .* regular expression, any comment is used as fallthrough comment. """ See https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html i.e. any other level above will either not match Eclipse or not match gcc. > >> I had wanted to move to the attribute because that would simplify things > >> in Smatch but it's not a huge deal to delay for another year. > > > > I can re-send them later on, no problem. On the other hand, doing the > > changes will push tools to get updated sooner ;-) > > > > If tools were doing something as fancy as comment parsing for > > diagnostics, they should have been updated with the attribute support > > (either gcc's or C++17's) -- it has been more than a year now since > > gcc 7.1 and the C++17 final draft. (Note that this does not apply for > > things like clang, since they weren't doing comment parsing to begin > > with.) > > That would be nice. And if they agree on the same texts (or accept per > default all somewhat widely used and/or old ones). > > After stumbling over > https://stackoverflow.com/questions/16935935/how-do-i-turn-off-a-static-code-analysis-warning-on-a-line-by-line-warning-in-cd, > looking into Eclipses Window -> Preferences -> C/C++ -> Code Analysis -> > "No break at the end of case" screen (that's the screenshot there) and I > tried various things: > > Preface: > I have > ---- snip ---- > #define __fallthrough __attribute__((fallthrough)) > ---- snip ---- > for gcc >= 7 (because clang doesn't know it and I had also older > gcc's in use before). > > So: > - Adding a comment to the #define doesn't change anything for Eclipse. It shouldn't according to the standard -- but who knows... :-) > - Eclipse looks *only* in comments for the string/regexp given > the warnings configuration (and that comment must be on the line > directly before the "case"). > - Eclipse understands [[fallthrough]] out-of-the-box though (which > is C++11 AFAIK) as does g++-7 (I use -std=gnu++17 - most of the > sources are C++, but not all) and clang++-6 (all the current standard > Ubuntu-18.06/Bionic packages). Eclipse understanding [[fallthrough]] is very good, actually. > Eclipse "accepts" [[fallthrough]] only in C++ sources (and not in C > sources). Bad, but I guess they will add it to C eventually, since it is probably coming for C2x. > - Neither gcc nor clang understand [[fallthrough]] (so it's probably a > no-go for the Kernel with C89 anyways). clang does it if you enable -fdouble-square-bracket-attributes (please see my other messages). gcc will at some point (if C2x gets the attributes), but at the moment the C parser is different than the C++ parser and there is no support for it on trunk that I could see, so they will have to copy the support; i.e. it will take a bit more time than clang, likely. Thanks a *lot* for taking a look at Eclipse! Cheers, Miguel
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 +#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
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 of its kind) to deal with this: [[fallthrough]] is meant to be a new control keyword in the form of an extension. 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). * It does not follow arbitrary rules for parsing (e.g. regexps for the comment parsing). * It may even become standarized in C as well: there are ongoing 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. * 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 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. 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@oracle.com> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> --- include/linux/compiler_attributes.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)