Miguel Ojeda
2020-Nov-22 20:35 UTC
[Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 7:22 PM James Bottomley <James.Bottomley at 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
James Bottomley
2020-Nov-22 22:36 UTC
[Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
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 at 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
Finn Thain
2020-Nov-22 22:54 UTC
[Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 22 Nov 2020, Miguel Ojeda wrote:> > It isn't that much effort, isn't it? Plus we need to take into account > the future mistakes that it might prevent, too.We should also take into account optimisim about future improvements in tooling.> So even if there were zero problems found so far, it is still a positive > change. >It is if you want to spin it that way.> I would agree if these changes were high risk, though; but they are > almost trivial. >This is trivial: case 1: this(); + fallthrough; case 2: that(); 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. But is anyone keeping score of the regressions? If unreported bugs count, what about unreported regressions?> Cheers, > Miguel >
James Bottomley
2020-Nov-22 23:04 UTC
[Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
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
Miguel Ojeda
2020-Nov-23 14:05 UTC
[Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 11:54 PM Finn Thain <fthain at 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