Re: [PATCH] Makefile.extrawarn: Move -Wunaligned-access to W=2

[Arnd Bergmann <arnd@arndb.de>]

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