x86/syscalls: Mark expected switch fall-throughs

Quoted 3 lines

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

arch/x86/include/asm/syscall.h static inline void syscall_get_arguments(struct task_struct *task, · line 124

Quoted 4 lines

arch/x86/include/asm/syscall.h +1 -0

 		case 0:
 			if (!n--) break;
 			*args++ = regs->bx;
+			/* fall through */

And these gazillions of pointless comments help enabling of -Wimplicit-fallthrough in which way?

Quoting Thomas Gleixner <[email protected]>:

Quoted 5 lines · Gustavo A. R. Silva

Quoted 3 lines

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

Quoted 8 lines

Review

Quoted 4 lines

		case 0:
			if (!n--) break;
			*args++ = regs->bx;

(quoted diff) +1 -0

+			/* 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.

Quoted 22 lines · Gustavo A. R. Silva

Quoting Thomas Gleixner <[email protected]>:

Quoted 5 lines · Gustavo A. R. Silva

Quoted 3 lines

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

Quoted 8 lines

Review

Quoted 4 lines

		case 0:
			if (!n--) break;
			*args++ = regs->bx;

(quoted diff) +1 -0

+			/* 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.

Quoted 7 lines

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.

Quoted 33 lines · Gustavo A. R. Silva

Quoted 16 lines

Quoting Thomas Gleixner <[email protected]>:

Quoted 5 lines · Gustavo A. R. Silva

Quoted 3 lines

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

Quoted 8 lines

Review

Quoted 4 lines

		case 0:
			if (!n--) break;
			*args++ = regs->bx;

(quoted diff) +1 -0

+			/* fall through */

And these gazillions of pointless comments help enabling of -Wimplicit-fallthrough in which way?

Quoted 2 lines

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

Quoted 1 line

mistakes as missing "break"s or "continue"s. This also documents

the intention

Quoted 1 line

for humans and provides a way for analyzers to report issues or

ignore False

Quoted 3 lines

Positives.

So prior to adding such option to the Makefile, we have to

properly add a code

Quoted 4 lines

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

Quoted 8 lines

Quoted 1 line

think this effort is valuable. Lastly, such a simple comment in

the code can

Quoted 1 line

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.

Quoting Thomas Gleixner <[email protected]>:

Quoted 6 lines

Quoted 5 lines

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.

Quoted 6 lines

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

Quoted 4 lines

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.

Quoted 19 lines · Gustavo A. R. Silva

Quoting Thomas Gleixner <[email protected]>:

Quoted 6 lines

Quoted 5 lines

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.

Quoted 6 lines

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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 28 lines · Gustavo A. R. Silva

Quoted 19 lines

Quoting Thomas Gleixner <[email protected]>:

Quoted 6 lines

Quoted 5 lines

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.

Quoted 6 lines

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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 8 lines

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.

+CC Linus.

Quoted 58 lines · Thomas Gleixner

Quoted 4 lines · Gustavo A. R. Silva

Quoted 2 lines

Quoting Thomas Gleixner <[email protected]>:

Quoted 1 line

Quoted 1 line

To be honest, such comments annoy me during a code review

especially when

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the fallthrough is so obvious as in this case. There might

be cases where

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line

The thing is that if we want to benefit from having the

compiler help us to

Quoted 1 line

Quoted 1 line

spot these kind of issues before committing our code, we have

to address every

Quoted 6 lines

Quoted 6 lines

place in the whole code-base.

Quoted 4 lines

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,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

e.g. falltrough()

That'd be useful, but adding all these comments and then

having to chase a

Quoted 1 line

Quoted 1 line

Quoted 1 line

gazillion of warning instances to figure out whether there is

a comment or

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

not is just backwards.

I have run into this before and people find what you suggest

even uglier.

Quoted 9 lines

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

Quoted 4 lines

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.

Quoted 2 lines

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.

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.

The notation in question has been standard in tools like lint since the end of the 1970s

This is what we want to add:

# Warn about missing switch break or fall-through comment. KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough)

Quoted 4 lines

Quoted 4 lines

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. :/

Quoted 3 lines · Alan Cox

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?

Quoted 3 lines · Alan Cox

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?

Quoted 2 lines

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

Quoted 1 line

even funny anymore.

The notation in question has been standard in tools like lint since the end of the 1970s

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 19 lines · Linus Torvalds

Quoted 3 lines · Alan Cox

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

Quoted 5 lines

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.

Quoted 3 lines

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.

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.

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

Quoting Linus Torvalds <[email protected]>:

Quoted 15 lines

Quoted 3 lines · Alan Cox

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

Quoted 9 lines

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.

Quoted 25 lines · Gustavo A. R. Silva

Quoting Linus Torvalds <[email protected]>:

Quoted 14 lines

Quoted 2 lines · Alan Cox

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

Quoted 9 lines

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.

Quoted 5 lines

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

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

Quoted 2 lines

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. :)

Quoting Thomas Gleixner <[email protected]>:

Quoted 4 lines

So I have to ask WHY this information was not in the changelog of the patch in question:

Quoted 4 lines

1) How it works

2) Why comments have been chosen over macros

I will add this info and send the patch again.

Quoted 8 lines

Quoted 2 lines

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. :)

Quoted 18 lines · Gustavo A. R. Silva

Quoting Thomas Gleixner <[email protected]>:

Quoted 8 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 3 lines

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.