These are two patches are meant to go on top of the rest of the compiler
attributes series on:
https://github.com/ojeda/linux/tree/compiler-attributes
which will be sent to Greg for the next merge window.
Please review them and let me know! (specially if someone is against
__fallthrough for some reason :-).
The first patch introduces the attribute and gives the rationale.
The second patch is an example of usage.
This was started in the following thread:
https://lore.kernel.org/lkml/20181017062255.oiu44y4zuuwilan3@mwanda/
Cheers,
Miguel
Cc: Dan Carpenter <[email protected]>
Cc: Andreas Dilger <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Konstantin Ryabitsev <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Paul Lawrence <[email protected]>
Cc: Sandipan Das <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Philippe Ombredanne <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Willy Tarreau <[email protected]>
Cc: Martin Sebor <[email protected]>
Cc: Christopher Li <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Theodore Ts'o <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Dominique Martinet <[email protected]>
Cc: Stefan Agner <[email protected]>
Cc: Luc Van Oostenryck <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Miguel Ojeda (2):
Compiler Attributes: add support for __fallthrough (gcc >= 7.1)
Compiler Attributes: auxdisplay: panel: use __fallthrough
drivers/auxdisplay/panel.c | 6 +++---
include/linux/compiler_attributes.h | 18 ++++++++++++++++++
2 files changed, 21 insertions(+), 3 deletions(-)
--
2.17.1
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 <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
---
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
+#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
Let gcc know these cases are meant to fall through to the next label
by annotating them with the new __fallthrough statement attribute;
and remove the comment since it conveys the same information
(which was also parsed by gcc to suppress the warning).
Signed-off-by: Miguel Ojeda <[email protected]>
---
drivers/auxdisplay/panel.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index 21b9b2f2470a..0755034e49ba 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -1367,7 +1367,7 @@ static void panel_process_inputs(void)
break;
input->rise_timer = 0;
input->state = INPUT_ST_RISING;
- /* fall through */
+ __fallthrough;
case INPUT_ST_RISING:
if ((phys_curr & input->mask) != input->value) {
input->state = INPUT_ST_LOW;
@@ -1380,11 +1380,11 @@ static void panel_process_inputs(void)
}
input->high_timer = 0;
input->state = INPUT_ST_HIGH;
- /* fall through */
+ __fallthrough;
case INPUT_ST_HIGH:
if (input_state_high(input))
break;
- /* fall through */
+ __fallthrough;
case INPUT_ST_FALLING:
input_state_falling(input);
}
--
2.17.1
On Sun, Oct 21, 2018 at 07:14:12PM +0200, Miguel Ojeda wrote:
> These are two patches are meant to go on top of the rest of the compiler
> attributes series on:
>
> https://github.com/ojeda/linux/tree/compiler-attributes
>
> which will be sent to Greg for the next merge window.
>
> Please review them and let me know! (specially if someone is against
> __fallthrough for some reason :-).
Will this work with all of the static tools that are currently looking
for the comment instead? I know coverity handles that, what about
others?
thanks,
greg k-h
On Sun, 2018-10-21 at 19:14 +0200, Miguel Ojeda wrote:
> Let gcc know these cases are meant to fall through to the next label
> by annotating them with the new __fallthrough statement attribute;
> and remove the comment since it conveys the same information
> (which was also parsed by gcc to suppress the warning).
Instead of many individual conversion patches,
perhaps a script to do all the conversions at once.
Maybe:
pattern='(?:\/\*\s*fall(?:\s*|\s*\-\s*)thr(?:u|ough)\s*\*\/|\/\/\s*fall\s*thr(?:u|ough)\s*$)'
git grep -P -i --name-only "$pattern" -- '*.[ch]' |
xargs perl -p -i -e "s/$pattern/__fallthrough;/gi"
On Sun, 2018-10-21 at 19:29 +0100, Greg Kroah-Hartman wrote:i
> On Sun, Oct 21, 2018 at 07:14:12PM +0200, Miguel Ojeda wrote:
> > These are two patches are meant to go on top of the rest of the compiler
> > attributes series on:
> >
> > https://github.com/ojeda/linux/tree/compiler-attributes
> >
> > which will be sent to Greg for the next merge window.
> >
> > Please review them and let me know! (specially if someone is against
> > __fallthrough for some reason :-).
>
> Will this work with all of the static tools that are currently looking
> for the comment instead?
Does anyone have a list of the static tools that
use comment style fallthrough notations?
> I know coverity handles that, what about others?
No doubt tool updates would be useful.
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 11:52:21AM -0700, Joe Perches wrote:
> On Sun, 2018-10-21 at 19:29 +0100, Greg Kroah-Hartman wrote:i
> > On Sun, Oct 21, 2018 at 07:14:12PM +0200, Miguel Ojeda wrote:
> > > These are two patches are meant to go on top of the rest of the compiler
> > > attributes series on:
> > >
> > > https://github.com/ojeda/linux/tree/compiler-attributes
> > >
> > > which will be sent to Greg for the next merge window.
> > >
> > > Please review them and let me know! (specially if someone is against
> > > __fallthrough for some reason :-).
> >
> > Will this work with all of the static tools that are currently looking
> > for the comment instead?
>
> Does anyone have a list of the static tools that
> use comment style fallthrough notations?
>
It would only be CPPcheck I think.
regards,
dan carpenter
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
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 11:36 AM Kees Cook <[email protected]> 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 Mon, Oct 22, 2018 at 11:41 AM Miguel Ojeda
<[email protected]> 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 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
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at
On Sun, Oct 21, 2018 at 8:29 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Sun, Oct 21, 2018 at 07:14:12PM +0200, Miguel Ojeda wrote:
> > These are two patches are meant to go on top of the rest of the compiler
> > attributes series on:
> >
> > https://github.com/ojeda/linux/tree/compiler-attributes
> >
> > which will be sent to Greg for the next merge window.
> >
> > Please review them and let me know! (specially if someone is against
> > __fallthrough for some reason :-).
>
> Will this work with all of the static tools that are currently looking
> for the comment instead? I know coverity handles that, what about
> others?
Thank you Greg, good question. I will try to keep all the information
we can get about the tools in the commit message.
I will also contact the different tools about this.
Cheers,
Miguel
On Sun, Oct 21, 2018 at 8:11 PM Joe Perches <[email protected]> wrote:
>
> On Sun, 2018-10-21 at 19:14 +0200, Miguel Ojeda wrote:
> > Let gcc know these cases are meant to fall through to the next label
> > by annotating them with the new __fallthrough statement attribute;
> > and remove the comment since it conveys the same information
> > (which was also parsed by gcc to suppress the warning).
>
> Instead of many individual conversion patches,
> perhaps a script to do all the conversions at once.
Note that this was only an example of the attribute (some people asked
for an example when introducing another one, so I preemptively did it
for this one).
Doing the conversion (and how :-) I left it for afterwards, if people
agree with the attribute.
>
> Maybe:
>
> pattern='(?:\/\*\s*fall(?:\s*|\s*\-\s*)thr(?:u|ough)\s*\*\/|\/\/\s*fall\s*thr(?:u|ough)\s*$)'
> git grep -P -i --name-only "$pattern" -- '*.[ch]' |
> xargs perl -p -i -e "s/$pattern/__fallthrough;/gi"
By the way, I checked first if coccinelle could match input comments,
but it doesn't, according to Julia. I am also thinking whether a
compiler plugin could easily do this, but I don't have my hopes high
given these are comments...
Also, regardless of how it is done, the patches need to be sent
individually to maintainers, no? I have a vague memory that big &
automated conversions were a bit frozen upon in the kernel. Greg...?
Cheers,
Miguel
On Mon, Oct 22, 2018 at 12:27 AM Theodore Y. Ts'o <[email protected]> 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 <[email protected]> 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
<[email protected]> wrote:
> On Mon, Oct 22, 2018 at 12:27 AM Theodore Y. Ts'o <[email protected]> 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
--
Kees Cook
Pixel Security
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
--
"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 2:41 AM, Miguel Ojeda
<[email protected]> wrote:
> On Mon, Oct 22, 2018 at 11:36 AM Kees Cook <[email protected]> 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
--
Kees Cook
Pixel Security
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 <[email protected]> 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 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 12:53 PM Kees Cook <[email protected]> wrote:
>
> On Mon, Oct 22, 2018 at 2:41 AM, Miguel Ojeda
> <[email protected]> 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 Mon, Oct 22, 2018 at 2:07 PM Luc Van Oostenryck
<[email protected]> 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
<[email protected]> wrote:
>
> On Mon, Oct 22, 2018 at 12:53 PM Kees Cook <[email protected]> wrote:
> >
> > On Mon, Oct 22, 2018 at 2:41 AM, Miguel Ojeda
> > <[email protected]> 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 Sun, Oct 21, 2018 at 10:14 AM, Miguel Ojeda
<[email protected]> wrote:
> Let gcc know these cases are meant to fall through to the next label
> by annotating them with the new __fallthrough statement attribute;
> and remove the comment since it conveys the same information
> (which was also parsed by gcc to suppress the warning).
>
> Signed-off-by: Miguel Ojeda <[email protected]>
> ---
> drivers/auxdisplay/panel.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
> index 21b9b2f2470a..0755034e49ba 100644
> --- a/drivers/auxdisplay/panel.c
> +++ b/drivers/auxdisplay/panel.c
> @@ -1367,7 +1367,7 @@ static void panel_process_inputs(void)
> break;
> input->rise_timer = 0;
> input->state = INPUT_ST_RISING;
> - /* fall through */
> + __fallthrough;
> case INPUT_ST_RISING:
> if ((phys_curr & input->mask) != input->value) {
> input->state = INPUT_ST_LOW;
> @@ -1380,11 +1380,11 @@ static void panel_process_inputs(void)
> }
> input->high_timer = 0;
> input->state = INPUT_ST_HIGH;
> - /* fall through */
> + __fallthrough;
> case INPUT_ST_HIGH:
> if (input_state_high(input))
> break;
> - /* fall through */
> + __fallthrough;
> case INPUT_ST_FALLING:
> input_state_falling(input);
> }
> --
> 2.17.1
>
I would prefer we continue to use the comment style until we've got
confirmed support for (at least) Clang, Coverity, CPPcheck, smatch,
and eclipse.
-Kees
--
Kees Cook
Pixel Security
On Mon, 2018-10-22 at 11:51 +0200, Miguel Ojeda wrote:
> On Sun, Oct 21, 2018 at 8:11 PM Joe Perches <[email protected]> wrote:
> > On Sun, 2018-10-21 at 19:14 +0200, Miguel Ojeda wrote:
> > > Let gcc know these cases are meant to fall through to the next label
> > > by annotating them with the new __fallthrough statement attribute;
> > > and remove the comment since it conveys the same information
> > > (which was also parsed by gcc to suppress the warning).
> >
> > Instead of many individual conversion patches,
> > perhaps a script to do all the conversions at once.
>
> Note that this was only an example of the attribute (some people asked
> for an example when introducing another one, so I preemptively did it
> for this one).
>
> Doing the conversion (and how :-) I left it for afterwards, if people
> agree with the attribute.
>
> > Maybe:
> >
> > pattern='(?:\/\*\s*fall(?:\s*|\s*\-\s*)thr(?:u|ough)\s*\*\/|\/\/\s*fall\s*thr(?:u|ough)\s*$)'
> > git grep -P -i --name-only "$pattern" -- '*.[ch]' |
> > xargs perl -p -i -e "s/$pattern/__fallthrough;/gi"
>
> By the way, I checked first if coccinelle could match input comments,
> but it doesn't, according to Julia. I am also thinking whether a
> compiler plugin could easily do this, but I don't have my hopes high
> given these are comments...
>
> Also, regardless of how it is done, the patches need to be sent
> individually to maintainers, no?
Not really no. For instance the spdx license treewide change.
commit b24413180f5600bcb3bb70fbed5cf186b60864bd
> I have a vague memory that big &
> automated conversions were a bit frozen upon in the kernel. Greg...?
There really needs to be some method to do treewide
conversions without involving multiple maintainers.
It'd be a good topic for a maintainer summit one day.
On Mon, Oct 22, 2018 at 2:43 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Mon, Oct 22, 2018 at 11:41 AM Miguel Ojeda
> <[email protected]> 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.
--
Thanks,
~Nick Desaulniers
On Mon, Oct 22, 2018 at 2:48 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Sun, Oct 21, 2018 at 8:29 PM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > Will this work with all of the static tools that are currently looking
> > for the comment instead? I know coverity handles that, what about
> > others?
>
> I will also contact the different tools about this.
Let's contact the authors of these tools if they don't parse the
attribute. I prefer to have the attributes rather than specifically
formatted comments.
I do think this may be tricky to provide backwards support for though;
Miguel, do you have info on which versions of GCC support comments vs
attribute?
--
Thanks,
~Nick Desaulniers
On Mon, Oct 22, 2018 at 2:26 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Mon, Oct 22, 2018 at 12:27 AM Theodore Y. Ts'o <[email protected]> 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
--
Thanks,
~Nick Desaulniers
On Mon, Oct 22, 2018 at 3:54 AM Dan Carpenter <[email protected]> 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
>
--
Thanks,
~Nick Desaulniers
On Sun, Oct 21, 2018 at 10:14 AM Miguel Ojeda
<[email protected]> 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 <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
> ---
> 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
Hi all!
On 22/10/18 13:07, Miguel Ojeda wrote:
> On Mon, Oct 22, 2018 at 12:54 PM Dan Carpenter <[email protected]> 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).
--
"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 10:50 AM Bernd Petrovitsch
<[email protected]> wrote:
>
> Hi all!
>
> On 22/10/18 13:07, Miguel Ojeda wrote:
> > On Mon, Oct 22, 2018 at 12:54 PM Dan Carpenter <[email protected]> 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
--
Thanks,
~Nick Desaulniers
Hi all!
On 22/10/18 19:54, Nick Desaulniers wrote:
> On Mon, Oct 22, 2018 at 10:50 AM Bernd Petrovitsch
> <[email protected]> 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
--
"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 11:15 AM Bernd Petrovitsch
<[email protected]> wrote:
>
> Hi all!
>
> On 22/10/18 19:54, Nick Desaulniers wrote:
> > On Mon, Oct 22, 2018 at 10:50 AM Bernd Petrovitsch
> > <[email protected]> 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
--
Thanks,
~Nick Desaulniers
On Mon, Oct 22, 2018 at 7:36 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Sun, Oct 21, 2018 at 10:14 AM Miguel Ojeda
> <[email protected]> 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 6:54 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Mon, Oct 22, 2018 at 2:48 AM Miguel Ojeda
> <[email protected]> wrote:
> >
> > On Sun, Oct 21, 2018 at 8:29 PM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > Will this work with all of the static tools that are currently looking
> > > for the comment instead? I know coverity handles that, what about
> > > others?
> >
> > I will also contact the different tools about this.
>
> Let's contact the authors of these tools if they don't parse the
> attribute. I prefer to have the attributes rather than specifically
> formatted comments.
Sorry, not sure what you mean -- isn't that what I said? Greg was
asking whether tools would support the attribute equally well compared
to the comment parsing; not the comments.
>
> I do think this may be tricky to provide backwards support for though;
> Miguel, do you have info on which versions of GCC support comments vs
> attribute?
It is in the commit message:
"""
In C mode, GCC supports the __fallthrough__ attribute since 7.1,
the same time the warning and the comment parsing were introduced.
"""
Cheers,
Miguel
On Mon, Oct 22, 2018 at 7:50 PM Bernd Petrovitsch
<[email protected]> wrote:
>
> Hi all!
>
> On 22/10/18 13:07, Miguel Ojeda wrote:
> > On Mon, Oct 22, 2018 at 12:54 PM Dan Carpenter <[email protected]> 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
On Mon, Oct 22, 2018 at 7:17 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Mon, Oct 22, 2018 at 2:43 AM Miguel Ojeda
> <[email protected]> wrote:
> >
> > On Mon, Oct 22, 2018 at 11:41 AM Miguel Ojeda
> > <[email protected]> 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 09:54:27AM -0700, Nick Desaulniers wrote:
> On Mon, Oct 22, 2018 at 2:48 AM Miguel Ojeda
> <[email protected]> wrote:
> >
> > On Sun, Oct 21, 2018 at 8:29 PM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > Will this work with all of the static tools that are currently looking
> > > for the comment instead? I know coverity handles that, what about
> > > others?
> >
> > I will also contact the different tools about this.
>
> Let's contact the authors of these tools if they don't parse the
> attribute. I prefer to have the attributes rather than specifically
> formatted comments.
>
> I do think this may be tricky to provide backwards support for though;
> Miguel, do you have info on which versions of GCC support comments vs
> attribute?
None. GCC 7.1 added support for the warning, the comment parsing and
the attribute so it's fine.
The only thing that we know for sure is an issue is Eclipse.
We need to test Coverity but it should work in theory. And we don't
know about CPPcheck.
regards,
dan carpenter
Hi Dan,
On Tue, Oct 23, 2018 at 7:37 AM Dan Carpenter <[email protected]> wrote:
>
> On Mon, Oct 22, 2018 at 07:10:02AM -0700, Kees Cook wrote:
> > I would prefer we continue to use the comment style until we've got
> > confirmed support for (at least) Clang, Coverity, CPPcheck, smatch,
> > and eclipse.
>
> Clang and Smatch don't support fall throught comments. Coverity
> supports both. CPPcheck is unknown.
>
> Eclipse doesn't support attributes. So it's just Eclipse and maybe
> CPP check.
Thanks for checking! Let's wait then a few months and see if we can
get cppcheck/Eclipse to support it.
Cheers,
Miguel
On Fri, Nov 2, 2018 at 11:49 AM Miguel Ojeda
<[email protected]> wrote:
>
> Thanks for checking! Let's wait then a few months and see if we can
> get cppcheck/Eclipse to support it.
In the meantime, saved here too:
https://github.com/ojeda/linux/tree/compiler-attributes-fallthrough
rebased on top of e468f5c06b5e ("Merge tag
'compiler-attributes-for-linus-4.20-rc1' of
https://github.com/ojeda/linux").
Cheers,
Miguel