On Wed, Feb 2, 2022 at 5:26 PM Nathan Chancellor <nathan@xxxxxxxxxx> 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@xxxxxxxxxx> 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
