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

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

[Nathan Chancellor]

-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(+)

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
 
 #

base-commit: 26291c54e111ff6ba87a164d85d4a4e134b7315c
-- 
2.35.1

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

[Arnd Bergmann]

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

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

[Nathan Chancellor]

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

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

[Arnd Bergmann]

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