On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote:

Hi all,

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

In preparation to enable -Wimplicit-fallthrough for Clang, explicitly add multiple break/goto/return/fallthrough statements instead of just letting the code fall through to the next case.

Notice that in order to enable -Wimplicit-fallthrough for Clang, this change[1] is meant to be reverted at some point. So, this patch helps to move in that direction.

This was a bit hard to parse for a second or three.

Thanks Gustavo.

How was this change done?

On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

In preparation to enable -Wimplicit-fallthrough for Clang, explicitly add multiple break/goto/return/fallthrough statements instead of just letting the code fall through to the next case.

Notice that in order to enable -Wimplicit-fallthrough for Clang, this change[1] is meant to be reverted at some point. So, this patch helps to move in that direction.

Something important to mention is that there is currently a discrepancy between GCC and Clang when dealing with switch fall-through to empty case statements or to cases that only contain a break/continue/return statement[2][3][4].

Are we sure we want to make this change? Was it discussed before?

Are there any bugs Clangs puritanical definition of fallthrough helped find?

IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!

Hi,

On 11/20/20 12:53, Jakub Kicinski wrote:

On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:

...

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

In preparation to enable -Wimplicit-fallthrough for Clang, explicitly add multiple break/goto/return/fallthrough statements instead of just letting the code fall through to the next case.

Notice that in order to enable -Wimplicit-fallthrough for Clang, this change[1] is meant to be reverted at some point. So, this patch helps to move in that direction.

Something important to mention is that there is currently a discrepancy between GCC and Clang when dealing with switch fall-through to empty case statements or to cases that only contain a break/continue/return statement[2][3][4].

Are we sure we want to make this change? Was it discussed before?

Are there any bugs Clangs puritanical definition of fallthrough helped find?

IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!

The justification for this is explained in this same changelog text:

Now that the -Wimplicit-fallthrough option has been globally enabled[5], any compiler should really warn on missing either a fallthrough annotation or any of the other case-terminating statements (break/continue/return/ goto) when falling through to the next case statement. Making exceptions to this introduces variation in case handling which may continue to lead to bugs, misunderstandings, and a general lack of robustness. The point of enabling options like -Wimplicit-fallthrough is to prevent human error and aid developers in spotting bugs before their code is even built/ submitted/committed, therefore eliminating classes of bugs. So, in order to really accomplish this, we should, and can, move in the direction of addressing any error-prone scenarios and get rid of the unintentional fallthrough bug-class in the kernel, entirely, even if there is some minor redundancy. Better to have explicit case-ending statements than continue to have exceptions where one must guess as to the right result. The compiler will eliminate any actual redundancy.

Note that there is already a patch in mainline that addresses almost 40,000 of these issues[6].

[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for clang for now") [2] ClangBuiltLinux#636 [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432 [4] https://godbolt.org/z/xgkvIh [5] commit a035d552a93b ("Makefile: Globally enable fall-through warning") [6] commit 4169e889e588 ("include: jhash/signal: Fix fall-through warnings for Clang")

Thanks -- Gustavo

On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:

On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:

...

On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:

...

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

In preparation to enable -Wimplicit-fallthrough for Clang, explicitly add multiple break/goto/return/fallthrough statements instead of just letting the code fall through to the next case.

Notice that in order to enable -Wimplicit-fallthrough for Clang, this change[1] is meant to be reverted at some point. So, this patch helps to move in that direction.

Something important to mention is that there is currently a discrepancy between GCC and Clang when dealing with switch fall-through to empty case statements or to cases that only contain a break/continue/return statement[2][3][4].

Are we sure we want to make this change? Was it discussed before?

Are there any bugs Clangs puritanical definition of fallthrough helped find?

IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!

It's certainly a place where the intent is not always clear. I think this makes all the cases unambiguous, and doesn't impact the machine code, since the compiler will happily optimize away any behavioral redundancy.

If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning.

I think clang is just being annoying here, but if I'm the only one who feels this way chances are I'm wrong :)

Hi Gustavo,

On Fri, Nov 20, 2020 at 7:21 PM Gustavo A. R. Silva gustavoars@kernel.org wrote:

Hi all,

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

Thanks for this.

Since this warning is reliable in both/all compilers and we are eventually getting rid of all the cases, what about going even further and making it an error right after?

Cheers, Miguel

On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:

On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:

...

On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:

...

On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:

...

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

In preparation to enable -Wimplicit-fallthrough for Clang, explicitly add multiple break/goto/return/fallthrough statements instead of just letting the code fall through to the next case.

Notice that in order to enable -Wimplicit-fallthrough for Clang, this change[1] is meant to be reverted at some point. So, this patch helps to move in that direction.

Something important to mention is that there is currently a discrepancy between GCC and Clang when dealing with switch fall-through to empty case statements or to cases that only contain a break/continue/return statement[2][3][4].

Are we sure we want to make this change? Was it discussed before?

Are there any bugs Clangs puritanical definition of fallthrough helped find?

IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!

It's certainly a place where the intent is not always clear. I think this makes all the cases unambiguous, and doesn't impact the machine code, since the compiler will happily optimize away any behavioral redundancy.

If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning.

FWIW, this series has found at least one bug so far: https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=K...

On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:

Please tell me our reward for all this effort isn't a single missing error print.

There were quite literally dozens of logical defects found by the fallthrough additions. Very few were logging only.

On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote:

On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:

...

On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:

...

Please tell me our reward for all this effort isn't a single missing error print.

There were quite literally dozens of logical defects found by the fallthrough additions. Very few were logging only.

So can you give us the best examples (or indeed all of them if someone is keeping score)? hopefully this isn't a US election situation ...

Gustavo? Are you running for congress now?

https://lwn.net/Articles/794944/

On Sun, Nov 22, 2020 at 7:22 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:

Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did.

We've been at this for three years now with nearly a thousand patches, firstly marking all the fall throughs with /* fall through */ and later changing it to fallthrough. At some point we do have to ask if the effort is commensurate with the protection afforded. Please tell me our reward for all this effort isn't a single missing error print.

It isn't that much effort, isn't it? Plus we need to take into account the future mistakes that it might prevent, too. So even if there were zero problems found so far, it is still a positive change.

I would agree if these changes were high risk, though; but they are almost trivial.

Cheers, Miguel

On Sun, 2020-11-22 at 21:35 +0100, Miguel Ojeda wrote:

On Sun, Nov 22, 2020 at 7:22 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:

...

Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did.

We've been at this for three years now with nearly a thousand patches, firstly marking all the fall throughs with /* fall through */ and later changing it to fallthrough. At some point we do have to ask if the effort is commensurate with the protection afforded. Please tell me our reward for all this effort isn't a single missing error print.

It isn't that much effort, isn't it?

Well, it seems to be three years of someone's time plus the maintainer review time and series disruption of nearly a thousand patches. Let's be conservative and assume the producer worked about 30% on the series and it takes about 5-10 minutes per patch to review, merge and for others to rework existing series. So let's say it's cost a person year of a relatively junior engineer producing the patches and say 100h of review and application time. The latter is likely the big ticket item because it's what we have in least supply in the kernel (even though it's 20x vs the producer time).

Plus we need to take into account the future mistakes that it might prevent, too. So even if there were zero problems found so far, it is still a positive change.

Well, the question I was asking is if it's worth the cost which I've tried to outline above.

I would agree if these changes were high risk, though; but they are almost trivial.

It's not about the risk of the changes it's about the cost of implementing them. Even if you discount the producer time (which someone gets to pay for, and if I were the engineering manager, I'd be unhappy about), the review/merge/rework time is pretty significant in exchange for six minor bug fixes. Fine, when a new compiler warning comes along it's certainly reasonable to see if we can benefit from it and the fact that the compiler people think it's worthwhile is enough evidence to assume this initially. But at some point you have to ask whether that assumption is supported by the evidence we've accumulated over the time we've been using it. And if the evidence doesn't support it perhaps it is time to stop the experiment.

James

On Mon, 2020-11-23 at 09:54 +1100, Finn Thain wrote:

But is anyone keeping score of the regressions? If unreported bugs count, what about unreported regressions?

Well, I was curious about the former (obviously no tool will tell me about the latter), so I asked git what patches had a fall-through series named in a fixes tag and these three popped out:

9cf51446e686 bpf, powerpc: Fix misuse of fallthrough in bpf_jit_comp() 6a9dc5fd6170 lib: Revert use of fallthrough pseudo-keyword in lib/ 91dbd73a1739 mips/oprofile: Fix fallthrough placement

I don't think any of these is fixing a significant problem, but they did cause someone time and trouble to investigate.

James

On Sun, Nov 22, 2020 at 11:54 PM Finn Thain fthain@telegraphics.com.au wrote:

We should also take into account optimisim about future improvements in tooling.

Not sure what you mean here. There is no reliable way to guess what the intention was with a missing fallthrough, even if you parsed whitespace and indentation.

It is if you want to spin it that way.

How is that a "spin"? It is a fact that we won't get *implicit* fallthrough mistakes anymore (in particular if we make it a hard error).

But what we inevitably get is changes like this:

case 3: this();

•   break;
  

case 4: hmmm();

Why? Mainly to silence the compiler. Also because the patch author argued successfully that they had found a theoretical bug, often in mature code.

If someone changes control flow, that is on them. Every kernel developer knows what `break` does.

But is anyone keeping score of the regressions? If unreported bugs count, what about unreported regressions?

Introducing `fallthrough` does not change semantics. If you are really keen, you can always compare the objects because the generated code shouldn't change.

Cheers, Miguel

On Mon, 2020-11-23 at 15:19 +0100, Miguel Ojeda wrote:

On Sun, Nov 22, 2020 at 11:36 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:

...

Well, it seems to be three years of someone's time plus the maintainer review time and series disruption of nearly a thousand patches. Let's be conservative and assume the producer worked about 30% on the series and it takes about 5-10 minutes per patch to review, merge and for others to rework existing series. So let's say it's cost a person year of a relatively junior engineer producing the patches and say 100h of review and application time. The latter is likely the big ticket item because it's what we have in least supply in the kernel (even though it's 20x vs the producer time).

How are you arriving at such numbers? It is a total of ~200 trivial lines.

Well, I used git. It says that as of today in Linus' tree we have 889 patches related to fall throughs and the first series went in in october 2017 ... ignoring a couple of outliers back to February.

...

It's not about the risk of the changes it's about the cost of implementing them. Even if you discount the producer time (which someone gets to pay for, and if I were the engineering manager, I'd be unhappy about), the review/merge/rework time is pretty significant in exchange for six minor bug fixes. Fine, when a new compiler warning comes along it's certainly reasonable to see if we can benefit from it and the fact that the compiler people think it's worthwhile is enough evidence to assume this initially. But at some point you have to ask whether that assumption is supported by the evidence we've accumulated over the time we've been using it. And if the evidence doesn't support it perhaps it is time to stop the experiment.

Maintainers routinely review 1-line trivial patches, not to mention internal API changes, etc.

We're also complaining about the inability to recruit maintainers:

https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_t...

And burn out:

http://antirez.com/news/129

The whole crux of your argument seems to be maintainers' time isn't important so we should accept all trivial patches ... I'm pushing back on that assumption in two places, firstly the valulessness of the time and secondly that all trivial patches are valuable.

If some company does not want to pay for that, that's fine, but they don't get to be maintainers and claim `Supported`.

What I'm actually trying to articulate is a way of measuring value of the patch vs cost ... it has nothing really to do with who foots the actual bill.

One thesis I'm actually starting to formulate is that this continual devaluing of maintainers is why we have so much difficulty keeping and recruiting them.

James

On Mon, 2020-11-23 at 07:03 -0600, Gustavo A. R. Silva wrote:

On Sun, Nov 22, 2020 at 11:53:55AM -0800, James Bottomley wrote:

...

On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote:

...

On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote:

...

On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:

...

On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:

...

Please tell me our reward for all this effort isn't a single missing error print.

There were quite literally dozens of logical defects found by the fallthrough additions. Very few were logging only.

So can you give us the best examples (or indeed all of them if someone is keeping score)? hopefully this isn't a US election situation ...

Gustavo? Are you running for congress now?

https://lwn.net/Articles/794944/

That's 21 reported fixes of which about 50% seem to produce no change in code behaviour at all, a quarter seem to have no user visible effect with the remaining quarter producing unexpected errors on obscure configuration parameters, which is why no-one really noticed them before.

The really important point here is the number of bugs this has prevented and will prevent in the future. See an example of this, below:

https://lore.kernel.org/linux-iio/20190813135802.GB27392@kroah.com/

I think this falls into the same category as the other six bugs: it changes the output/input for parameters but no-one has really noticed, usually because the command is obscure or the bias effect is minor.

This work is still relevant, even if the total number of issues/bugs we find in the process is zero (which is not the case).

Really, no ... something which produces no improvement has no value at all ... we really shouldn't be wasting maintainer time with it because it has a cost to merge. I'm not sure we understand where the balance lies in value vs cost to merge but I am confident in the zero value case.

"The sucky thing about doing hard work to deploy hardening is that the result is totally invisible by definition (things not happening) [..]"

• Dmitry Vyukov

Really, no. Something that can't be measured at all doesn't exist.

And actually hardening is one of those things you can measure (which I do have to admit isn't true for everything in the security space) ... it's number of exploitable bugs found before you did it vs number of exploitable bugs found after you did it. Usually hardening eliminates a class of bug, so the way I've measured hardening before is to go through the CVE list for the last couple of years for product X, find all the bugs that are of the class we're looking to eliminate and say if we had hardened X against this class of bug we'd have eliminated Y% of the exploits. It can be quite impressive if Y is a suitably big number.

James

On Mon, Nov 23, 2020 at 4:58 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:

Well, I used git. It says that as of today in Linus' tree we have 889 patches related to fall throughs and the first series went in in october 2017 ... ignoring a couple of outliers back to February.

I can see ~10k insertions over ~1k commits and 15 years that mention a fallthrough in the entire repo. That is including some commits (like the biggest one, 960 insertions) that have nothing to do with C fallthrough. A single kernel release has an order of magnitude more changes than this...

But if we do the math, for an author, at even 1 minute per line change and assuming nothing can be automated at all, it would take 1 month of work. For maintainers, a couple of trivial lines is noise compared to many other patches.

In fact, this discussion probably took more time than the time it would take to review the 200 lines. :-)

We're also complaining about the inability to recruit maintainers:

https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_t...

And burn out:

http://antirez.com/news/129

Accepting trivial and useful 1-line patches is not what makes a voluntary maintainer quit... Thankless work with demanding deadlines is.

The whole crux of your argument seems to be maintainers' time isn't important so we should accept all trivial patches

I have not said that, at all. In fact, I am a voluntary one and I welcome patches like this. It takes very little effort on my side to review and it helps the kernel overall. Paid maintainers are the ones that can take care of big features/reviews.

What I'm actually trying to articulate is a way of measuring value of the patch vs cost ... it has nothing really to do with who foots the actual bill.

I understand your point, but you were the one putting it in terms of a junior FTE. In my view, 1 month-work (worst case) is very much worth removing a class of errors from a critical codebase.

One thesis I'm actually starting to formulate is that this continual devaluing of maintainers is why we have so much difficulty keeping and recruiting them.

That may very well be true, but I don't feel anybody has devalued maintainers in this discussion.

Cheers, Miguel

On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote:

On Mon, Nov 23, 2020 at 4:58 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:

...

Well, I used git. It says that as of today in Linus' tree we have 889 patches related to fall throughs and the first series went in in october 2017 ... ignoring a couple of outliers back to February.

I can see ~10k insertions over ~1k commits and 15 years that mention a fallthrough in the entire repo. That is including some commits (like the biggest one, 960 insertions) that have nothing to do with C fallthrough. A single kernel release has an order of magnitude more changes than this...

But if we do the math, for an author, at even 1 minute per line change and assuming nothing can be automated at all, it would take 1 month of work. For maintainers, a couple of trivial lines is noise compared to many other patches.

So you think a one line patch should take one minute to produce ... I really don't think that's grounded in reality. I suppose a one line patch only takes a minute to merge with b4 if no-one reviews or tests it, but that's not really desirable.

In fact, this discussion probably took more time than the time it would take to review the 200 lines. :-)

I'm framing the discussion in terms of the whole series of changes we have done for fall through, both what's in the tree currently (889 patches) both in terms of the produce and the consumer. That's what I used for my figures for cost.

...

We're also complaining about the inability to recruit maintainers:

https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_t...

And burn out:

http://antirez.com/news/129

Accepting trivial and useful 1-line patches

Part of what I'm trying to measure is the "and useful" bit because that's not a given.

is not what makes a voluntary maintainer quit...

so the proverb "straw which broke the camel's back" uniquely doesn't apply to maintainers

Thankless work with demanding deadlines is.

That's another potential reason, but it doesn't may other reasons less valid.

...

The whole crux of your argument seems to be maintainers' time isn't important so we should accept all trivial patches

I have not said that, at all. In fact, I am a voluntary one and I welcome patches like this. It takes very little effort on my side to review and it helps the kernel overall.

Well, you know, subsystems are very different in terms of the amount of patches a maintainer has to process per release cycle of the kernel. If a maintainer is close to capacity, additional patches, however trivial, become a problem. If a maintainer has spare cycles, trivial patches may look easy.

Paid maintainers are the ones that can take care of big features/reviews.

...

What I'm actually trying to articulate is a way of measuring value of the patch vs cost ... it has nothing really to do with who foots the actual bill.

I understand your point, but you were the one putting it in terms of a junior FTE.

No, I evaluated the producer side in terms of an FTE. What we're mostly arguing about here is the consumer side: the maintainers and people who have to rework their patch sets. I estimated that at 100h.

In my view, 1 month-work (worst case) is very much worth removing a class of errors from a critical codebase.

...

One thesis I'm actually starting to formulate is that this continual devaluing of maintainers is why we have so much difficulty keeping and recruiting them.

That may very well be true, but I don't feel anybody has devalued maintainers in this discussion.

You seem to be saying that because you find it easy to merge trivial patches, everyone should. I'm reminded of a friend long ago who thought being a Tees River Pilot was a sinecure because he could navigate the Tees blindfold. What he forgot, of course, is that just because it's easy with a trawler doesn't mean it's easy with an oil tanker. In fact it takes longer to qualify as a Tees River Pilot than it does to get a PhD.

James

On Sun, Nov 22, 2020 at 10:26:09AM +0100, Jens Wiklander wrote:

On Fri, Nov 20, 2020 at 7:33 PM Gustavo A. R. Silva gustavoars@kernel.org wrote:

...

In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement instead of letting the code fall through to the next case.

Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva gustavoars@kernel.org

---

drivers/tee/tee_core.c | 1 + 1 file changed, 1 insertion(+)

Acked-by: Jens Wiklander jens.wiklander@linaro.org

Thanks, Jens. -- Gustavo

On Tue, 2020-11-24 at 11:58 +1100, Finn Thain wrote:

it's not for me to prove that such patches don't affect code generation. That's for the patch author and (unfortunately) for reviewers.

Ideally, that proof would be provided by the compilation system itself and not patch authors nor reviewers nor maintainers.

Unfortunately gcc does not guarantee repeatability or deterministic output. To my knowledge, neither does clang.

On Mon, 23 Nov 2020 17:32:51 -0800 Nick Desaulniers wrote:

On Sun, Nov 22, 2020 at 8:17 AM Kees Cook keescook@chromium.org wrote:

...

On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:

...

If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning.

FWIW, this series has found at least one bug so far: https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=K...

So looks like the bulk of these are: switch (x) { case 0: ++x; default: break; }

I have a patch that fixes those up for clang: https://reviews.llvm.org/D91895

😍

On Mon, Nov 23, 2020 at 04:03:45PM -0400, Jason Gunthorpe wrote:

On Fri, Nov 20, 2020 at 12:21:39PM -0600, Gustavo A. R. Silva wrote:

...

IB/hfi1: Fix fall-through warnings for Clang IB/mlx4: Fix fall-through warnings for Clang IB/qedr: Fix fall-through warnings for Clang RDMA/mlx5: Fix fall-through warnings for Clang

I picked these four to the rdma tree, thanks

Awesome. :)

Thank you, Jason. -- Gustavo

On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:

On Sun, Nov 22, 2020 at 8:17 AM Kees Cook keescook@chromium.org wrote:

...

On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:

...

If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning.

FWIW, this series has found at least one bug so far: https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=K...

So looks like the bulk of these are: switch (x) { case 0: ++x; default: break; }

I have a patch that fixes those up for clang: https://reviews.llvm.org/D91895

I still think this isn't right -- it's a case statement that runs off the end without an explicit flow control determination. I think Clang is right to warn for these, and GCC should also warn.

On Tue, 24 Nov 2020, Kees Cook wrote:

On Mon, Nov 23, 2020 at 08:31:30AM -0800, James Bottomley wrote:

...

Really, no ... something which produces no improvement has no value at all ... we really shouldn't be wasting maintainer time with it because it has a cost to merge. I'm not sure we understand where the balance lies in value vs cost to merge but I am confident in the zero value case.

What? We can't measure how many future bugs aren't introduced because the kernel requires explicit case flow-control statements for all new code.

These statements are not "missing" unless you presume that code written before the latest de facto language spec was written should somehow be held to that spec.

If the 'fallthrough' statement is not part of the latest draft spec then we should ask why not before we embrace it. Being that the kernel still prefers -std=gnu89 you might want to consider what has prevented -std=gnu99 or -std=gnu2x etc.

We already enable -Wimplicit-fallthrough globally, so that's not the discussion. The issue is that Clang is (correctly) even more strict than GCC for this, so these are the remaining ones to fix for full Clang coverage too.

Seems to me you should be patching the compiler.

When you have consensus among the language lawyers you'll have more credibility with those being subjected to enforcement.

On Tue, Nov 24, 2020 at 1:58 AM Finn Thain fthain@telegraphics.com.au wrote:

What I meant was that you've used pessimism as if it was fact.

"future mistakes that it might prevent" is neither pessimism nor states a fact.

For example, "There is no way to guess what the effect would be if the compiler trained programmers to add a knee-jerk 'break' statement to avoid a warning".

It is only knee-jerk if you think you are infallible.

Moreover, what I meant was that preventing programmer mistakes is a problem to be solved by development tools

This warning comes from a development tool -- the compiler.

The idea that retro-fitting new language constructs onto mature code is somehow necessary to "prevent future mistakes" is entirely questionable.

The kernel is not a frozen codebase.

Further, "mature code vs. risk of change" arguments don't apply here because the semantics of the program and binary output isn't changing.

Sure. And if you put -Wimplicit-fallthrough into the Makefile and if that leads to well-intentioned patches that cause regressions, it is partly on you.

Again: adding a `fallthrough` does not change the program semantics. If you are a maintainer and want to cross-check, compare the codegen.

Have you ever considered the overall cost of the countless -Wpresume-incompetence flags?

Yeah: negative. On the other hand, the overall cost of the countless -fI-am-infallible flags is very noticeable.

Perhaps you pay the power bill for a build farm that produces logs that no-one reads? Perhaps you've run git bisect, knowing that the compiler messages are not interesting? Or compiled software in using a language that generates impenetrable messages? If so, here's a tip:

# grep CFLAGS /etc/portage/make.conf CFLAGS="... -Wno-all -Wno-extra ..." CXXFLAGS="${CFLAGS}"

Now allow me some pessimism: the hardware upgrades, gigawatt hours and wait time attributable to obligatory static analyses are a net loss.

If you really believe compiler warnings and static analysis are useless and costly, I think there is not much point in continuing the discussion.

No, it's not for me to prove that such patches don't affect code generation. That's for the patch author and (unfortunately) for reviewers.

I was not asking you to prove it. I am stating that proving it is very easy.

Cheers, Miguel

On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote:

On Mon, Nov 23, 2020 at 08:31:30AM -0800, James Bottomley wrote:

...

Really, no ... something which produces no improvement has no value at all ... we really shouldn't be wasting maintainer time with it because it has a cost to merge. I'm not sure we understand where the balance lies in value vs cost to merge but I am confident in the zero value case.

What? We can't measure how many future bugs aren't introduced because the kernel requires explicit case flow-control statements for all new code.

No but we can measure how vulnerable our current coding habits are to the mistake this warning would potentially prevent. I don't think it's wrong to extrapolate that if we had no instances at all of prior coding problems we likely wouldn't have any in future either making adopting the changes needed to enable the warning valueless ... that's the zero value case I was referring to above.

Now, what we have seems to be about 6 cases (at least what's been shown in this thread) where a missing break would cause potentially user visible issues. That means the value of this isn't zero, but it's not a no-brainer massive win either. That's why I think asking what we've invested vs the return isn't a useless exercise.

We already enable -Wimplicit-fallthrough globally, so that's not the discussion. The issue is that Clang is (correctly) even more strict than GCC for this, so these are the remaining ones to fix for full Clang coverage too.

People have spent more time debating this already than it would have taken to apply the patches. :)

You mean we've already spent 90% of the effort to come this far so we might as well go the remaining 10% because then at least we get some return? It's certainly a clinching argument in defence procurement ...

This is about robustness and language wrangling. It's a big code- base, and this is the price of our managing technical debt for permanent robustness improvements. (The numbers I ran from Gustavo's earlier patches were that about 10% of the places adjusted were identified as legitimate bugs being fixed. This final series may be lower, but there are still bugs being found from it -- we need to finish this and shut the door on it for good.)

I got my six patches by analyzing the lwn.net report of the fixes that was cited which had 21 of which 50% didn't actually change the emitted code, and 25% didn't have a user visible effect.

But the broader point I'm making is just because the compiler people come up with a shiny new warning doesn't necessarily mean the problem it's detecting is one that causes us actual problems in the code base. I'd really be happier if we had a theory about what classes of CVE or bug we could eliminate before we embrace the next new warning.

James

On Mon, Nov 23, 2020 at 10:39 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:

On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote:

...

On Mon, Nov 23, 2020 at 4:58 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:

...

...

But if we do the math, for an author, at even 1 minute per line change and assuming nothing can be automated at all, it would take 1 month of work. For maintainers, a couple of trivial lines is noise compared to many other patches.

So you think a one line patch should take one minute to produce ... I really don't think that's grounded in reality. I suppose a one line patch only takes a minute to merge with b4 if no-one reviews or tests it, but that's not really desirable.

In my practice most of the one line patches were either to fix or to introduce quite interesting issues. 1 minute is 2-3 orders less than usually needed for such patches. That's why I don't like churn produced by people who often even didn't compile their useful contributions.

On Wed, 25 Nov 2020 04:24:27 -0800 Nick Desaulniers wrote:

I even agree that most of the churn comes from

case 0: ++x; default: break;

And just to spell it out,

case ENUM_VALUE1: bla(); break; case ENUM_VALUE2: bla(); default: break;

is a fairly idiomatic way of indicating that not all values of the enum are expected to be handled by the switch statement.

I really hope the Clang folks are reasonable and merge your patch.

If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)?

The burn out point is about enjoying your work and feeling that it matters. It really doesn't make much difference if you're doing something you don't like for 12 hours every day or only in shifts with another maintainer. You'll dislike it either way.

Applying a real patch set and then getting a few follow ups the next day for trivial coding things like fallthrough missing or static missing, just because I didn't have the full range of compilers to check with before applying makes me feel pretty shitty, like I'm not doing a good job. YMMV.

On Tue, Nov 24, 2020 at 11:05:35PM -0800, James Bottomley wrote:

Now, what we have seems to be about 6 cases (at least what's been shown in this thread) where a missing break would cause potentially user visible issues. That means the value of this isn't zero, but it's not a no-brainer massive win either. That's why I think asking what we've invested vs the return isn't a useless exercise.

The number is much higher[1]. If it were 6 in the entire history of the kernel, I would agree with you. :) Some were fixed _before_ Gustavo's effort too, which I also count towards the idea of "this is a dangerous weakness in C, and now we have stopped it forever."

But the broader point I'm making is just because the compiler people come up with a shiny new warning doesn't necessarily mean the problem it's detecting is one that causes us actual problems in the code base. I'd really be happier if we had a theory about what classes of CVE or bug we could eliminate before we embrace the next new warning.

But we did! It was long ago justified and documented[2], and even links to the CWE[3] for it. This wasn't random joy over discovering a new warning we could turn on, this was turning on a warning that the compiler folks finally gave us to handle an entire class of flaws. If we need to update the code-base to address it not a useful debate -- that was settled already, even if you're only discovering it now. :P. This last patch set is about finishing that work for Clang, which is correctly even more strict than GCC.

-Kees

[1] https://outflux.net/slides/2019/lss/kspp.pdf calls out specific numbers (about 6.5% of the patches fixed missing breaks): v4.19: 3 of 129 v4.20: 2 of 59 v5.0: 3 of 56 v5.1: 10 of 100 v5.2: 6 of 71 v5.3: 7 of 69

And in the history of the kernel, it's been an ongoing source of flaws:

$ l --no-merges | grep -i 'missing break' | wc -l 185

The frequency of such errors being "naturally" found was pretty steady until the static checkers started warning, and then it was on the rise, but the full effort flushed the rest out, and now it's dropped to almost zero:

1 v2.6.12 3 v2.6.16.28 1 v2.6.17 1 v2.6.19 2 v2.6.21 1 v2.6.22 3 v2.6.24 3 v2.6.29 1 v2.6.32 1 v2.6.33 1 v2.6.35 4 v2.6.36 3 v2.6.38 2 v2.6.39 7 v3.0 2 v3.1 2 v3.2 2 v3.3 3 v3.4 1 v3.5 8 v3.6 7 v3.7 3 v3.8 6 v3.9 3 v3.10 2 v3.11 5 v3.12 5 v3.13 2 v3.14 4 v3.15 2 v3.16 3 v3.17 2 v3.18 2 v3.19 1 v4.0 2 v4.1 5 v4.2 4 v4.5 5 v4.7 6 v4.8 1 v4.9 3 v4.10 2 v4.11 6 v4.12 3 v4.13 2 v4.14 5 v4.15 2 v4.16 7 v4.18 2 v4.19 6 v4.20 3 v5.0 12 v5.1 3 v5.2 4 v5.3 2 v5.4 1 v5.8

And the reason it's fully zero, is because we still have the cases we're cleaning up right now. Even this last one from v5.8 is specifically of the same type this series addresses:

case 4: color_index = TrueCModeIndex; + break; default: return; }

[2] https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-swit...

All switch/case blocks must end in one of:

break; fallthrough; continue; goto <label>; return [expression];

[3] https://cwe.mitre.org/data/definitions/484.html

On Wed, Nov 25, 2020 at 8:24 AM Jakub Kicinski kuba@kernel.org wrote:

Applying a real patch set and then getting a few follow ups the next day for trivial coding things like fallthrough missing or static missing, just because I didn't have the full range of compilers to check with before applying makes me feel pretty shitty, like I'm not doing a good job. YMMV.

I understand. Everyone feels that way, except maybe Bond villains and robots. My advice in that case is don't take it personally. We're working with a language that's more error prone relative to others. While one would like to believe they are flawless, over time they can't beat the aggregate statistics. A balance between Imposter Syndrome and Dunning Kruger is walked by all software developers, and the fear of making mistakes in public is one of the number one reasons folks don't take the plunge contributing to open source software or even the kernel. My advice to them is "don't sweat the small stuff."

On 25/11/2020 00:32, Miguel Ojeda wrote:

I have said *authoring* lines of *this* kind takes a minute per line. Specifically: lines fixing the fallthrough warning mechanically and repeatedly where the compiler tells you to, and doing so full-time for a month.

<snip>

It is useful since it makes intent clear.

To make the intent clear, you have to first be certain that you understand the intent; otherwise by adding either a break or a fallthrough to suppress the warning you are just destroying the information that "the intent of this code is unknown". Figuring out the intent of a piece of unfamiliar code takes more than 1 minute; just because case foo: thing; case bar: break; produces identical code to case foo: thing; break; case bar: break; doesn't mean that *either* is correct — maybe the author meant to write case foo: return thing; case bar: break; and by inserting that break you've destroyed the marker that would direct someone who knew what the code was about to look at that point in the code and spot the problem. Thus, you *always* have to look at more than just the immediate mechanical context of the code, to make a proper judgement that yes, this was the intent. If you think that that sort of thing can be done in an *average* time of one minute, then I hope you stay away from code I'm responsible for! One minute would be an optimistic target for code that, as the maintainer, one is already somewhat familiar with. For code that you're seeing for the first time, as is usually the case with the people doing these mechanical fix-a-warning patches, it's completely unrealistic.

A warning is only useful because it makes you *think* about the code. If you suppress the warning without doing that thinking, then you made the warning useless; and if the warning made you think about code that didn't *need* it, then the warning was useless from the start.

So make your mind up: does Clang's stricter -Wimplicit-fallthrough flag up code that needs thought (in which case the fixes take effort both to author and to review) or does it flag up code that can be mindlessly "fixed" (in which case the warning is worthless)? Proponents in this thread seem to be trying to have it both ways.

-ed

On Wed, 25 Nov 2020, Nick Desaulniers wrote:

On Wed, Nov 25, 2020 at 1:33 PM Finn Thain fthain@telegraphics.com.au wrote:

...

Or do you think that a codebase can somehow satisfy multiple checkers and their divergent interpretations of the language spec?

Have we found any cases yet that are divergent? I don't think so.

There are many implementations, so I think you are guaranteed to find more divergence if you look. That's because the spec is full of language like this: "implementations are encouraged not to emit a diagnostic" and "implementations are encouraged to issue a diagnostic".

Some implementations will decide to not emit (under the premise that vast amounts of existing code would have to get patched until the compiler goes quiet) whereas other implementations will decide to emit (under the premise that the author is doing the checking and not the janitor or the packager).

It sounds to me like GCC's cases it warns for is a subset of Clang's. Having additional coverage with Clang then should ensure coverage for both.

If that claim were true, the solution would be simple. (It's not.)

For the benefit of projects that enable -Werror and projects that nominated gcc as their preferred compiler, clang would simply need a flag to enable conformance with gcc by suppressing those additional warnings that clang would normally produce.

This simple solution is, of course, completely unworkable, since it would force clang to copy some portion of gcc's logic (rewritten under LLVM's unique license) and then to track future changes to that portion of gcc indefinitely.

On Wed, Nov 25, 2020 at 11:44 PM Edward Cree ecree.xilinx@gmail.com wrote:

To make the intent clear, you have to first be certain that you understand the intent; otherwise by adding either a break or a fallthrough to suppress the warning you are just destroying the information that "the intent of this code is unknown".

If you don't know what the intent of your own code is, then you *already* have a problem in your hands.

Figuring out the intent of a piece of unfamiliar code takes more than 1 minute; just because case foo: thing; case bar: break; produces identical code to case foo: thing; break; case bar: break; doesn't mean that *either* is correct — maybe the author meant

What takes 1 minute is adding it *mechanically* by the author, i.e. so that you later compare whether codegen is the same.

to write case foo: return thing; case bar: break; and by inserting that break you've destroyed the marker that would direct someone who knew what the code was about to look at that point in the code and spot the problem.

Then it means you already have a bug. This patchset gives the maintainer a chance to notice it, which is a good thing. The "you've destroyed the market" claim is bogus, because: 1. you were not looking into it 2. you didn't notice the bug so far 3. is implicit -- harder to spot 4. is only useful if you explicitly take a look at this kind of bug. So why don't you do it now?

Thus, you *always* have to look at more than just the immediate mechanical context of the code, to make a proper judgement that yes, this was the intent.

I find that is the responsibility of the maintainers and reviewers for tree-wide patches like this, assuming they want. They can also keep the behavior (and the bugs) without spending time. Their choice.

If you think that that sort of thing can be done in an *average* time of one minute, then I hope you stay away from code I'm responsible for!

Please don't accuse others of recklessness or incompetence, especially if you didn't understand what they said.

A warning is only useful because it makes you *think* about the code. If you suppress the warning without doing that thinking, then you made the warning useless; and if the warning made you think about code that didn't *need* it, then the warning was useless from the start.

We are not suppressing the warning. Quite the opposite, in fact.

So make your mind up: does Clang's stricter -Wimplicit-fallthrough flag up code that needs thought (in which case the fixes take effort both to author and to review)

As I said several times already, it does take time to review if the maintainer wants to take the chance to see if they had a bug to begin with, but it does not require thought for the author if they just go for equivalent codegen.

or does it flag up code that can be mindlessly "fixed" (in which case the warning is worthless)? Proponents in this thread seem to be trying to have it both ways.

A warning is not worthless just because you can mindlessly fix it. There are many counterexamples, e.g. many checkpatch/lint/lang-format/indentation warnings, functional ones like the `if (a = b)` warning...

Cheers, Miguel

On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven geert@linux-m68k.org wrote:

Hi Miguel,

On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:

...

On Wed, Nov 25, 2020 at 11:44 PM Edward Cree ecree.xilinx@gmail.com wrote:

...

To make the intent clear, you have to first be certain that you understand the intent; otherwise by adding either a break or a fallthrough to suppress the warning you are just destroying the information that "the intent of this code is unknown".

If you don't know what the intent of your own code is, then you *already* have a problem in your hands.

The maintainer is not necessarily the owner/author of the code, and thus may not know the intent of the code.

...

...

or does it flag up code that can be mindlessly "fixed" (in which case the warning is worthless)? Proponents in this thread seem to be trying to have it both ways.

A warning is not worthless just because you can mindlessly fix it. There are many counterexamples, e.g. many checkpatch/lint/lang-format/indentation warnings, functional ones like the `if (a = b)` warning...

BTW, you cannot mindlessly fix the latter, as you cannot know if "(a == b)" or "((a = b))" was intended, without understanding the code (and the (possibly unavailable) data sheet, and the hardware, ...).

to allow assignments in if statements was clearly a mistake and if you need outside information to understand the code, your code is the issue already.

P.S. So far I've stayed out of this thread, as I like it if the compiler flags possible mistakes. After all I was the one fixing new "may be used uninitialized" warnings thrown up by gcc-4.1, until (a bit later than) support for that compiler was removed...

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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Gustavo,

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

Applied 20-22,54,120-124 to 5.11/scsi-staging, thanks.

On Sun, Nov 22, 2020 at 08:17:03AM -0800, Kees Cook wrote:

On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:

...

On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:

...

On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:

...

On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:

...

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

In preparation to enable -Wimplicit-fallthrough for Clang, explicitly add multiple break/goto/return/fallthrough statements instead of just letting the code fall through to the next case.

Notice that in order to enable -Wimplicit-fallthrough for Clang, this change[1] is meant to be reverted at some point. So, this patch helps to move in that direction.

Something important to mention is that there is currently a discrepancy between GCC and Clang when dealing with switch fall-through to empty case statements or to cases that only contain a break/continue/return statement[2][3][4].

Are we sure we want to make this change? Was it discussed before?

Are there any bugs Clangs puritanical definition of fallthrough helped find?

IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!

It's certainly a place where the intent is not always clear. I think this makes all the cases unambiguous, and doesn't impact the machine code, since the compiler will happily optimize away any behavioral redundancy.

If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning.

FWIW, this series has found at least one bug so far: https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=K...

This is a fallthrough to a return and not to a break. That should trigger a warning. The fallthrough to a break should not generate a warning.

The bug we're trying to fix is "missing break statement" but if the result of the bug is "we hit a break statement" then now we're just talking about style. GCC should limit itself to warning about potentially buggy code.

regards, dan carpenter

On Fri, 20 Nov 2020 12:21:39 -0600, Gustavo A. R. Silva wrote:

This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.

In preparation to enable -Wimplicit-fallthrough for Clang, explicitly add multiple break/goto/return/fallthrough statements instead of just letting the code fall through to the next case.

[...]

Applied to 5.11/scsi-queue, thanks!

[054/141] target: Fix fall-through warnings for Clang https://git.kernel.org/mkp/scsi/c/492096ecfa39

Hi,

On 4/20/21 15:20, Gustavo A. R. Silva wrote:

Hi all,

Friendly ping: who can take this, please?

Thanks

Gustavo

On 11/23/20 16:55, Gustavo A. R. Silva wrote:

...

On Sun, Nov 22, 2020 at 10:26:09AM +0100, Jens Wiklander wrote:

...

On Fri, Nov 20, 2020 at 7:33 PM Gustavo A. R. Silva gustavoars@kernel.org wrote:

...

In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement instead of letting the code fall through to the next case.

Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva gustavoars@kernel.org

---

drivers/tee/tee_core.c | 1 + 1 file changed, 1 insertion(+)

Acked-by: Jens Wiklander jens.wiklander@linaro.org

I've added this to my -next[1] branch for v5.14.

Thanks -- Gustavo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=...