[AArch64][ARM] add -Wunaligned-access only for clang

[lenary]

I don't fully understand the reasoning for the patch, and you haven't really explained it. I think what you are saying is that the IsAux argument to getTargetFeatures should be considered because it's true for offloading to another compiler, but I don't understand why we think the offload compiler is not clang-compatible, as the features added in getAArch64TargetFeatures and getARMTargetFeatures (and passed to the offloaded compiler) use the LLVM-specific internal names. I would like you to explain this further, and to document what IsAux implies in the code.

That all said code given here is not equivalent to the feature as originally landed, because you've only re-added the -Wunaligned-access flag for the AArch64 target - please make the same addition in Clang::AddARMTargetArgs as we want the warning on both. I think this also makes CmdArgs unused in both aarch64::getAArch64TargetFeatures and arm::getARMTargetFeatures as I think that you don't actually want us adding to CmdArgs in those functions (again, I hope the full reasoning behind this will be explained when you explain why the whole patch is necessary).

[ychen]

• add ARM64 tests

[ychen]

In D119301#3309140, @lenary wrote:

I don't fully understand the reasoning for the patch, and you haven't really explained it. I think what you are saying is that the IsAux argument to getTargetFeatures should be considered because it's true for offloading to another compiler, but I don't understand why we think the offload compiler is not clang-compatible, as the features added in getAArch64TargetFeatures and getARMTargetFeatures (and passed to the offloaded compiler) use the LLVM-specific internal names. I would like you to explain this further, and to document what IsAux implies in the code.

Apologies for the lack of explanation. My point is that getAArch64TargetFeatures / getARMTargetFeatures should only get the TargetFeatures, not adding compiler flags as a side effect, which belongs to clang's job construction. This makes code reuse harder.

That all said code given here is not equivalent to the feature as originally landed, because you've only re-added the -Wunaligned-access flag for the AArch64 target - please make the same addition in Clang::AddARMTargetArgs as we want the warning on both. I think this also makes CmdArgs unused in both aarch64::getAArch64TargetFeatures and arm::getARMTargetFeatures as I think that you don't actually want us adding to CmdArgs in those functions (again, I hope the full reasoning behind this will be explained when you explain why the whole patch is necessary).

Will do the same thing for Clang::AddARMTargetArgs. Thanks. I've tested this patch but not sure why the original test still passing without a Clang::AddARMTargetArgs change.

[ychen]

• update

[ychen]

In D119301#3309253, @ychen wrote:

In D119301#3309140, @lenary wrote:

I don't fully understand the reasoning for the patch, and you haven't really explained it. I think what you are saying is that the IsAux argument to getTargetFeatures should be considered because it's true for offloading to another compiler, but I don't understand why we think the offload compiler is not clang-compatible, as the features added in getAArch64TargetFeatures and getARMTargetFeatures (and passed to the offloaded compiler) use the LLVM-specific internal names. I would like you to explain this further, and to document what IsAux implies in the code.

Apologies for the lack of explanation. My point is that getAArch64TargetFeatures / getARMTargetFeatures should only get the TargetFeatures, not adding compiler flags as a side effect, which belongs to clang's job construction. This makes code reuse harder.

That all said code given here is not equivalent to the feature as originally landed, because you've only re-added the -Wunaligned-access flag for the AArch64 target - please make the same addition in Clang::AddARMTargetArgs as we want the warning on both. I think this also makes CmdArgs unused in both aarch64::getAArch64TargetFeatures and arm::getARMTargetFeatures as I think that you don't actually want us adding to CmdArgs in those functions (again, I hope the full reasoning behind this will be explained when you explain why the whole patch is necessary).

Will do the same thing for Clang::AddARMTargetArgs. Thanks. I've tested this patch but not sure why the original test still passing without a Clang::AddARMTargetArgs change.

NVM. I see why: there were no driver tests. I'll add those.

[ychen]

• update

[ychen]

• Handle ARM
• Add driver tests

[lenary]

Ah, I see.

Please can you also remove the CmdArgs parameter from arm::getARMTargetFeatures when you commit this? This would be useful, along with a comment at the top of getTargetFeatures to discourage targets adding to or using CmdArgs in their get<Target>TargetFeatures functions.

[ychen]

• Remove CmdArgs from arm::getARMTargetFeatures

clang/lib/Driver/ToolChains/Arch/AArch64.h

 
 namespace clang {
 namespace driver {
 namespace tools {
 namespace aarch64 {
 
 void getAArch64TargetFeatures(const Driver &D, const llvm::Triple &Triple,
                               const llvm::opt::ArgList &Args,
-                              llvm::opt::ArgStringList &CmdArgs,
                               std::vector<llvm::StringRef> &Features,
                               bool ForAS);
 
 std::string getAArch64TargetCPU(const llvm::opt::ArgList &Args,
                                 const llvm::Triple &Triple, llvm::opt::Arg *&A);
 
 } // end namespace aarch64
 } // end namespace target
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_AARCH64_H

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

if (!DecodeAArch64Mcpu(D, McpuLowerCase, CPU, DecodedFeature))
     return false;
 
   return getAArch64MicroArchFeaturesFromMtune(D, CPU, Args, Features);
 }
 
 void aarch64::getAArch64TargetFeatures(const Driver &D,
                                        const llvm::Triple &Triple,
                                        const ArgList &Args,
-                                       llvm::opt::ArgStringList &CmdArgs,
                                        std::vector<StringRef> &Features,
                                        bool ForAS) {
   Arg *A;
   bool success = true;
   // Enable NEON by default.
   Features.push_back("+neon");
   llvm::StringRef WaMArch;
   if (ForAS)
const char *Archs[] = {"+v8.6a", "+v8.7a", "+v8.8a",
                          "+v9.1a", "+v9.2a", "+v9.3a"};
   auto Pos = std::find_first_of(Features.begin(), Features.end(),
                                 std::begin(Archs), std::end(Archs));
   if (Pos != std::end(Features))
     Pos = Features.insert(std::next(Pos), {"+i8mm", "+bf16"});
 
   if (Arg *A = Args.getLastArg(options::OPT_mno_unaligned_access,
                                options::OPT_munaligned_access)) {
-    if (A->getOption().matches(options::OPT_mno_unaligned_access)) {
+    if (A->getOption().matches(options::OPT_mno_unaligned_access))
       Features.push_back("+strict-align");
-      if (!ForAS)
-        CmdArgs.push_back("-Wunaligned-access");
-    }
-  } else if (Triple.isOSOpenBSD()) {
+  } else if (Triple.isOSOpenBSD())
     Features.push_back("+strict-align");
-    if (!ForAS)
-      CmdArgs.push_back("-Wunaligned-access");
-  }
 
   if (Args.hasArg(options::OPT_ffixed_x1))
     Features.push_back("+reserve-x1");
 
   if (Args.hasArg(options::OPT_ffixed_x2))
     Features.push_back("+reserve-x2");
 
   if (Args.hasArg(options::OPT_ffixed_x3))

clang/lib/Driver/ToolChains/Arch/ARM.h

void setArchNameInTriple(const Driver &D, const llvm::opt::ArgList &Args,
                          types::ID InputType, llvm::Triple &Triple);
 
 bool useAAPCSForMachO(const llvm::Triple &T);
 void getARMArchCPUFromArgs(const llvm::opt::ArgList &Args,
                            llvm::StringRef &Arch, llvm::StringRef &CPU,
                            bool FromAs = false);
 void getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                           const llvm::opt::ArgList &Args,
-                          llvm::opt::ArgStringList &CmdArgs,
                           std::vector<llvm::StringRef> &Features, bool ForAS);
 int getARMSubArchVersionNumber(const llvm::Triple &Triple);
 bool isARMMProfile(const llvm::Triple &Triple);
 bool isARMAProfile(const llvm::Triple &Triple);
 
 } // end namespace arm
 } // end namespace tools
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_ARM_H

clang/lib/Driver/ToolChains/Arch/ARM.cpp

 static bool hasIntegerMVE(const std::vector<StringRef> &F) {
   auto MVE = llvm::find(llvm::reverse(F), "+mve");
   auto NoMVE = llvm::find(llvm::reverse(F), "-mve");
   return MVE != F.rend() &&
          (NoMVE == F.rend() || std::distance(MVE, NoMVE) > 0);
 }
 
 void arm::getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
-                               const ArgList &Args, ArgStringList &CmdArgs,
+                               const ArgList &Args,
                                std::vector<StringRef> &Features, bool ForAS) {
   bool KernelOrKext =
       Args.hasArg(options::OPT_mkernel, options::OPT_fapple_kext);
   arm::FloatABI ABI = arm::getARMFloatABI(D, Triple, Args);
   llvm::Optional<std::pair<const Arg *, StringRef>> WaCPU, WaFPU, WaHDiv,
       WaArch;
 
   // This vector will accumulate features from the architecture
if (Arg *A = Args.getLastArg(options::OPT_mexecute_only, options::OPT_mno_execute_only)) {
         Features.push_back("+execute-only");
       }
     }
   }
 
   // Kernel code has more strict alignment requirements.
   if (KernelOrKext) {
     Features.push_back("+strict-align");
-    if (!ForAS)
-      CmdArgs.push_back("-Wunaligned-access");
   } else if (Arg *A = Args.getLastArg(options::OPT_mno_unaligned_access,
                                       options::OPT_munaligned_access)) {
     if (A->getOption().matches(options::OPT_munaligned_access)) {
       // No v6M core supports unaligned memory access (v6M ARM ARM A3.2).
       if (Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v6m)
         D.Diag(diag::err_target_unsupported_unaligned) << "v6m";
       // v8M Baseline follows on from v6M, so doesn't support unaligned memory
       // access either.
       else if (Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v8m_baseline)
         D.Diag(diag::err_target_unsupported_unaligned) << "v8m.base";
-    } else {
+    } else
       Features.push_back("+strict-align");
-      if (!ForAS)
-        CmdArgs.push_back("-Wunaligned-access");
-    }
   } else {
     // 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.
     int VersionNum = getARMSubArchVersionNumber(Triple);
     if (Triple.isOSDarwin() || Triple.isOSNetBSD()) {
       if (VersionNum < 6 ||
-          Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v6m) {
+          Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v6m)
         Features.push_back("+strict-align");
-        if (!ForAS)
-          CmdArgs.push_back("-Wunaligned-access");
-      }
     } else if (Triple.isOSLinux() || Triple.isOSNaCl() ||
                Triple.isOSWindows()) {
-      if (VersionNum < 7) {
+      if (VersionNum < 7)
         Features.push_back("+strict-align");
-        if (!ForAS)
-          CmdArgs.push_back("-Wunaligned-access");
-      }
-    } else {
+    } else
       Features.push_back("+strict-align");
-      if (!ForAS)
-        CmdArgs.push_back("-Wunaligned-access");
-    }
   }
 
   // llvm does not support reserving registers in general. There is support
   // for reserving r9 on ARM though (defined as a platform-specific register
   // in ARM EABI).
   if (Args.hasArg(options::OPT_ffixed_r9))
     Features.push_back("+reserve-r9");
 

clang/lib/Driver/ToolChains/Clang.cpp

static void getTargetFeatures(const Driver &D, const llvm::Triple &Triple,
   case llvm::Triple::mips64el:
     mips::getMIPSTargetFeatures(D, Triple, Args, Features);
     break;
 
   case llvm::Triple::arm:
   case llvm::Triple::armeb:
   case llvm::Triple::thumb:
   case llvm::Triple::thumbeb:
-    arm::getARMTargetFeatures(D, Triple, Args, CmdArgs, Features, ForAS);
+    arm::getARMTargetFeatures(D, Triple, Args, Features, ForAS);
     break;
 
   case llvm::Triple::ppc:
   case llvm::Triple::ppcle:
   case llvm::Triple::ppc64:
   case llvm::Triple::ppc64le:
     ppc::getPPCTargetFeatures(D, Triple, Args, Features);
     break;
   case llvm::Triple::riscv32:
   case llvm::Triple::riscv64:
     riscv::getRISCVTargetFeatures(D, Triple, Args, Features);
     break;
   case llvm::Triple::systemz:
     systemz::getSystemZTargetFeatures(D, Args, Features);
     break;
   case llvm::Triple::aarch64:
   case llvm::Triple::aarch64_32:
   case llvm::Triple::aarch64_be:
-    aarch64::getAArch64TargetFeatures(D, Triple, Args, CmdArgs, Features,
-                                      ForAS);
+    aarch64::getAArch64TargetFeatures(D, Triple, Args, Features, ForAS);
     break;
   case llvm::Triple::x86:
   case llvm::Triple::x86_64:
     x86::getX86TargetFeatures(D, Triple, Args, Features);
     break;
   case llvm::Triple::hexagon:
     hexagon::getHexagonTargetFeatures(D, Args, Features);
     break;
void RenderARMABI(const Driver &D, const llvm::Triple &Triple,
   } else {
     std::string CPU = getCPUName(D, Args, Triple, /*FromAs*/ false);
     ABIName = llvm::ARM::computeDefaultTargetABI(Triple, CPU).data();
   }
 
   CmdArgs.push_back("-target-abi");
   CmdArgs.push_back(ABIName);
 }
+
+void AddUnalignedAccessWarning(ArgStringList &CmdArgs) {
+  auto StrictAlignIter =
+      std::find_if(CmdArgs.rbegin(), CmdArgs.rend(), [](StringRef Arg) {
+        return Arg == "+strict-align" || Arg == "-strict-align";
+      });
+  if (StrictAlignIter != CmdArgs.rend() &&
+      StringRef(*StrictAlignIter) == "+strict-align")
+    CmdArgs.push_back("-Wunaligned-access");
+}
 }
 
 static void CollectARMPACBTIOptions(const ToolChain &TC, const ArgList &Args,
                                     ArgStringList &CmdArgs, bool isAArch64) {
   const Arg *A = isAArch64
                      ? Args.getLastArg(options::OPT_msign_return_address_EQ,
                                        options::OPT_mbranch_protection_EQ)
                      : Args.getLastArg(options::OPT_mbranch_protection_EQ);
void Clang::AddARMTargetArgs(const llvm::Triple &Triple, const ArgList &Args,
 
   if (Args.getLastArg(options::OPT_mcmse))
     CmdArgs.push_back("-mcmse");
 
   AddAAPCSVolatileBitfieldArgs(Args, CmdArgs);
 
   // Enable/disable return address signing and indirect branch targets.
   CollectARMPACBTIOptions(getToolChain(), Args, CmdArgs, false /*isAArch64*/);
+
+  AddUnalignedAccessWarning(CmdArgs);
 }
 
 void Clang::RenderTargetOptions(const llvm::Triple &EffectiveTriple,
                                 const ArgList &Args, bool KernelOrKext,
                                 ArgStringList &CmdArgs) const {
   const ToolChain &TC = getToolChain();
 
   // Add the target features
if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_mtune_EQ)) {
     else
       TuneCPU = std::string(Name);
 
     if (!TuneCPU.empty()) {
       CmdArgs.push_back("-tune-cpu");
       CmdArgs.push_back(Args.MakeArgString(TuneCPU));
     }
   }
+
+  AddUnalignedAccessWarning(CmdArgs);
 }
 
 void Clang::AddMIPSTargetArgs(const ArgList &Args,
                               ArgStringList &CmdArgs) const {
   const Driver &D = getToolChain().getDriver();
   StringRef CPUName;
   StringRef ABIName;
   const llvm::Triple &Triple = getToolChain().getTriple();

clang/test/Driver/arm-alignment.c

 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
 
 // RUN: %clang -target aarch64-none-gnueabi -mkernel -mno-unaligned-access -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
 
 // RUN: %clang -target aarch64-unknown-openbsd -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
 
-// CHECK-ALIGNED-ARM: "-target-feature" "+strict-align"
-// CHECK-ALIGNED-AARCH64: "-target-feature" "+strict-align"
+// CHECK-ALIGNED-ARM: "-target-feature" "+strict-align" {{.*}} "-Wunaligned-access"
+// CHECK-ALIGNED-AARCH64: "-target-feature" "+strict-align" {{.*}} "-Wunaligned-access"
 
 // Make sure that v6M cores and v8M Baseline always trigger the unsupported
 // aligned accesses error for all supported architecture triples.
 // RUN: not %clang -c -target thumbv6m-none-gnueabi -mcpu=cortex-m0 -munaligned-access %s 2>&1 | \
 // RUN:   FileCheck --check-prefix CHECK-UNALIGN-NOT-SUPPORTED %s
 // RUN: not %clang -c -target thumb-none-gnueabi -mcpu=cortex-m0 -munaligned-access %s 2>&1 | \
 // RUN:   FileCheck --check-prefix CHECK-UNALIGN-NOT-SUPPORTED %s
 // RUN: not %clang -c -target thumbv8m.base-none-gnueabi -munaligned-access %s 2>&1 | \
 // RUN:   FileCheck --check-prefix CHECK-UNALIGN-NOT-SUPPORTED %s
 
 // CHECK-UNALIGN-NOT-SUPPORTED: error: the {{.*}} sub-architecture does not support unaligned accesses