On Wed, Feb 2, 2022 at 12:22 AM Nathan Chancellor <[email protected]> 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: [email protected]
> Link: https://github.com/llvm/llvm-project/commit/35737df4dcd28534bd3090157c224c19b501278a
> Signed-off-by: Nathan Chancellor <[email protected]>
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 <[email protected]> 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: [email protected]
> > Link: https://github.com/llvm/llvm-project/commit/35737df4dcd28534bd3090157c224c19b501278a
> > Signed-off-by: Nathan Chancellor <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]> 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