Thumb state not being passed through to LLVM triple when using clang -cc1as

[labrinea]

Summary

When running clang with an arm triple such as '--target=thumbv7m-arm-none-eabi' that has a thumb only CPU by default (cortex-m3), and when using the assembler, the default thumb state of the CPU does not get passed via the triple to LLVM:

$ clang -target thumbv7m-arm-none-eabi -c -v test.s
clang -cc1as ... -triple armv7m-arm-none-eabi ... test.s

[t.p.northover]

This looks mostly fine, just one quick question that shouldn't block anything:

test/Driver/arm-ias-Wa.s

67 ↗

(On Diff #38547)

Is the -arm- vendor intentional (because DS-5 uses that or something)? Fine if so (it could probably do with being tested actually), a bit weird otherwise.

[rengolin]

test/Driver/arm-ias-Wa.s

67 ↗

(On Diff #38547)

No, that seems a typo. I don't think it should stay.

[rengolin]

I wish there was a way to get the info if a target is thumb-only, but this is ok as an intermediate solution. :)

LGTM too, thanks!

[labrinea]

lib/Driver/ToolChain.cpp

485 ↗

(On Diff #38552)

Alternatively we could remove the check of InputType. I am not sure why this and the above comment are present anyway.

[rengolin]

lib/Driver/ToolChain.cpp

485 ↗

(On Diff #38552)

I agree with you, this logic is inconsistent.

Historically, ARM assembly (especially inline assembly) is assumed ARM unless told otherwise. This is why you have to specify ".thumb" and not ".arm". However, that doesn't mean the assembly file *has* to be ARM, even if the arch name is "thumbvN" or if "-mthumb" is specified.

Not even GNU tools behave in such odd ways, so I think we should be consistent. If the triple is thumb, asm is thumb. If we have -mthumb, asm is thumb. If not, it's arm. Precisely how everything else works, so it's only logical that we should remove the InputType check.

[t.p.northover]

lib/Driver/ToolChain.cpp

485 ↗

(On Diff #38552)

I suspect the issue is with our weirdness. When I remove the TY_PP_Asm check "clang -arch armv7 -c tmp.s" defaults to Thumb-mode. I think I'd be lynched by the OS-folks if that changed.

(That's basically the entire content of the Radar Chad referred to in his original addition).

[rengolin]

lib/Driver/ToolChain.cpp

485 ↗

(On Diff #38552)

Well, that's clearly a bug. I'm ok with this going is as it is, but would be good to create a new bug to solve this mess.

[labrinea]

It seems I missed your comments before updating the patch.
@t.p.northover: What is the exact command? "clang -arch armv7 -c tmp.s" didn't work for me.

[rengolin]

lib/Driver/ToolChain.cpp

472 ↗

(On Diff #38558)

You could cache the profile and use it here, too.

test/Driver/arm-ias-Wa.s

75 ↗

(On Diff #38558)

You should also add "armv7m" and check that it defaults to Thumb, no?

[labrinea]

Is this revision valid after all? I am confused by Tim's comment. I did not see any regressions locally.

lib/Driver/ToolChain.cpp

472 ↗

(On Diff #38558)

I don't see any checks based on profile in this line.

test/Driver/arm-ias-Wa.s

75 ↗

(On Diff #38558)

It does default to thumb, if we are happy with this check I can add it.

[t.p.northover]

If you're on Linux or something you need "clang -target x86_64-apple-darwin -arch armv7 -c tmp.s". Another mess I keep meaning to fix.

I suspect the reason for this hack is that we've already changed the triple to "thumbv7-apple-iosN" by this point (because -arch armv7 compiles to Thumb), so it needs undoing for .s files. It might be reasonably easy to push the TY_PP_Asm check back into the Darwin codepath, or it might be horrible. So much has changed since 2011.

[rengolin]

In D14121#276389, @t.p.northover wrote:

If you're on Linux or something you need "clang -target x86_64-apple-darwin -arch armv7 -c tmp.s".

x86_64 + ARMv7? This doesn't make sense... What is this trying to achieve?

I suspect the reason for this hack is that we've already changed the triple to "thumbv7-apple-iosN" by this point (because -arch armv7 compiles to Thumb), so it needs undoing for .s files. It might be reasonably easy to push the TY_PP_Asm check back into the Darwin codepath, or it might be horrible. So much has changed since 2011.

I think so. If Darwin is special, in which it treats "armv7" as Thumb, then it needs special treatment, not the other way around.

Also, Alexandros, it would be good to add the same set of tests for Darwin, to make sure we're not messing up their side.

cheers,
--renato

lib/Driver/ToolChain.cpp

472 ↗

(On Diff #38558)

Sorry, not profile, but ARMTargetParser function.

You have just replaced:

Suffix.startswith("v6m") || Suffix.startswith("v7m") || Suffix.startswith("v7em")

with:

ARM::parseArchProfile(Suffix) == ARM::PK_M

You should also replace:

Suffix.startswith("v7")

with:

ARM::parseArchVersion(Suffix) == 7

test/Driver/arm-ias-Wa.s

75 ↗

(On Diff #38558)

That will give people looking at your commit in the future the idea of what you were trying to achieve.

[labrinea]

In D14121#276389, @t.p.northover wrote:

If you're on Linux or something you need "clang -target x86_64-apple-darwin -arch armv7 -c tmp.s". Another mess I keep meaning to fix.

I suspect the reason for this hack is that we've already changed the triple to "thumbv7-apple-iosN" by this point (because -arch armv7 compiles to Thumb), so it needs undoing for .s files. It might be reasonably easy to push the TY_PP_Asm check back into the Darwin codepath, or it might be horrible. So much has changed since 2011.

I just tried that and I get :
-cc1 -triple thumbv7-apple-ios5.0.0
-cc1as -triple thumbv7-apple-ios5.0.0
which means you were right.

My previous revision gives:
-cc1 -triple thumbv7-apple-ios5.0.0
-cc1as -triple armv7-apple-ios5.0.0
so I think it should be ok.

[labrinea]

I think the patch is now in it's final shape:

• kept TY_PP_Asm check
• added darwin and armv7m in checks in tests
• cached IsMProfile
• used ARM::parseArchVersion

[rengolin]

LGTM, thanks!

cfe/trunk/lib/Driver/ToolChain.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetParser.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
+using namespace llvm;
 using namespace llvm::opt;
 
 static llvm::opt::Arg *GetRTTIArgument(const ArgList &Args) {
   return Args.getLastArg(options::OPT_mkernel, options::OPT_fapple_kext,
                          options::OPT_fno_rtti, options::OPT_frtti);
 }
 
 static ToolChain::RTTIMode CalculateRTTIMode(const ArgList &Args,
case llvm::Triple::thumbeb: {
     if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
       MArch = A->getValue();
     std::string CPU =
         Triple.isOSBinFormatMachO()
             ? tools::arm::getARMCPUForMArch(MArch, Triple).str()
             : tools::arm::getARMTargetCPU(MCPU, MArch, Triple);
     StringRef Suffix =
       tools::arm::getLLVMArchSuffixForARM(CPU, MArch, Triple);
-    bool ThumbDefault = Suffix.startswith("v6m") || Suffix.startswith("v7m") ||
-      Suffix.startswith("v7em") ||
-      (Suffix.startswith("v7") && getTriple().isOSBinFormatMachO());
+    bool IsMProfile = ARM::parseArchProfile(Suffix) == ARM::PK_M;
+    bool ThumbDefault = IsMProfile || (ARM::parseArchVersion(Suffix) == 7 && 
+                                       getTriple().isOSBinFormatMachO());
     // FIXME: this is invalid for WindowsCE
     if (getTriple().isOSWindows())
       ThumbDefault = true;
     std::string ArchName;
     if (IsBigEndian)
       ArchName = "armeb";
     else
       ArchName = "arm";
 
-    // Assembly files should start in ARM mode.
-    if (InputType != types::TY_PP_Asm &&
-        Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb, ThumbDefault))
+    // Assembly files should start in ARM mode, unless arch is M-profile.
+    if (IsMProfile || (InputType != types::TY_PP_Asm &&
+         Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb, ThumbDefault)))
     {
       if (IsBigEndian)
         ArchName = "thumbeb";
       else
         ArchName = "thumb";
     }
     Triple.setArchName(ArchName + Suffix.str());
 

cfe/trunk/test/Driver/arm-ias-Wa.s

 // RUN: %clang -target arm -Wa,-mhwdiv=bogus -c %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-BOGUS-HDIV %s
 // CHECK-BOGUS-HDIV: error: {{.*}} does not support '-Wa,-mhwdiv=bogus'
 
 // RUN: %clang -target arm -mhwdiv=arm -Wa,-mhwdiv=thumb -c %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-DUP-HDIV %s
 // CHECK-DUP-HDIV: "-target-feature" "-hwdiv-arm"
 // CHECK-DUP-HDIV: "-target-feature" "+hwdiv"
+
+// ========================================================== Triple
+// RUN: %clang -target armv7a-none-eabi -c %s -### 2>&1 \
+// RUN: %clang -target x86_64-apple-darwin -arch armv7 -c %s -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-A-PROFILE %s
+// CHECK-A-PROFILE: "-triple" "armv7-{{.*}}"
+
+// RUN: %clang -target armv7r-none-eabi -c %s -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-R-PROFILE %s
+// CHECK-R-PROFILE: "-triple" "armv7r-none--eabi"
+
+// RUN: %clang -target armv7m-none-eabi -c %s -### 2>&1 \
+// RUN: %clang -target thumbv7m-none-eabi -c %s -### 2>&1 \
+// RUN: %clang -target x86_64-apple-darwin -arch armv7m -c %s -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-M-PROFILE %s
+// CHECK-M-PROFILE: "-triple" "thumbv7m-{{.*}}"
+