| Message ID | 20220201232229.2992968-1-nathan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile.extrawarn: Move -Wunaligned-access to W=2 | expand |
On Wed, Feb 2, 2022 at 12:22 AM Nathan Chancellor <nathan@kernel.org> wrote: > > -Wunaligned-access is a new warning in clang that is default enabled for > arm and arm64 under certain circumstances within the clang frontend (see > LLVM commit below). Under an ARCH=arm allmodconfig, there are > 1284 total/70 unique instances of this warning (most of the instances > are in header files), which is quite noisy. > > To keep the build green through CONFIG_WERROR, only allow this warning > with W=2, which seems appropriate according to its description: > "warnings which occur quite often but may still be relevant". > > This intentionally does not use the -Wno-... + -W... pattern that the > rest of the Makefile does because this warning is not enabled for > anything other than certain arm and arm64 configurations within clang so > it should only be "not disabled", rather than explicitly enabled, so > that other architectures are not disturbed by the warning. > > Cc: stable@vger.kernel.org > Link: https://github.com/llvm/llvm-project/commit/35737df4dcd28534bd3090157c224c19b501278a > Signed-off-by: Nathan Chancellor <nathan@kernel.org> The warning seems important enough to be considered for W=1 on 32-bit arm, otherwise the chances of anyone actually fixing drivers for it is relatively slim. Can you point post the (sufficiently trimmed) output that you get with the warning enabled in an allmodconfig build? I'm not sure why this is enabled by default for arm64, which does not have the problem with fixup handlers. Arnd
On Wed, Feb 02, 2022 at 09:12:06AM +0100, Arnd Bergmann wrote: > On Wed, Feb 2, 2022 at 12:22 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > -Wunaligned-access is a new warning in clang that is default enabled for > > arm and arm64 under certain circumstances within the clang frontend (see > > LLVM commit below). Under an ARCH=arm allmodconfig, there are > > 1284 total/70 unique instances of this warning (most of the instances > > are in header files), which is quite noisy. > > > > To keep the build green through CONFIG_WERROR, only allow this warning > > with W=2, which seems appropriate according to its description: > > "warnings which occur quite often but may still be relevant". > > > > This intentionally does not use the -Wno-... + -W... pattern that the > > rest of the Makefile does because this warning is not enabled for > > anything other than certain arm and arm64 configurations within clang so > > it should only be "not disabled", rather than explicitly enabled, so > > that other architectures are not disturbed by the warning. > > > > Cc: stable@vger.kernel.org > > Link: https://github.com/llvm/llvm-project/commit/35737df4dcd28534bd3090157c224c19b501278a > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > The warning seems important enough to be considered for W=1 on 32-bit arm, > otherwise the chances of anyone actually fixing drivers for it is > relatively slim. Fair point, I suppose barely anyone does W=2 builds, which means we might as well just disable it outright. > Can you point post the (sufficiently trimmed) output that you get with > the warning > enabled in an allmodconfig build? Sure thing. Here is the trimmed version: https://gist.github.com/nathanchance/6682e6894f75790059ca698c29212c71/raw/f63d54819afeb96f3ea0bb055096849912ac0185/trimmed.log Here is the full output of 'make ARCH=arm LLVM=1 allmodconfig': https://gist.github.com/nathanchance/6682e6894f75790059ca698c29212c71/raw/f63d54819afeb96f3ea0bb055096849912ac0185/full.log > I'm not sure why this is enabled by default for arm64, which does not have > the problem with fixup handlers. It is not enabled for arm64 for the kernel. If I am reading the commit right, it is only enabled for arm64 when -mno-unaligned-access is passed or building for OpenBSD, which obviously don't apply to the kernel (see AArch64.cpp). For ARM, we see it in the kernel because it is enabled for any version less than 7, according to this block in clang/lib/Driver/ToolChains/Arch/ARM.cpp: } else if (Triple.isOSLinux() || Triple.isOSNaCl() || Triple.isOSWindows()) { if (VersionNum < 7) { Features.push_back("+strict-align"); if (!ForAS) CmdArgs.push_back("-Wunaligned-access"); } There is this comment above this block in the source code: // Assume pre-ARMv6 doesn't support unaligned accesses. // // ARMv6 may or may not support unaligned accesses depending on the // SCTLR.U bit, which is architecture-specific. We assume ARMv6 // Darwin and NetBSD targets support unaligned accesses, and others don't. // // ARMv7 always has SCTLR.U set to 1, but it has a new SCTLR.A bit // which raises an alignment fault on unaligned accesses. Linux // defaults this bit to 0 and handles it as a system-wide (not // per-process) setting. It is therefore safe to assume that ARMv7+ // Linux targets support unaligned accesses. The same goes for NaCl // and Windows. // // The above behavior is consistent with GCC. I notice that CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS under certain conditions in arch/arm/Kconfig. Would it be worth telling clang that it can generate unaligned accesses in those cases via -munaligned-access or would that be too expensive? If we did, these warnings would be eliminated for configs with CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y, then it could be safely placed under W=1. Cheers, Nathan
On Wed, Feb 2, 2022 at 5:26 PM Nathan Chancellor <nathan@kernel.org> wrote: > On Wed, Feb 02, 2022 at 09:12:06AM +0100, Arnd Bergmann wrote: > > On Wed, Feb 2, 2022 at 12:22 AM Nathan Chancellor <nathan@kernel.org> wrote: > > Fair point, I suppose barely anyone does W=2 builds, which means we > might as well just disable it outright. > > > Can you point post the (sufficiently trimmed) output that you get with > > the warning > > enabled in an allmodconfig build? > > Sure thing. > > Here is the trimmed version: > > https://gist.github.com/nathanchance/6682e6894f75790059ca698c29212c71/raw/f63d54819afeb96f3ea0bb055096849912ac0185/trimmed.log > > Here is the full output of 'make ARCH=arm LLVM=1 allmodconfig': > > https://gist.github.com/nathanchance/6682e6894f75790059ca698c29212c71/raw/f63d54819afeb96f3ea0bb055096849912ac0185/full.log Thanks, that does sound useful, and not that hard to fix. Since these only warn about structure definitions, I think in most cases we can either mark the outer structure as aligned or the inner one as unaligned, which will then avoid the warning as well as make the accesses to the inner structures standard conformant. > > I'm not sure why this is enabled by default for arm64, which does not have > > the problem with fixup handlers. > > It is not enabled for arm64 for the kernel. If I am reading the commit > right, it is only enabled for arm64 when -mno-unaligned-access is passed > or building for OpenBSD, which obviously don't apply to the kernel (see > AArch64.cpp). Ok > For ARM, we see it in the kernel because it is enabled for any version less > than 7, according to this block in clang/lib/Driver/ToolChains/Arch/ARM.cpp: > > } else if (Triple.isOSLinux() || Triple.isOSNaCl() || > Triple.isOSWindows()) { > if (VersionNum < 7) { > Features.push_back("+strict-align"); > if (!ForAS) > CmdArgs.push_back("-Wunaligned-access"); > } > > There is this comment above this block in the source code: > > // Assume pre-ARMv6 doesn't support unaligned accesses. > // > // ARMv6 may or may not support unaligned accesses depending on the > // SCTLR.U bit, which is architecture-specific. We assume ARMv6 > // Darwin and NetBSD targets support unaligned accesses, and others don't. This does not match what we do on Linux though: While this is correct on ARMv5 and below, which has there is no support for unaligned access and also on ARMv7, which has limited unaligned access in both LE and BE8 mode, Linux treats ARMv6 the same way as ARMv7 in practice. The reason is that ARMv6 can support unaligned accesses both on little-endian mode and in BE8-mode, but obviously not in ARMv5-style BE32 mode (this would swap the wrong bytes). Linux itself does not run in ARMv6 BE32 mode because that would in turn not work on ARMv7, in addition to not having unaligned accesses. > // ARMv7 always has SCTLR.U set to 1, but it has a new SCTLR.A bit > // which raises an alignment fault on unaligned accesses. Linux > // defaults this bit to 0 and handles it as a system-wide (not > // per-process) setting. It is therefore safe to assume that ARMv7+ > // Linux targets support unaligned accesses. The same goes for NaCl > // and Windows. > // > // The above behavior is consistent with GCC. > > I notice that CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS under certain > conditions in arch/arm/Kconfig. Would it be worth telling clang that it > can generate unaligned accesses in those cases via -munaligned-access or > would that be too expensive? If we did, these warnings would be > eliminated for configs with CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y, > then it could be safely placed under W=1. It's complicated. For ARMv4/v5, we must never set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS because unaligned accesses are not efficient there. For v6 and higher, most unaligned accesses are fast and don't trap, so we set the flag, but there are two conflicting interpretations of what the flag actually means: - traditionally, it was interpreted as allowing architectures to safely pass around misaligned pointers and dereference them with normal load/store instructions, in violation of the ABI. This however does not work when the compiler generates ARMv6/v7/v8 aarch32 ldm/stm/ldrd/strd instructions that trigger an alignment fault, or with recent gcc versions that may cause unintended behavior when passed a misaligned pointer regardless of the architecture - the new interpretation of the flag is that using get_unaligned()/ put_unaligned() for accessing a pointer is turned into a normal load/store instruction, which avoids both problems but requires code changes in some places that rely on direct pointer dereferences. Ard has made some progress converting the remaining instances that get it wrong, but I don't know how many more remain. For the -munaligned-access flag, I did a quick test about the defaults, and found that it does not behave as I would expect, see https://www.godbolt.org/z/dxd9jMs3h : - Building for ARMv6, an unaligned store gets turned into individual byte accesses in both gcc and clang, which matches the -mno-unaligned-access behavior and your exaplanation above. I'm fairly sure this is not what we want though, as the kernel already assumes that it can do unaligned ldr/str, just not ldrd/strd. A little more research would help here regarding why gcc and linux make different assumptions about ARMv6, but I think we actually want to pass -munaligned-access on ARMv6. - For a trivial aligned store, clang and gcc behave differently -- gcc uses strd for a 64-bit store, while clang uses two str. I don't know if this is an intentional difference, but I like the clang behavior here, because this avoids alignment faults in case we do get passed an unaligned pointer, at the expense of potentially wasting a few CPU cycles and some i-cache. Ideally there would be a third compiler flag in addition to -munaligned-access and -mno-unaligned-access that tells the compiler (ideally both gcc and clang of course) that unaligned ldr/str is allowed, but ldrd/strd/ldm/stm must be avoided even for pointers that are assumed to be aligned. For the warning flag, I think we probably want this to be enabled all the time (after we address the known problems), to be sure that this does not cause traps on ARMv5, or on ARMv7 with gcc generating ldrd/strd instructions on misaligned struct members. Arnd
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index d53825503874..5f75fec4d5ac 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -70,6 +70,20 @@ KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN2 +else + +ifdef CONFIG_CC_IS_CLANG +# While this warning is architecture agnostic, it is only default enabled for +# 32-bit and 64-bit ARM under certain conditions within clang: +# +# https://github.com/llvm/llvm-project/commit/35737df4dcd28534bd3090157c224c19b501278a +# +# To allow it to be disabled under a normal build or W=1 but show up under W=2 +# for those configurations, disable it when W=2 is not set, rather than enable +# -Wunaligned-access in the above block explicitly. +KBUILD_CFLAGS += $(call cc-disable-warning, unaligned-access) +endif + endif #
-Wunaligned-access is a new warning in clang that is default enabled for arm and arm64 under certain circumstances within the clang frontend (see LLVM commit below). Under an ARCH=arm allmodconfig, there are 1284 total/70 unique instances of this warning (most of the instances are in header files), which is quite noisy. To keep the build green through CONFIG_WERROR, only allow this warning with W=2, which seems appropriate according to its description: "warnings which occur quite often but may still be relevant". This intentionally does not use the -Wno-... + -W... pattern that the rest of the Makefile does because this warning is not enabled for anything other than certain arm and arm64 configurations within clang so it should only be "not disabled", rather than explicitly enabled, so that other architectures are not disturbed by the warning. Cc: stable@vger.kernel.org Link: https://github.com/llvm/llvm-project/commit/35737df4dcd28534bd3090157c224c19b501278a Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- scripts/Makefile.extrawarn | 14 ++++++++++++++ 1 file changed, 14 insertions(+) base-commit: 26291c54e111ff6ba87a164d85d4a4e134b7315c