linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86/syscalls: Mark expected switch fall-throughs
@ 2019-01-29 23:56 Gustavo A. R. Silva
  2019-01-30  0:14 ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2019-01-29 23:56 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86
  Cc: linux-kernel, Dominik Brodowski, Andy Lutomirski, Kees Cook

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2019-01-29 23:56 [PATCH] x86/syscalls: Mark expected switch fall-throughs Gustavo A. R. Silva
@ 2019-01-30  0:14 ` Thomas Gleixner
  0 siblings, 0 replies; 27+ messages in thread
From: Thomas Gleixner @ 2019-01-30  0:14 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-kernel,
	Dominik Brodowski, Andy Lutomirski, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-29 15:14                         ` Thomas Gleixner
@ 2017-11-30  0:21                           ` Kees Cook
  0 siblings, 0 replies; 27+ messages in thread
From: Kees Cook @ 2017-11-30  0:21 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Gustavo A. R. Silva, Alan Cox, Ingo Molnar, H. Peter Anvin, X86 ML, LKML

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-29 15:10                       ` Gustavo A. R. Silva
@ 2017-11-29 15:14                         ` Thomas Gleixner
  2017-11-30  0:21                           ` Kees Cook
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-29 15:14 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Alan Cox, Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 21:25                     ` Thomas Gleixner
@ 2017-11-29 15:10                       ` Gustavo A. R. Silva
  2017-11-29 15:14                         ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-29 15:10 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Alan Cox, Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-29  1:07                     ` Joe Perches
@ 2017-11-29  8:20                       ` Geert Uytterhoeven
  0 siblings, 0 replies; 27+ messages in thread
From: Geert Uytterhoeven @ 2017-11-29  8:20 UTC (permalink / raw)
  To: Joe Perches
  Cc: Gustavo A. R. Silva, Linus Torvalds, Alan Cox, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, the arch/x86 maintainers, LKML,
	Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 20:37                   ` Gustavo A. R. Silva
@ 2017-11-29  1:07                     ` Joe Perches
  2017-11-29  8:20                       ` Geert Uytterhoeven
  0 siblings, 1 reply; 27+ messages in thread
From: Joe Perches @ 2017-11-29  1:07 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Linus Torvalds
  Cc: Alan Cox, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	the arch/x86 maintainers, LKML, Kees Cook

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);
	}

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 20:25                   ` Gustavo A. R. Silva
@ 2017-11-28 21:25                     ` Thomas Gleixner
  2017-11-29 15:10                       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 21:25 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Alan Cox, Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 19:10                 ` Linus Torvalds
  2017-11-28 19:59                   ` Joe Perches
  2017-11-28 20:08                   ` Thomas Gleixner
@ 2017-11-28 20:37                   ` Gustavo A. R. Silva
  2017-11-29  1:07                     ` Joe Perches
  2 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-28 20:37 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Alan Cox, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	the arch/x86 maintainers, LKML, Kees Cook


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 20:08                   ` Thomas Gleixner
@ 2017-11-28 20:34                     ` Kees Cook
  0 siblings, 0 replies; 27+ messages in thread
From: Kees Cook @ 2017-11-28 20:34 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Linus Torvalds, Alan Cox, Gustavo A. R. Silva, Ingo Molnar,
	H. Peter Anvin, the arch/x86 maintainers, LKML

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 20:11                 ` Thomas Gleixner
@ 2017-11-28 20:25                   ` Gustavo A. R. Silva
  2017-11-28 21:25                     ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-28 20:25 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Alan Cox, Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 19:00               ` Alan Cox
  2017-11-28 19:10                 ` Linus Torvalds
@ 2017-11-28 20:11                 ` Thomas Gleixner
  2017-11-28 20:25                   ` Gustavo A. R. Silva
  1 sibling, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 20:11 UTC (permalink / raw)
  To: Alan Cox
  Cc: Gustavo A. R. Silva, Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 19:10                 ` Linus Torvalds
  2017-11-28 19:59                   ` Joe Perches
@ 2017-11-28 20:08                   ` Thomas Gleixner
  2017-11-28 20:34                     ` Kees Cook
  2017-11-28 20:37                   ` Gustavo A. R. Silva
  2 siblings, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 20:08 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Alan Cox, Gustavo A. R. Silva, Ingo Molnar, H. Peter Anvin,
	the arch/x86 maintainers, LKML, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 19:10                 ` Linus Torvalds
@ 2017-11-28 19:59                   ` Joe Perches
  2017-11-28 20:08                   ` Thomas Gleixner
  2017-11-28 20:37                   ` Gustavo A. R. Silva
  2 siblings, 0 replies; 27+ messages in thread
From: Joe Perches @ 2017-11-28 19:59 UTC (permalink / raw)
  To: Linus Torvalds, Alan Cox
  Cc: Thomas Gleixner, Gustavo A. R. Silva, Ingo Molnar,
	H. Peter Anvin, the arch/x86 maintainers, LKML, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:53                 ` Gustavo A. R. Silva
@ 2017-11-28 19:48                   ` Thomas Gleixner
  0 siblings, 0 replies; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 19:48 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook, Linus Torvalds

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 19:00               ` Alan Cox
@ 2017-11-28 19:10                 ` Linus Torvalds
  2017-11-28 19:59                   ` Joe Perches
                                     ` (2 more replies)
  2017-11-28 20:11                 ` Thomas Gleixner
  1 sibling, 3 replies; 27+ messages in thread
From: Linus Torvalds @ 2017-11-28 19:10 UTC (permalink / raw)
  To: Alan Cox
  Cc: Thomas Gleixner, Gustavo A. R. Silva, Ingo Molnar,
	H. Peter Anvin, the arch/x86 maintainers, LKML, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:35             ` Thomas Gleixner
  2017-11-28 18:45               ` Thomas Gleixner
@ 2017-11-28 19:00               ` Alan Cox
  2017-11-28 19:10                 ` Linus Torvalds
  2017-11-28 20:11                 ` Thomas Gleixner
  1 sibling, 2 replies; 27+ messages in thread
From: Alan Cox @ 2017-11-28 19:00 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Gustavo A. R. Silva, Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook

> 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:45               ` Thomas Gleixner
@ 2017-11-28 18:53                 ` Gustavo A. R. Silva
  2017-11-28 19:48                   ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-28 18:53 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook, Linus Torvalds


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:35             ` Thomas Gleixner
@ 2017-11-28 18:45               ` Thomas Gleixner
  2017-11-28 18:53                 ` Gustavo A. R. Silva
  2017-11-28 19:00               ` Alan Cox
  1 sibling, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 18:45 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook, Linus Torvalds

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:27           ` Thomas Gleixner
@ 2017-11-28 18:35             ` Thomas Gleixner
  2017-11-28 18:45               ` Thomas Gleixner
  2017-11-28 19:00               ` Alan Cox
  0 siblings, 2 replies; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 18:35 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Ingo Molnar, H. Peter Anvin, x86, LKML, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:22         ` Gustavo A. R. Silva
@ 2017-11-28 18:27           ` Thomas Gleixner
  2017-11-28 18:35             ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 18:27 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:17       ` Thomas Gleixner
@ 2017-11-28 18:22         ` Gustavo A. R. Silva
  2017-11-28 18:27           ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-28 18:22 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Kees Cook


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:10     ` Thomas Gleixner
@ 2017-11-28 18:17       ` Thomas Gleixner
  2017-11-28 18:22         ` Gustavo A. R. Silva
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 18:17 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 18:05   ` Gustavo A. R. Silva
@ 2017-11-28 18:10     ` Thomas Gleixner
  2017-11-28 18:17       ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 18:10 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-28 13:49 ` Thomas Gleixner
@ 2017-11-28 18:05   ` Gustavo A. R. Silva
  2017-11-28 18:10     ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-28 18:05 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Kees Cook


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
  2017-11-27 23:52 Gustavo A. R. Silva
@ 2017-11-28 13:49 ` Thomas Gleixner
  2017-11-28 18:05   ` Gustavo A. R. Silva
  0 siblings, 1 reply; 27+ messages in thread
From: Thomas Gleixner @ 2017-11-28 13:49 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH] x86/syscalls: Mark expected switch fall-throughs
@ 2017-11-27 23:52 Gustavo A. R. Silva
  2017-11-28 13:49 ` Thomas Gleixner
  0 siblings, 1 reply; 27+ messages in thread
From: Gustavo A. R. Silva @ 2017-11-27 23:52 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
  Cc: linux-kernel, Gustavo A. R. Silva, Kees Cook

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2019-01-30  0:18 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-29 23:56 [PATCH] x86/syscalls: Mark expected switch fall-throughs Gustavo A. R. Silva
2019-01-30  0:14 ` Thomas Gleixner
  -- strict thread matches above, loose matches on Subject: below --
2017-11-27 23:52 Gustavo A. R. Silva
2017-11-28 13:49 ` Thomas Gleixner
2017-11-28 18:05   ` Gustavo A. R. Silva
2017-11-28 18:10     ` Thomas Gleixner
2017-11-28 18:17       ` Thomas Gleixner
2017-11-28 18:22         ` Gustavo A. R. Silva
2017-11-28 18:27           ` Thomas Gleixner
2017-11-28 18:35             ` Thomas Gleixner
2017-11-28 18:45               ` Thomas Gleixner
2017-11-28 18:53                 ` Gustavo A. R. Silva
2017-11-28 19:48                   ` Thomas Gleixner
2017-11-28 19:00               ` Alan Cox
2017-11-28 19:10                 ` Linus Torvalds
2017-11-28 19:59                   ` Joe Perches
2017-11-28 20:08                   ` Thomas Gleixner
2017-11-28 20:34                     ` Kees Cook
2017-11-28 20:37                   ` Gustavo A. R. Silva
2017-11-29  1:07                     ` Joe Perches
2017-11-29  8:20                       ` Geert Uytterhoeven
2017-11-28 20:11                 ` Thomas Gleixner
2017-11-28 20:25                   ` Gustavo A. R. Silva
2017-11-28 21:25                     ` Thomas Gleixner
2017-11-29 15:10                       ` Gustavo A. R. Silva
2017-11-29 15:14                         ` Thomas Gleixner
2017-11-30  0:21                           ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).