Re: [PATCH v2] kbuild: Change fallthrough comments to attributes

[Joe Perches <joe@perches.com>]

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@perches.com/
> 
> 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?