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

[Joe Perches <joe@perches.com>]

On Mon, 2019-08-12 at 16:11 -0700, Nick Desaulniers wrote:
> On Mon, Aug 12, 2019 at 3:40 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2019-08-12 at 15:14 -0700, Nathan Huckleberry wrote:
> > > Clang does not support the use of comments to label
> > > intentional fallthrough. This patch replaces some uses
> > > of comments to attributesto cut down a significant number
> > > of warnings on clang (from ~50000 to ~200). Only comments
> > > in commonly used header files have been replaced.
> > > 
> > > Since there is still quite a bit of noise, this
> > > patch moves -Wimplicit-fallthrough to
> > > Makefile.extrawarn if you are compiling with
> > > clang.
> > 
> > Unmodified clang does not emit this warning without a patch.
> 
> 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.
> 
> > > diff --git a/Makefile b/Makefile
> > []
> > > @@ -846,7 +846,11 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > >  KBUILD_CFLAGS += -Wdeclaration-after-statement
> > > 
> > >  # Warn about unmarked fall-throughs in switch statement.
> > > +# If the compiler is clang, this warning is only enabled if W=1 in
> > > +# Makefile.extrawarn
> > > +ifndef CONFIG_CC_IS_CLANG
> > >  KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> > > +endif
> > 
> > It'd be better to remove CONFIG_CC_IS_CLANG everywhere
> > eventually as it adds complexity and makes .config files
> > not portable to multiple systems.
> > 
> > > diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> > []
> > > @@ -253,4 +253,8 @@
> > >   */
> > >  #define __weak                          __attribute__((__weak__))
> > > 
> > > +#if __has_attribute(fallthrough)
> > > +#define __fallthrough                   __attribute__((fallthrough))
> > 
> > This should be __attribute__((__fallthrough__))
> 
> Agreed.  I think the GCC documentation on attributes had a point about
> why the __ prefix/suffix was important, which is why we went with that
> in Miguel's original patchset.
> 
> > And there is still no agreement about whether this should
> > be #define fallthrough or #define __fallthrough
> > 
> > https://lore.kernel.org/patchwork/patch/1108577/
> > 
> > > diff --git a/include/linux/jhash.h b/include/linux/jhash.h
> > []
> > > @@ -86,19 +86,43 @@ static inline u32 jhash(const void *key, u32 length, u32 initval)
> > []
> > > +     case 12:
> > > +             c += (u32)k[11]<<24;
> > > +             __fallthrough;
> > 
> > You might consider trying out the scripted conversion tool
> > attached to this email:
> > 
> > https://lore.kernel.org/lkml/61ddbb86d5e68a15e24ccb06d9b399bbf5ce2da7.camel@perches.com/
> 
> I guess the thing I'm curious about is why /* fall through */ is being
> used vs __attribute__((__fallthrough__))?  Surely there's some
> discussion someone can point me to?

AFAIK:

It's historic.

https://lkml.org/lkml/2019/8/4/83

coverity and lint do not support __attribute__((__fallthrough__))
but do support /* fallthrough */ comments in their analysis output.

I prefer converting all the comments to a macro / pseudo keyword.

The cvt_style.pl script does a reasonable job of conversion.