In preparation to enable -Wimplicit-fallthrough by default, mark
switch-case statements where fall-through is intentional, explicitly in
order to fix a bunch of -Wimplicit-fallthrough warnings.
Warning level 3 was used: -Wimplicit-fallthrough=3.
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
arch/x86/include/asm/syscall.h | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index d653139857af..04fc5c120558 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -125,23 +125,30 @@ static inline void syscall_get_arguments(struct task_struct *task,
case 0:
if (!n--) break;
*args++ = regs->bx;
+ /* fall through */
case 1:
if (!n--) break;
*args++ = regs->cx;
+ /* fall through */
case 2:
if (!n--) break;
*args++ = regs->dx;
+ /* fall through */
case 3:
if (!n--) break;
*args++ = regs->si;
+ /* fall through */
case 4:
if (!n--) break;
*args++ = regs->di;
+ /* fall through */
case 5:
if (!n--) break;
*args++ = regs->bp;
+ /* fall through */
case 6:
if (!n--) break;
+ /* fall through */
default:
BUG();
break;
@@ -152,23 +159,30 @@ static inline void syscall_get_arguments(struct task_struct *task,
case 0:
if (!n--) break;
*args++ = regs->di;
+ /* fall through */
case 1:
if (!n--) break;
*args++ = regs->si;
+ /* fall through */
case 2:
if (!n--) break;
*args++ = regs->dx;
+ /* fall through */
case 3:
if (!n--) break;
*args++ = regs->r10;
+ /* fall through */
case 4:
if (!n--) break;
*args++ = regs->r8;
+ /* fall through */
case 5:
if (!n--) break;
*args++ = regs->r9;
+ /* fall through */
case 6:
if (!n--) break;
+ /* fall through */
default:
BUG();
break;
@@ -186,23 +200,30 @@ static inline void syscall_set_arguments(struct task_struct *task,
case 0:
if (!n--) break;
regs->bx = *args++;
+ /* fall through */
case 1:
if (!n--) break;
regs->cx = *args++;
+ /* fall through */
case 2:
if (!n--) break;
regs->dx = *args++;
+ /* fall through */
case 3:
if (!n--) break;
regs->si = *args++;
+ /* fall through */
case 4:
if (!n--) break;
regs->di = *args++;
+ /* fall through */
case 5:
if (!n--) break;
regs->bp = *args++;
+ /* fall through */
case 6:
if (!n--) break;
+ /* fall through */
default:
BUG();
break;
@@ -213,23 +234,30 @@ static inline void syscall_set_arguments(struct task_struct *task,
case 0:
if (!n--) break;
regs->di = *args++;
+ /* fall through */
case 1:
if (!n--) break;
regs->si = *args++;
+ /* fall through */
case 2:
if (!n--) break;
regs->dx = *args++;
+ /* fall through */
case 3:
if (!n--) break;
regs->r10 = *args++;
+ /* fall through */
case 4:
if (!n--) break;
regs->r8 = *args++;
+ /* fall through */
case 5:
if (!n--) break;
regs->r9 = *args++;
+ /* fall through */
case 6:
if (!n--) break;
+ /* fall through */
default:
BUG();
break;
--
2.20.1
On Tue, 29 Jan 2019, Gustavo A. R. Silva wrote:
> In preparation to enable -Wimplicit-fallthrough by default, mark
> switch-case statements where fall-through is intentional, explicitly in
> order to fix a bunch of -Wimplicit-fallthrough warnings.
>
> Warning level 3 was used: -Wimplicit-fallthrough=3.
Was used for what? For writing the patch or for finding the places?
Please be precise in your changelog statements. Te above doesn't make sense
for the casual reader.
Thanks,
tglx
On Wed, Nov 29, 2017 at 7:14 AM, Thomas Gleixner <[email protected]> wrote:
> On Wed, 29 Nov 2017, Gustavo A. R. Silva wrote:
>> Quoting Thomas Gleixner <[email protected]>:
>>
>> >
>> > So I have to ask WHY this information was not in the changelog of the patch
>> > in question:
>> >
>> > 1) How it works
>> >
>> > 2) Why comments have been chosen over macros
>> >
>>
>> I will add this info and send the patch again.
>>
>> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> > > where we are expecting to fall through.
>> >
>> > It's not a reviewers job to chase that information down.
>> >
>> > While I can understand that the comments are intentional due to existing
>> > tools, I still prefer the macro/annotation. But I'm not religious about it
>> > when there is common consensus. :)
>
> ^^^^^^^^^^^^^^^^
>
> This is the important point. And there are people aside of me who prefer the
> macro annotation.
Understood. I, too, prefer the macro annotation, but when considering
where the primary benefit comes from, I had to admit that using the
comments was tolerable: the many external tools (e.g. Coverity,
Eclipse, gcc, clang, etc) that already process the comments means we
gain their coverage without any additional work to teach them about
yet another way to mark fall-through. In other words, making a marking
that only works for humans and gcc leaves out the other tools. To me,
"it's ugly" isn't sufficient to limit the wider benefit.
And I do recognize that when we get all fixes landed and we add
-Wimplicit-fallthrough, then we can just ignore the tools that don't
understand the new marking and announce false positives: but it's not
worth everyone else's time to deal with those false positives. There
is already a solution that all "missing break" tools handle, so let's
use it, all false positives vanish, and everyone wins (with a small
"ugliness" cost).
-Kees
--
Kees Cook
Pixel Security
From 1585413938333802106@xxx Wed Nov 29 15:15:53 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Wed, 29 Nov 2017, Gustavo A. R. Silva wrote:
> Quoting Thomas Gleixner <[email protected]>:
>
> >
> > So I have to ask WHY this information was not in the changelog of the patch
> > in question:
> >
> > 1) How it works
> >
> > 2) Why comments have been chosen over macros
> >
>
> I will add this info and send the patch again.
>
> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > where we are expecting to fall through.
> >
> > It's not a reviewers job to chase that information down.
> >
> > While I can understand that the comments are intentional due to existing
> > tools, I still prefer the macro/annotation. But I'm not religious about it
> > when there is common consensus. :)
^^^^^^^^^^^^^^^^
This is the important point. And there are people aside of me who prefer the
macro annotation.
Thanks,
tglx
From 1585413678863127095@xxx Wed Nov 29 15:11:46 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Quoting Thomas Gleixner <[email protected]>:
>
> So I have to ask WHY this information was not in the changelog of the patch
> in question:
>
> 1) How it works
>
> 2) Why comments have been chosen over macros
>
I will add this info and send the patch again.
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>
> It's not a reviewers job to chase that information down.
>
> While I can understand that the comments are intentional due to existing
> tools, I still prefer the macro/annotation. But I'm not religious about it
> when there is common consensus. :)
>
Awesome
Thanks, Thomas.
--
Gustavo A. R. Silva
From 1585387939368028629@xxx Wed Nov 29 08:22:39 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Hi Joe,
On Wed, Nov 29, 2017 at 2:07 AM, Joe Perches <[email protected]> wrote:
> On Tue, 2017-11-28 at 14:37 -0600, Gustavo A. R. Silva wrote:
>> Quoting Linus Torvalds <[email protected]>:
>> > On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox
>> > <[email protected]> wrote:
>> > > The notation in question has been standard in tools like lint since the
>> > > end of the 1970s
>> >
>> > Yes.
>> >
>> > That said, maybe one option would be to annotate the "case:" and
>> > "default:" statements if that makes people happier.
>> >
>> > IOW, we could do something like
>> >
>> > #define fallthrough __atttibute__((fallthrough))
>> >
>> > and then write
>> >
>> > fallthrough case 1:
>> > ...
>> >
>> > which while absolutely not traditional, might look and read a bit more
>> > logical to people. I mean, it literally _is_ a "fallthrough case", so
>> > it makes semantic sense.
>> >
>>
>> This is elegant. The thing is that this makes it appear as if there is
>> an unconditional fall through.
>>
>> It is not uncommon to have multiple break statements in the same case
>> block and to fall through also.
>
> My preferred syntax would be to use __fallthrough or fallthrough
> in the same manner as break;
>
> switch (foo) {
> case bar:
> bar();
> fallthrough;
> case baz:
> baz();
> break;
> default;
> qux();
> exit(1);
> }
Makes sense to me.
Comments are fragile.
In addition, they are stripped if you run cpp (or gcc -E) separately, unlike
__atttibute__((fallthrough)).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From 1585360624122457906@xxx Wed Nov 29 01:08:29 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 2017-11-28 at 14:37 -0600, Gustavo A. R. Silva wrote:
> Quoting Linus Torvalds <[email protected]>:
>
> > On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox
> > <[email protected]> wrote:
> > >
> > > The notation in question has been standard in tools like lint since the
> > > end of the 1970s
> >
> > Yes.
> >
> > That said, maybe one option would be to annotate the "case:" and
> > "default:" statements if that makes people happier.
> >
> > IOW, we could do something like
> >
> > #define fallthrough __atttibute__((fallthrough))
> >
> > and then write
> >
> > fallthrough case 1:
> > ...
> >
> > which while absolutely not traditional, might look and read a bit more
> > logical to people. I mean, it literally _is_ a "fallthrough case", so
> > it makes semantic sense.
> >
>
> This is elegant. The thing is that this makes it appear as if there is
> an unconditional fall through.
>
> It is not uncommon to have multiple break statements in the same case
> block and to fall through also.
My preferred syntax would be to use __fallthrough or fallthrough
in the same manner as break;
switch (foo) {
case bar:
bar();
fallthrough;
case baz:
baz();
break;
default;
qux();
exit(1);
}
From 1585346717362488393@xxx Tue Nov 28 21:27:26 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> The thing about taking 'any comment' as valid is false if you add the
> following to your Makefile:
>
> KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough)
>
> This option takes the following comments as valid:
>
> /* fall through */
> /* Fall through */
> /* fall through - ... */
> /* Fall through - ... */
>
> Comments as fallthru, fallthrough, FALLTHRU are invalid.
>
> And of course if you intentionally change the option to:
>
> KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=1)
>
> it means that you obviously want to ignore any warning.
So I have to ask WHY this information was not in the changelog of the patch
in question:
1) How it works
2) Why comments have been chosen over macros
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
It's not a reviewers job to chase that information down.
While I can understand that the comments are intentional due to existing
tools, I still prefer the macro/annotation. But I'm not religious about it
when there is common consensus. :)
Thanks,
tglx
From 1585343630921733857@xxx Tue Nov 28 20:38:23 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Quoting Linus Torvalds <[email protected]>:
> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox
> <[email protected]> wrote:
>>
>> The notation in question has been standard in tools like lint since the
>> end of the 1970s
>
> Yes.
>
> That said, maybe one option would be to annotate the "case:" and
> "default:" statements if that makes people happier.
>
> IOW, we could do something like
>
> #define fallthrough __atttibute__((fallthrough))
>
> and then write
>
> fallthrough case 1:
> ...
>
> which while absolutely not traditional, might look and read a bit more
> logical to people. I mean, it literally _is_ a "fallthrough case", so
> it makes semantic sense.
>
This is elegant. The thing is that this makes it appear as if there is
an unconditional fall through.
It is not uncommon to have multiple break statements in the same case
block and to fall through also.
Thanks
--
Gustavo A. R. Silva
From 1585343591506835175@xxx Tue Nov 28 20:37:45 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, Nov 28, 2017 at 12:08 PM, Thomas Gleixner <[email protected]> wrote:
> On Tue, 28 Nov 2017, Linus Torvalds wrote:
>
>> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <[email protected]> wrote:
>> >
>> > The notation in question has been standard in tools like lint since the
>> > end of the 1970s
>>
>> Yes.
>>
>> That said, maybe one option would be to annotate the "case:" and
>> "default:" statements if that makes people happier.
>>
>> IOW, we could do something like
>>
>> #define fallthrough __atttibute__((fallthrough))
>>
>> and then write
>>
>> fallthrough case 1:
>> ...
>>
>> which while absolutely not traditional, might look and read a bit more
>> logical to people. I mean, it literally _is_ a "fallthrough case", so
>> it makes semantic sense.
>>
>> Or maybe people hate that kind of "making up new syntax" too?
>
> Fine with me. Better than any comment.
One of the strong reasons to do this with comments is because it lets
us leverage existing static analyzers. The long-standard method of
marking fall-through has been with comments, and that's what the
kernel should be (and has been) doing. If we invent another method,
we'll be shooting ourselves in the foot by making it harder to spot
these cases using existing tools. Fall-through is uncommon, and it's
not a big price to carry these comments when the gain is so clear.
The most "ugly" cases of these are when the switch statement is
_entirely_ fall-through (usually for bit-width processing of some
kind), but again, they're rare in the grand scheme of things.
-Kees
--
Kees Cook
Pixel Security
From 1585342907000306431@xxx Tue Nov 28 20:26:53 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Quoting Thomas Gleixner <[email protected]>:
> On Tue, 28 Nov 2017, Alan Cox wrote:
>
>> > I have no idea who came up with that brilliant idea of parsing comments in
>> > the code. It's so simple to make this parser completely fail that it's not
>>
>> Stephen Johnson (author of the V7 portable C compiler), which is where
>> it's from (the lint tool). He also wrote yacc so he does know a bit about
>> parsers 8).
>
> I don't doubt that.
>
>> > even funny anymore.
>>
>> The notation in question has been standard in tools like lint since the
>> end of the 1970s
>
> Fair enough.
>
> Still that does not make the GCC implementation which defaults to take 'any
> comment' as valid any better and does not solve other parsing issues which
> have been pointed out in various GCC bugs. Using the macro annotation is
> distinct and has no ifs and buts.
>
The thing about taking 'any comment' as valid is false if you add the
following to your Makefile:
KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough)
This option takes the following comments as valid:
/* fall through */
/* Fall through */
/* fall through - ... */
/* Fall through - ... */
Comments as fallthru, fallthrough, FALLTHRU are invalid.
And of course if you intentionally change the option to:
KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=1)
it means that you obviously want to ignore any warning.
Thanks
--
Gustavo A. R. Silva
From 1585342041888456712@xxx Tue Nov 28 20:13:08 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Alan Cox wrote:
> > I have no idea who came up with that brilliant idea of parsing comments in
> > the code. It's so simple to make this parser completely fail that it's not
>
> Stephen Johnson (author of the V7 portable C compiler), which is where
> it's from (the lint tool). He also wrote yacc so he does know a bit about
> parsers 8).
I don't doubt that.
> > even funny anymore.
>
> The notation in question has been standard in tools like lint since the
> end of the 1970s
Fair enough.
Still that does not make the GCC implementation which defaults to take 'any
comment' as valid any better and does not solve other parsing issues which
have been pointed out in various GCC bugs. Using the macro annotation is
distinct and has no ifs and buts.
Thanks,
tglx
From 1585341933997968398@xxx Tue Nov 28 20:11:25 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Linus Torvalds wrote:
> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <[email protected]> wrote:
> >
> > The notation in question has been standard in tools like lint since the
> > end of the 1970s
>
> Yes.
>
> That said, maybe one option would be to annotate the "case:" and
> "default:" statements if that makes people happier.
>
> IOW, we could do something like
>
> #define fallthrough __atttibute__((fallthrough))
>
> and then write
>
> fallthrough case 1:
> ...
>
> which while absolutely not traditional, might look and read a bit more
> logical to people. I mean, it literally _is_ a "fallthrough case", so
> it makes semantic sense.
>
> Or maybe people hate that kind of "making up new syntax" too?
Fine with me. Better than any comment.
Thanks,
tglx
From 1585341355527698265@xxx Tue Nov 28 20:02:13 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 2017-11-28 at 11:10 -0800, Linus Torvalds wrote:
> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <[email protected]> wrote:
> >
> > The notation in question has been standard in tools like lint since the
> > end of the 1970s
>
> Yes.
>
> That said, maybe one option would be to annotate the "case:" and
> "default:" statements if that makes people happier.
>
> IOW, we could do something like
>
> #define fallthrough __atttibute__((fallthrough))
>
> and then write
>
> fallthrough case 1:
> ...
>
> which while absolutely not traditional, might look and read a bit more
> logical to people. I mean, it literally _is_ a "fallthrough case", so
> it makes semantic sense.
>
> Or maybe people hate that kind of "making up new syntax" too?
I don't
https://lkml.org/lkml/2017/2/10/485
From 1585340571358614764@xxx Tue Nov 28 19:49:45 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> This is what we want to add:
>
> # Warn about missing switch break or fall-through comment.
> KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough)
Can this be made to ignore comments and only accept the proper annotation?
> > > I have no idea who came up with that brilliant idea of parsing comments in
> > > the code. It's so simple to make this parser completely fail that it's not
> > > even funny anymore.
> > >
>
> I don't get why someone would want to do that to himself. :/
Well, it's not intentional.
You add any comment and the parser thinks its correct. Or you typo the
thing and it fails to recognize.
Thanks,
tglx
From 1585338206098438038@xxx Tue Nov 28 19:12:09 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <[email protected]> wrote:
>
> The notation in question has been standard in tools like lint since the
> end of the 1970s
Yes.
That said, maybe one option would be to annotate the "case:" and
"default:" statements if that makes people happier.
IOW, we could do something like
#define fallthrough __atttibute__((fallthrough))
and then write
fallthrough case 1:
...
which while absolutely not traditional, might look and read a bit more
logical to people. I mean, it literally _is_ a "fallthrough case", so
it makes semantic sense.
Or maybe people hate that kind of "making up new syntax" too?
Linus
From 1585337663928630564@xxx Tue Nov 28 19:03:32 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
> I have no idea who came up with that brilliant idea of parsing comments in
> the code. It's so simple to make this parser completely fail that it's not
Stephen Johnson (author of the V7 portable C compiler), which is where
it's from (the lint tool). He also wrote yacc so he does know a bit about
parsers 8).
> even funny anymore.
The notation in question has been standard in tools like lint since the
end of the 1970s
Alan
From 1585337167266328565@xxx Tue Nov 28 18:55:39 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Quoting Thomas Gleixner <[email protected]>:
> On Tue, 28 Nov 2017, Thomas Gleixner wrote:
>
> +CC Linus.
>
>> On Tue, 28 Nov 2017, Thomas Gleixner wrote:
>>
>> > On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
>> > > Quoting Thomas Gleixner <[email protected]>:
>> > > > > To be honest, such comments annoy me during a code review
>> especially when
>> > > > > the fallthrough is so obvious as in this case. There might
>> be cases where
>> > > > > its worth to document because it's non obvious, but documenting the
>> > > > > obvious
>> > > > > just for the sake of documenting it is just wrong.
>> > > >
>> > >
>> > > I understand that and I agree that in this particular case it
>> is just obvious.
>> > > The thing is that if we want to benefit from having the
>> compiler help us to
>> > > spot these kind of issues before committing our code, we have
>> to address every
>> > > place in the whole code-base.
>> > >
>> > > > And _IF_ at all then you want a fixed macro for this and not a comment
>> > > > which will be formatted as people see it fit.
>> > > >
>> > > > GCC supports: __attribute__ ((fallthrough)) which we can wrap
>> into a macro,
>> > > > e.g. falltrough()
>> > > >
>> > > > That'd be useful, but adding all these comments and then
>> having to chase a
>> > > > gazillion of warning instances to figure out whether there is
>> a comment or
>> > > > not is just backwards.
>> > > >
>> > >
>> > > I have run into this before and people find what you suggest
>> even uglier.
>> >
>> > It's not about ugly. It's about _USEFULL_.
>> >
>> > The comments are ugly AND completely useless for the compiler and they are
>> > going to be malformatted so checker tools can't differentiate the false
>> > positives.
>> >
>> > The macro, in which more or less ugly form written, is both documentation
>> > and helps the compiler NOT to emit the same crap over and over.
>>
>> Just checked and GCC really supports analyzing the comment to some extent.
>>
>> But just look at
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77817
>>
>> " It is not really possible. __attribute__((fallthrough)) has precise
>> rules on where it can appear, while /* FALLTHRU */ comments, being
>> comments, can appear anywhere. Especially with -Wimplicit-fallthrough=1
>> when all comments are considered fallthru comments... "
>>
This is what we want to add:
# Warn about missing switch break or fall-through comment.
KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough)
>> I have no idea who came up with that brilliant idea of parsing comments in
>> the code. It's so simple to make this parser completely fail that it's not
>> even funny anymore.
>>
I don't get why someone would want to do that to himself. :/
>> I don't care what other people prefer. The code base I'm responsible for
>> gets either proper annotations or nothing.
>
> And in fact we want ONE solution for the whole kernel. And comments are
> obviously the wrong one.
>
OK. I'll discuss this and see how we can come up with the best solution.
Thank you for your feedback
--
Gustavo A. R. Silva
From 1585336670880278820@xxx Tue Nov 28 18:47:45 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Thomas Gleixner wrote:
+CC Linus.
> On Tue, 28 Nov 2017, Thomas Gleixner wrote:
>
> > On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> > > Quoting Thomas Gleixner <[email protected]>:
> > > > > To be honest, such comments annoy me during a code review especially when
> > > > > the fallthrough is so obvious as in this case. There might be cases where
> > > > > its worth to document because it's non obvious, but documenting the
> > > > > obvious
> > > > > just for the sake of documenting it is just wrong.
> > > >
> > >
> > > I understand that and I agree that in this particular case it is just obvious.
> > > The thing is that if we want to benefit from having the compiler help us to
> > > spot these kind of issues before committing our code, we have to address every
> > > place in the whole code-base.
> > >
> > > > And _IF_ at all then you want a fixed macro for this and not a comment
> > > > which will be formatted as people see it fit.
> > > >
> > > > GCC supports: __attribute__ ((fallthrough)) which we can wrap into a macro,
> > > > e.g. falltrough()
> > > >
> > > > That'd be useful, but adding all these comments and then having to chase a
> > > > gazillion of warning instances to figure out whether there is a comment or
> > > > not is just backwards.
> > > >
> > >
> > > I have run into this before and people find what you suggest even uglier.
> >
> > It's not about ugly. It's about _USEFULL_.
> >
> > The comments are ugly AND completely useless for the compiler and they are
> > going to be malformatted so checker tools can't differentiate the false
> > positives.
> >
> > The macro, in which more or less ugly form written, is both documentation
> > and helps the compiler NOT to emit the same crap over and over.
>
> Just checked and GCC really supports analyzing the comment to some extent.
>
> But just look at
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77817
>
> " It is not really possible. __attribute__((fallthrough)) has precise
> rules on where it can appear, while /* FALLTHRU */ comments, being
> comments, can appear anywhere. Especially with -Wimplicit-fallthrough=1
> when all comments are considered fallthru comments... "
>
> I have no idea who came up with that brilliant idea of parsing comments in
> the code. It's so simple to make this parser completely fail that it's not
> even funny anymore.
>
> I don't care what other people prefer. The code base I'm responsible for
> gets either proper annotations or nothing.
And in fact we want ONE solution for the whole kernel. And comments are
obviously the wrong one.
Thanks,
tglx
From 1585336052036810145@xxx Tue Nov 28 18:37:55 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Thomas Gleixner wrote:
> On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> > Quoting Thomas Gleixner <[email protected]>:
> > > > To be honest, such comments annoy me during a code review especially when
> > > > the fallthrough is so obvious as in this case. There might be cases where
> > > > its worth to document because it's non obvious, but documenting the
> > > > obvious
> > > > just for the sake of documenting it is just wrong.
> > >
> >
> > I understand that and I agree that in this particular case it is just obvious.
> > The thing is that if we want to benefit from having the compiler help us to
> > spot these kind of issues before committing our code, we have to address every
> > place in the whole code-base.
> >
> > > And _IF_ at all then you want a fixed macro for this and not a comment
> > > which will be formatted as people see it fit.
> > >
> > > GCC supports: __attribute__ ((fallthrough)) which we can wrap into a macro,
> > > e.g. falltrough()
> > >
> > > That'd be useful, but adding all these comments and then having to chase a
> > > gazillion of warning instances to figure out whether there is a comment or
> > > not is just backwards.
> > >
> >
> > I have run into this before and people find what you suggest even uglier.
>
> It's not about ugly. It's about _USEFULL_.
>
> The comments are ugly AND completely useless for the compiler and they are
> going to be malformatted so checker tools can't differentiate the false
> positives.
>
> The macro, in which more or less ugly form written, is both documentation
> and helps the compiler NOT to emit the same crap over and over.
Just checked and GCC really supports analyzing the comment to some extent.
But just look at
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77817
" It is not really possible. __attribute__((fallthrough)) has precise
rules on where it can appear, while /* FALLTHRU */ comments, being
comments, can appear anywhere. Especially with -Wimplicit-fallthrough=1
when all comments are considered fallthru comments... "
I have no idea who came up with that brilliant idea of parsing comments in
the code. It's so simple to make this parser completely fail that it's not
even funny anymore.
I don't care what other people prefer. The code base I'm responsible for
gets either proper annotations or nothing.
Thanks,
tglx
From 1585335433902976416@xxx Tue Nov 28 18:28:06 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> Quoting Thomas Gleixner <[email protected]>:
> > > To be honest, such comments annoy me during a code review especially when
> > > the fallthrough is so obvious as in this case. There might be cases where
> > > its worth to document because it's non obvious, but documenting the
> > > obvious
> > > just for the sake of documenting it is just wrong.
> >
>
> I understand that and I agree that in this particular case it is just obvious.
> The thing is that if we want to benefit from having the compiler help us to
> spot these kind of issues before committing our code, we have to address every
> place in the whole code-base.
>
> > And _IF_ at all then you want a fixed macro for this and not a comment
> > which will be formatted as people see it fit.
> >
> > GCC supports: __attribute__ ((fallthrough)) which we can wrap into a macro,
> > e.g. falltrough()
> >
> > That'd be useful, but adding all these comments and then having to chase a
> > gazillion of warning instances to figure out whether there is a comment or
> > not is just backwards.
> >
>
> I have run into this before and people find what you suggest even uglier.
It's not about ugly. It's about _USEFULL_.
The comments are ugly AND completely useless for the compiler and they are
going to be malformatted so checker tools can't differentiate the false
positives.
The macro, in which more or less ugly form written, is both documentation
and helps the compiler NOT to emit the same crap over and over.
Thanks,
tglx
From 1585335141593876673@xxx Tue Nov 28 18:23:27 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Quoting Thomas Gleixner <[email protected]>:
> On Tue, 28 Nov 2017, Thomas Gleixner wrote:
>> On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
>> > Quoting Thomas Gleixner <[email protected]>:
>> >
>> > > On Mon, 27 Nov 2017, Gustavo A. R. Silva wrote:
>> > >
>> > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> > > > where we are expecting to fall through.
>> > >
>> > > > case 0:
>> > > > if (!n--) break;
>> > > > *args++ = regs->bx;
>> > > > + /* fall through */
>> > >
>> > > And these gazillions of pointless comments help enabling of
>> > > -Wimplicit-fallthrough in which way?
>> > >
>> >
>> > The -Wimplicit-fallthrough option was added to GCC 7. We want to add that
>> > option to the top-level Makefile so we can have the compiler help
>> us not make
>> > mistakes as missing "break"s or "continue"s. This also documents
>> the intention
>> > for humans and provides a way for analyzers to report issues or
>> ignore False
>> > Positives.
>> >
>> > So prior to adding such option to the Makefile, we have to
>> properly add a code
>> > comment wherever the code is intended to fall through.
>> >
>> > During the process of placing these comments I have identified actual bugs
>> > (missing "break"s/"continue"s) in a variety of components in the
>> kernel, so I
>> > think this effort is valuable. Lastly, such a simple comment in
>> the code can
>> > save a person plenty of time during a code review.
>>
>> To be honest, such comments annoy me during a code review especially when
>> the fallthrough is so obvious as in this case. There might be cases where
>> its worth to document because it's non obvious, but documenting the obvious
>> just for the sake of documenting it is just wrong.
>
I understand that and I agree that in this particular case it is just
obvious. The thing is that if we want to benefit from having the
compiler help us to spot these kind of issues before committing our
code, we have to address every place in the whole code-base.
> And _IF_ at all then you want a fixed macro for this and not a comment
> which will be formatted as people see it fit.
>
> GCC supports: __attribute__ ((fallthrough)) which we can wrap into a macro,
> e.g. falltrough()
>
> That'd be useful, but adding all these comments and then having to chase a
> gazillion of warning instances to figure out whether there is a comment or
> not is just backwards.
>
I have run into this before and people find what you suggest even uglier.
Thanks
--
Gustavo A. R. Silva
From 1585334846275683855@xxx Tue Nov 28 18:18:45 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Thomas Gleixner wrote:
> On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> > Quoting Thomas Gleixner <[email protected]>:
> >
> > > On Mon, 27 Nov 2017, Gustavo A. R. Silva wrote:
> > >
> > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > > where we are expecting to fall through.
> > >
> > > > case 0:
> > > > if (!n--) break;
> > > > *args++ = regs->bx;
> > > > + /* fall through */
> > >
> > > And these gazillions of pointless comments help enabling of
> > > -Wimplicit-fallthrough in which way?
> > >
> >
> > The -Wimplicit-fallthrough option was added to GCC 7. We want to add that
> > option to the top-level Makefile so we can have the compiler help us not make
> > mistakes as missing "break"s or "continue"s. This also documents the intention
> > for humans and provides a way for analyzers to report issues or ignore False
> > Positives.
> >
> > So prior to adding such option to the Makefile, we have to properly add a code
> > comment wherever the code is intended to fall through.
> >
> > During the process of placing these comments I have identified actual bugs
> > (missing "break"s/"continue"s) in a variety of components in the kernel, so I
> > think this effort is valuable. Lastly, such a simple comment in the code can
> > save a person plenty of time during a code review.
>
> To be honest, such comments annoy me during a code review especially when
> the fallthrough is so obvious as in this case. There might be cases where
> its worth to document because it's non obvious, but documenting the obvious
> just for the sake of documenting it is just wrong.
And _IF_ at all then you want a fixed macro for this and not a comment
which will be formatted as people see it fit.
GCC supports: __attribute__ ((fallthrough)) which we can wrap into a macro,
e.g. falltrough()
That'd be useful, but adding all these comments and then having to chase a
gazillion of warning instances to figure out whether there is a comment or
not is just backwards.
Sure, but slapping a comment everywhere is just simpler than reading the
documentation and make something useful and understandable.
Thanks,
tglx
From 1585334479184006714@xxx Tue Nov 28 18:12:55 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> Quoting Thomas Gleixner <[email protected]>:
>
> > On Mon, 27 Nov 2017, Gustavo A. R. Silva wrote:
> >
> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > where we are expecting to fall through.
> >
> > > case 0:
> > > if (!n--) break;
> > > *args++ = regs->bx;
> > > + /* fall through */
> >
> > And these gazillions of pointless comments help enabling of
> > -Wimplicit-fallthrough in which way?
> >
>
> The -Wimplicit-fallthrough option was added to GCC 7. We want to add that
> option to the top-level Makefile so we can have the compiler help us not make
> mistakes as missing "break"s or "continue"s. This also documents the intention
> for humans and provides a way for analyzers to report issues or ignore False
> Positives.
>
> So prior to adding such option to the Makefile, we have to properly add a code
> comment wherever the code is intended to fall through.
>
> During the process of placing these comments I have identified actual bugs
> (missing "break"s/"continue"s) in a variety of components in the kernel, so I
> think this effort is valuable. Lastly, such a simple comment in the code can
> save a person plenty of time during a code review.
To be honest, such comments annoy me during a code review especially when
the fallthrough is so obvious as in this case. There might be cases where
its worth to document because it's non obvious, but documenting the obvious
just for the sake of documenting it is just wrong.
Thanks,
tglx
From 1585334143643478720@xxx Tue Nov 28 18:07:35 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
Quoting Thomas Gleixner <[email protected]>:
> On Mon, 27 Nov 2017, Gustavo A. R. Silva wrote:
>
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>
>> case 0:
>> if (!n--) break;
>> *args++ = regs->bx;
>> + /* fall through */
>
> And these gazillions of pointless comments help enabling of
> -Wimplicit-fallthrough in which way?
>
The -Wimplicit-fallthrough option was added to GCC 7. We want to add
that option to the top-level Makefile so we can have the compiler help
us not make mistakes as missing "break"s or "continue"s. This also
documents the intention for humans and provides a way for analyzers to
report issues or ignore False Positives.
So prior to adding such option to the Makefile, we have to properly
add a code comment wherever the code is intended to fall through.
During the process of placing these comments I have identified actual
bugs (missing "break"s/"continue"s) in a variety of components in the
kernel, so I think this effort is valuable. Lastly, such a simple
comment in the code can save a person plenty of time during a code
review.
Thanks
--
Gustavo A. R. Silva
From 1585317989981270721@xxx Tue Nov 28 13:50:50 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Mon, 27 Nov 2017, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> case 0:
> if (!n--) break;
> *args++ = regs->bx;
> + /* fall through */
And these gazillions of pointless comments help enabling of
-Wimplicit-fallthrough in which way?
Thanks,
tglx
From 1585265423856272058@xxx Mon Nov 27 23:55:19 +0000 2017
X-GM-THRID: 1585265423856272058
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread