-Wdeclaration-after-statement in lib/raid6/ with proposed -std=gnu11
nathanchance opened this issue · 14 comments
Linus is proposing switching to -std=gnu11. In my tests, I found this introduced a warning with an arm64 big endian defconfig:
$ git diff HEAD
diff --git a/Makefile b/Makefile
index 289ce2be8032..e8d600be95a4 100644
--- a/Makefile
+++ b/Makefile
@@ -515,7 +515,7 @@ KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
-Werror=implicit-function-declaration -Werror=implicit-int \
-Werror=return-type -Wno-format-security \
- -std=gnu89
+ -std=gnu11 -Wno-shift-count-negative
KBUILD_CPPFLAGS := -D__KERNEL__
KBUILD_AFLAGS_KERNEL :=
KBUILD_CFLAGS_KERNEL :=
$ make -skj"$(nproc)" ARCH=arm64 LLVM=1 mrproper defconfig
$ scripts/config -d CPU_LITTLE_ENDIAN -e CPU_BIG_ENDIAN
$ make -skj"$(nproc)" ARCH=arm64 LLVM=1 olddefconfig lib/raid6/
lib/raid6/recov_neon_inner.c:55:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
vy = vshrq_n_u8(vx, 4);
^
/home/nathan/cbl/toolchains/stow/llvm/2022-02-23_16-14-30-18fa0b15ccf610f34af1231440f89d20cb99e7a0/lib/clang/15.0.0/include/arm_neon.h:25231:14: note: expanded from macro 'vshrq_n_u8'
uint8x16_t __ret; \
^
lib/raid6/recov_neon_inner.c:60:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
vy = vshrq_n_u8(px, 4);
^
/home/nathan/cbl/toolchains/stow/llvm/2022-02-23_16-14-30-18fa0b15ccf610f34af1231440f89d20cb99e7a0/lib/clang/15.0.0/include/arm_neon.h:25231:14: note: expanded from macro 'vshrq_n_u8'
uint8x16_t __ret; \
^
lib/raid6/recov_neon_inner.c:96:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
vy = vshrq_n_u8(vx, 4);
^
/home/nathan/cbl/toolchains/stow/llvm/2022-02-23_16-14-30-18fa0b15ccf610f34af1231440f89d20cb99e7a0/lib/clang/15.0.0/include/arm_neon.h:25231:14: note: expanded from macro 'vshrq_n_u8'
uint8x16_t __ret; \
^
3 warnings generated.
...Looking at arm_neon.h:
#ifdef __LITTLE_ENDIAN__
#define vshrq_n_u8(__p0, __p1) __extension__ ({ \
uint8x16_t __s0 = __p0; \
uint8x16_t __ret; \
__ret = (uint8x16_t) __builtin_neon_vshrq_n_v((int8x16_t)__s0, __p1, 48); \
__ret; \
})
#else
#define vshrq_n_u8(__p0, __p1) __extension__ ({ \
uint8x16_t __s0 = __p0; \
uint8x16_t __rev0; __rev0 = __builtin_shufflevector(__s0, __s0, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0); \
uint8x16_t __ret; \
__ret = (uint8x16_t) __builtin_neon_vshrq_n_v((int8x16_t)__rev0, __p1, 48); \
__ret = __builtin_shufflevector(__ret, __ret, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0); \
__ret; \
})
#endifIt looks like this file is automatically generated, so I am not sure how to make __rev0 appear after __ret.
cc @DavidSpickett for thoughts, as you have helped with ARM LLVM problems before.
I think part of the issue here is that <arm_neon.h> is a system header, which has more relaxed rules for warnings, so we intentionally don't get a warning with "-std=gnu89 -Wdeclaration-after-statement -Wno-gnu" despite the code being in standard c89 without GNU extensions.
It appears that with "-std=gnu11 -Wdeclaration-after-statement", clang incorrectly warns about it even in system headers as this is correct behavior in both c11 and gnu11, but we ask for a warning anyway.
Right. I can see in Sema::ActOnCompoundStmt() that there is a check for the diagnostic being ignored (!Diags.isIgnored(MixedDeclsCodeID, L)), which calls DiagnosticIDs::getDiagnosticSeverity(), which does check if the source location is in a system macro; maybe something about that flow is messed up?
I've filed an upstream LLVM bug: llvm/llvm-project#54062
Why C11? C18 is exactly the same, with just one unused macro that was introduced in C11 removed.
C11 is the newest standard that the oldest supported compiler (gcc-5.1) already understands. I think we can probably just do away with explicitly picking a -std= option and go with the compiler's default, which should be gnu11 or higher for all supported versions of gcc or clang.
This is fixed upstream in clang-15. I've requested a backport to the 14.0.1 release, but that hasn't and might not land (I suspect it will land just fine though).
Any thoughts on how we want to avoid this warning for clang-11 through clang-14 in the kernel sources?
Any thoughts on how we want to avoid this warning for clang-11 through clang-14 in the kernel sources?
Something along the lines of:
diff --git a/lib/raid6/Makefile b/lib/raid6/Makefile
index 45e17619422b..f0a17bc7bd1d 100644
--- a/lib/raid6/Makefile
+++ b/lib/raid6/Makefile
@@ -41,6 +41,12 @@ NEON_FLAGS += -isystem $(shell $(CC) -print-file-name=include)
ifeq ($(ARCH),arm)
NEON_FLAGS += -march=armv7-a -mfloat-abi=softfp -mfpu=neon
endif
+# https://github.com/ClangBuiltLinux/linux/issues/1603
+ifdef CONFIG_CPU_BIG_ENDIAN
+ifeq ($(shell test $(CONFIG_CLANG_VERSION) -lt 140001; echo $$?),0)
+NEON_FLAGS += -Wno-declaration-after-statement
+endif
+endif
CFLAGS_recov_neon_inner.o += $(NEON_FLAGS)
ifeq ($(ARCH),arm64)
CFLAGS_REMOVE_recov_neon_inner.o += -mgeneral-regs-onlyshould work I think? I'll test in a bit, tracking down another regression.
That patch works. As we discussed in the meeting, I will wait to send that until the backport to release/14.x is merged.
I have sent https://lore.kernel.org/r/20220404210804.3537324-1-nathan@kernel.org/ to silence this warning on older versions of clang.
@arndb see @nathanchance 's blurb, specifically
It was initially surprising that clang warned about a macro that
originates from a system header, as this deviates from GCC. The
machinery in clang to ignore warnings in macros from system headers is
relatively new in clang (January 2022). When that change was landed, the
code owner of clang was not confident that clang's diagnostic system
provided enough information to avoid falsely disabling warnings that
originated from user controlled code that was mixed with system macros,
so the default behavior of warning within system macros remained
unchanged.
After writing and sending https://lore.kernel.org/20220404210804.3537324-1-nathan@kernel.org/, I realized that the window of breakage is just clang 14.0.0, as it contains llvm/llvm-project@118f966, which allows -Wdeclaration-after-statement to work with -std=c99 and later. In other words, previous versions of clang will not show -Wdeclaration-after-statement with -std=c99 and later.
I am not really inclined to send a v2 (-lt 140001 -> -eq 140000) since I am guessing there are no groups other than us testing arm64 big endian; even if there are, we can just tell them to upgrade the compiler.
As such, I think we just close this out, thoughts?
Yep, folks using 14.0.0 should move to 14.0.1 (or let us know why they cannot).