[PATCH] x86/syscalls: Mark expected switch fall-throughs

[PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

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 <garsilva@embeddedor.com>
---
 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Kees Cook]

On Wed, Nov 29, 2017 at 7:14 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 29 Nov 2017, Gustavo A. R. Silva wrote:
>> Quoting Thomas Gleixner <tglx@linutronix.de>:
>>
>> >
>> > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

On Wed, 29 Nov 2017, Gustavo A. R. Silva wrote:
> Quoting Thomas Gleixner <tglx@linutronix.de>:
> 
> > 
> > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting Thomas Gleixner <tglx@linutronix.de>:

>
> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Geert Uytterhoeven]

Hi Joe,

On Wed, Nov 29, 2017 at 2:07 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2017-11-28 at 14:37 -0600, Gustavo A. R. Silva wrote:
>> Quoting Linus Torvalds <torvalds@linux-foundation.org>:
>> > On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox
>> > <gnomes@lxorguk.ukuu.org.uk> 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 -- geert@linux-m68k.org

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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Joe Perches]

On Tue, 2017-11-28 at 14:37 -0600, Gustavo A. R. Silva wrote:
> Quoting Linus Torvalds <torvalds@linux-foundation.org>:
> 
> > On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox  
> > <gnomes@lxorguk.ukuu.org.uk> 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);
	}

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting Linus Torvalds <torvalds@linux-foundation.org>:

> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox  
> <gnomes@lxorguk.ukuu.org.uk> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Kees Cook]

On Tue, Nov 28, 2017 at 12:08 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 28 Nov 2017, Linus Torvalds wrote:
>
>> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting Thomas Gleixner <tglx@linutronix.de>:

> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

On Tue, 28 Nov 2017, Linus Torvalds wrote:

> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Joe Perches]

On Tue, 2017-11-28 at 11:10 -0800, Linus Torvalds wrote:
> On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Linus Torvalds]

On Tue, Nov 28, 2017 at 11:00 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Alan Cox]

> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting Thomas Gleixner <tglx@linutronix.de>:

> 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 <tglx@linutronix.de>:
>> > > > > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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 <tglx@linutronix.de>:
> > > > > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

On Tue, 28 Nov 2017, Thomas Gleixner wrote:

> On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> > Quoting Thomas Gleixner <tglx@linutronix.de>:
> > > > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> Quoting Thomas Gleixner <tglx@linutronix.de>:
> > > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting Thomas Gleixner <tglx@linutronix.de>:

> On Tue, 28 Nov 2017, Thomas Gleixner wrote:
>> On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
>> > Quoting Thomas Gleixner <tglx@linutronix.de>:
>> >
>> > > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

On Tue, 28 Nov 2017, Thomas Gleixner wrote:
> On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> > Quoting Thomas Gleixner <tglx@linutronix.de>:
> > 
> > > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

On Tue, 28 Nov 2017, Gustavo A. R. Silva wrote:
> Quoting Thomas Gleixner <tglx@linutronix.de>:
> 
> > 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting Thomas Gleixner <tglx@linutronix.de>:

> 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

Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs

[Thomas Gleixner]

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

[PATCH] x86/syscalls: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 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 e3c95e8..63b01b1 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -121,23 +121,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;
@@ -148,23 +155,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;
@@ -182,23 +196,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;
@@ -209,23 +230,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.7.4