[Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

[peter.smith]

Summary

The Unified Arm Assembler Language is designed so that the majority of assembler files can be assembled for both Arm and Thumb with the choice made as a compilation option. The way this is done in gcc is to pass -mthumb to the assembler with either -Wa,-mthumb or -Xassembler -mthumb. This change adds support for these options to clang. There is no assembler equivalent of -mno-thumb, -marm or -mno-arm so we don't need to recognize these.

Ideally we would do all of the processing in CollectArgsForIntegratedAssembler(). Unfortunately we need to change the triple and at that point it is too late. Instead we look for the option earlier in ComputeLLVMTriple().

Fixes PR34519

[compnerd]

lib/Driver/ToolChain.cpp

549–556 ↗

(On Diff #123174)

Why not write this as:

const auto &Values = Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler);
bool IsIntegratedAssemblerThumb =
    std::any_of(std::begin(A->getValues()),
                std::end(A->getValues()),
                [](StringRef Arg) { return Arg == "-mthumb"});

560–564 ↗

(On Diff #123174)

This is horribly complicated to read. Can we just split this out of the condition and create a variable?

[peter.smith]

lib/Driver/ToolChain.cpp

549–556 ↗

(On Diff #123174)

I gave it a try and I don't think it can be done simply as there are two loops and not one. There could be two Args, one for Wa_COMMA and one for X_Assembler. Unless there is a way of combining the iterator ranges into one I don't think this can be done without nested loops. I checked the other uses of Args.filtered and I couldn't find any of use outside of the range for. Please do let me know if I'm missing something though as I'm happy to change it if there is something better.

560–564 ↗

(On Diff #123174)

Agreed. I'll post an alternative that attempts to evaluate -mthumb before the boolean expression.

[peter.smith]

Updated diff with an attempt to simplify the check for filetype and mthumb. I've left the existing Args.filtered in expression for now as I couldn't make a better alternative with std::for_any.

[compnerd]

Would be nice to rename the variable prior to commit.

lib/Driver/ToolChain.cpp

547 ↗

(On Diff #123310)

I think that this should be IsThumb. The -m is just an indicator that this is a machine flag.

549–556 ↗

(On Diff #123174)

I should've hoisted the ->getValues(), but I suspect that would work. But that is a slight tweak, not anything to hold this up on.

[peter.smith]

In D40127#929578, @compnerd wrote:

Would be nice to rename the variable prior to commit.

Thanks for the review, I've renamed the variable as suggested.

cfe/trunk/lib/Driver/ToolChain.cpp

case llvm::Triple::thumbeb: {
     if (IsMProfile && ARMModeRequested) {
       if (!MCPU.empty())
         getDriver().Diag(diag::err_cpu_unsupported_isa) << CPU << "ARM";
        else
         getDriver().Diag(diag::err_arch_unsupported_isa)
           << tools::arm::getARMArch(MArch, getTriple()) << "ARM";
     }
 
-    // Assembly files should start in ARM mode, unless arch is M-profile.
-    // Windows is always thumb.
-    if ((InputType != types::TY_PP_Asm && Args.hasFlag(options::OPT_mthumb,
-         options::OPT_mno_thumb, ThumbDefault)) || IsMProfile ||
-         getTriple().isOSWindows()) {
+    // Check to see if an explicit choice to use thumb has been made via
+    // -mthumb. For assembler files we must check for -mthumb in the options
+    // passed to the assember via -Wa or -Xassembler.
+    bool IsThumb = false;
+    if (InputType != types::TY_PP_Asm)
+      IsThumb = Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb,
+                              ThumbDefault);
+    else {
+      // Ideally we would check for these flags in
+      // CollectArgsForIntegratedAssembler but we can't change the ArchName at
+      // that point. There is no assembler equivalent of -mno-thumb, -marm, or
+      // -mno-arm.
+      for (const Arg *A :
+           Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
+        for (StringRef Value : A->getValues()) {
+          if (Value == "-mthumb")
+            IsThumb = true;
+        }
+      }
+    }
+    // Assembly files should start in ARM mode, unless arch is M-profile, or
+    // -mthumb has been passed explicitly to the assembler. Windows is always
+    // thumb.
+    if (IsThumb || IsMProfile || getTriple().isOSWindows()) {
       if (IsBigEndian)
         ArchName = "thumbeb";
       else
         ArchName = "thumb";
     }
     Triple.setArchName(ArchName + Suffix.str());
 
     return Triple.getTriple();

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

for (StringRef Value : A->getValues()) {
 
       if (C.getDefaultToolChain().getTriple().isOSBinFormatCOFF() &&
           Value == "-mbig-obj")
         continue; // LLVM handles bigobj automatically
 
       switch (C.getDefaultToolChain().getArch()) {
       default:
         break;
+      case llvm::Triple::thumb:
+      case llvm::Triple::thumbeb:
+      case llvm::Triple::arm:
+      case llvm::Triple::armeb:
+        if (Value == "-mthumb")
+          // -mthumb has already been processed in ComputeLLVMTriple()
+          // recognize but skip over here.
+          continue;
+
       case llvm::Triple::mips:
       case llvm::Triple::mipsel:
       case llvm::Triple::mips64:
       case llvm::Triple::mips64el:
         if (Value == "--trap") {
           CmdArgs.push_back("-target-feature");
           CmdArgs.push_back("+use-tcc-in-div");
           continue;

cfe/trunk/test/Driver/arm-target-as-mthumb.s

+// Make sure -mthumb does not affect assembler triple, but -Wa,-mthumb or
+// -Xassembler -mthumb does. Also check that -Wa,-mthumb or -Xassembler -mthumb
+// does not affect non assembler files.
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-ARM %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb \
+// RUN: %S/Inputs/wildcard1.c  2>&1 | FileCheck -check-prefix=TRIPLE-ARM %s
+
+// TRIPLE-ARM: "-triple" "armv7--linux-gnueabi"
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-THUMB %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Xassembler -mthumb %s \
+// RUN: 2>&1 | FileCheck -check-prefix=TRIPLE-THUMB %s
+
+// TRIPLE-THUMB: "-triple" "thumbv7--linux-gnueabi"