On Mon, 2019-08-12 at 23:33 -0700, Nathan Chancellor wrote: > On Mon, Aug 12, 2019 at 04:11:26PM -0700, Nick Desaulniers wrote: > > Correct, Nathan is currently implementing support for attribute > > fallthrough in Clang in: > > https://reviews.llvm.org/D64838 > > > > I asked him in person to evaluate how many warnings we'd see in an > > arm64 defconfig with his patch applied. There were on the order of > > 50k warnings, mostly from these headers. I asked him to send these > > patches, then land support in the compiler, that way should our CI > > catch fire overnight, we can carry out of tree fixes until they land. > > With the changes here to Makefile.extrawarn, we should not need to > > carry any out of tree patches. > > I think that if we are modifying this callsite to be favorable to clang, > we should consider a straight revert of commit bfd77145f35c ("Makefile: > Convert -Wimplicit-fallthrough=3 to just -Wipmlicit-fallthrough for > clang"). oh bother. > It would save us a change in scripts/Makefile.extrawarn and > tying testing of this warning to W=1 will make the build noisy from > all of the other warnings that we don't care about plus we will need to > revert that change once we have finished the conversion process anyways. > I think it is cleaner to just pass KCFLAGS=-Wimplicit-fallthrough to > make when testing so that just that additional warning appears but > that is obviously subjective. > > > > You might consider trying out the scripted conversion tool > > > attached to this email: > > > > > > https://lore.kernel.org/lkml/61ddbb86d5e68a15e24ccb06d9b399bbf5ce2da7.camel@xxxxxxxxxxx/ > > I gave the script a go earlier today and it does a reasonable job at > convering the comments to the fallthrough keyword. Here is a list of > the warnings I still see in an x86 allyesconfig build with D64838 on > next-20190812: > > https://gist.github.com/ffbd71b48ba197837e1bdd9bb863b85f > I have gone through about 20-30 of them and while there are a few missed > conversion spots (which is obviously fine for a treewide conversion), The _vast_ majority of case /* fallthrough */ style comments in switch blocks are immediately before another case or default The afs ones seem to be because the last comment in the block is not the fallthrough, but a description of the next case; e.g.: from fs/afs/fsclient.c: /* extract the volume name */ case 3: _debug("extract volname"); ret = afs_extract_data(call, true); if (ret < 0) return ret; p = call->buffer; p[call->count] = 0; _debug("volname '%s'", p); afs_extract_to_tmp(call); call->unmarshall++; /* Fall through */ /* extract the offline message length */ case 4: The script modifies a /* fallthrough */ style comment only if the next non-blank line is 'case <foo>' or "default:' There are many other /* fallthrough */ style comments that are not actually fallthroughs or used in switch blocks so this can't really be automated particularly easily. Likely these remainders would have to be converted manually. > the majority of them come from a disagreement between GCC and Clang on > emitting a warning when falling through to a case statement that is > either the last one and empty or simply breaks.. > > Example: https://godbolt.org/z/xgkvIh > > I have more information on our issue tracker if anyone else wants to > take a look: https://github.com/ClangBuiltLinux/linux/issues/636 > > I personally think that GCC is right and Clang should adapt but I don't > know enough about the Clang codebase to know how feasible this is. I think gcc is wrong here and code like switch (foo) { case 1: bar = 1; default: break; } should emit a fallthrough warning. > I just know there will be even more churn than necessary if we have to > annotate all of those places, taking the conversion process from maybe a > release cycle to several. Luckily, there's a list so it's not a hard problem and it's easily scriptable. There are < 350 entries, not many really. btw: What does the 1st column mean? 1 fs/xfs/scrub/agheader.c:89:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough] 3507 include/linux/jhash.h:113:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough] Number of times emitted?
