On Mon, Aug 12, 2019 at 3:40 PM Joe Perches <joe@xxxxxxxxxxx> 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@xxxxxxxxxxx/ 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? -- Thanks, ~Nick Desaulniers
