[clang][driver][NFC] Call IsARMBigEndain function only for isARM and isThumb.

[simpal01]

Summary

IsARMBIgEndian function returns true only if:

1. The triples are either arm or thumb and the commandline has the option -mbig-endian
2. The triples are either armeb or thumbeb.

Missing the checking of arm or thumb triples in the
first case pass through the --be8 endian flag to
linker For AArch64 as well which is not expected.
This is the regression happened from the previous
patch https://reviews.llvm.org/D154786.

It is better to refactor to only call IsARMBigEndian
for isARM and isthumb satisfying conditions which
keeps ARM and AArch64 separate.

[peter.smith]

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

39 ↗

(On Diff #542403)

Is this the right place to fix?

I would expect it to be a precondition that the Triple was Arm or Thumb before calling isARMBigEndian?

For example

if (Triple.isARM() || Triple.isThumb() || Triple.isAArch64()) {
    bool IsBigEndian = arm::isARMBigEndian(Triple, Args);
    if (IsBigEndian)
      arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
    IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;
    CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
  }

Shouldn't this be refactored to only call isARMBigEndian for isARM and isThumb? Something like:

if ((Triple.isARM() || Triple.isThumb()) {
  bool BigEndian = arm::isARMBigEndian(Triple, Args);
  if (BigEndian)
    arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
  CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
} else if (Triple.isAArch64)) {
  CmdArgs.push_back(Arch == llvm::Triple::aarch64_be ? "-EB" : "-EL");
}

This is a bit longer but it is easier to read and keeps ARM and AArch64 separate.

[simpal01]

Addressing review comments.

[MaskRay]

Sounds good to make arm::isARMBigEndian focus on AArch32 :)

[MaskRay]

Add NFC if this is NFC?

[simpal01]

In D155808#4528543, @MaskRay wrote:

Add NFC if this is NFC?

Done

clang/lib/Driver/ToolChains/BareMetal.cpp

void baremetal::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   auto &TC = static_cast<const toolchains::BareMetal &>(getToolChain());
   const llvm::Triple::ArchType Arch = TC.getArch();
   const llvm::Triple &Triple = getToolChain().getEffectiveTriple();
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
   CmdArgs.push_back("-Bstatic");
 
-  if (Triple.isARM() || Triple.isThumb() || Triple.isAArch64()) {
+  if (Triple.isARM() || Triple.isThumb()) {
     bool IsBigEndian = arm::isARMBigEndian(Triple, Args);
     if (IsBigEndian)
       arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
-    IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;
     CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
+  } else if (Triple.isAArch64()) {
+    CmdArgs.push_back(Arch == llvm::Triple::aarch64_be ? "-EB" : "-EL");
   }
 
   Args.AddAllArgs(CmdArgs,
                   {options::OPT_L, options::OPT_T_Group, options::OPT_s,
                    options::OPT_t, options::OPT_Z_Flag, options::OPT_r});
 
   TC.AddFilePathLibArgs(Args, CmdArgs);
 

clang/lib/Driver/ToolChains/Gnu.cpp

void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   }
 
   if (Args.hasArg(options::OPT_rdynamic))
     CmdArgs.push_back("-export-dynamic");
 
   if (Args.hasArg(options::OPT_s))
     CmdArgs.push_back("-s");
 
-  if (Triple.isARM() || Triple.isThumb() || Triple.isAArch64()) {
+  if (Triple.isARM() || Triple.isThumb()) {
     bool IsBigEndian = arm::isARMBigEndian(Triple, Args);
     if (IsBigEndian)
       arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
-    IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;
     CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
+  } else if (Triple.isAArch64()) {
+    CmdArgs.push_back(Arch == llvm::Triple::aarch64_be ? "-EB" : "-EL");
   }
 
   // Most Android ARM64 targets should enable the linker fix for erratum
   // 843419. Only non-Cortex-A53 devices are allowed to skip this flag.
   if (Arch == llvm::Triple::aarch64 && (isAndroid || isOHOSFamily)) {
     std::string CPU = getCPUName(D, Args, Triple);
     if (CPU.empty() || CPU == "generic" || CPU == "cortex-a53")
       CmdArgs.push_back("--fix-cortex-a53-843419");