[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

[Peter Smith via Phabricator via cfe-commits]

peter.smith marked 7 inline comments as done.
peter.smith added a comment.

Thanks very much for the comments. I'll post an update shortly.



================
Comment at: lib/Driver/ToolChains/Gnu.cpp:357-364
+    const char* EndianFlag = "-EL";
+    if (isArmBigEndian(Triple, Args)) {
+      EndianFlag = "-EB";
+      arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
+    }
+    else if (Arch == llvm::Triple::aarch64_be)
+      EndianFlag = "-EB";
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > ```
> > bool IsBigEndian = isArmBigEndian(Triple, Args);
> > if (IsBigEndian)
> >   arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
> > IsBigEndian |= Arch == llvm::Triple::aarch64_be;
> > CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
> > ```
> `IsBigEndian |= Arch == llvm::Triple::aarch64_be;`
> 
> should be:
> 
> `IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;`
> 
> in order to not evaluate `Arch == llvm::Triple::aarch64_b` if `IsBigEndian` is already true.
Thanks for the suggestion. One thing it highlighted was that isArmBigEndian could return true for an aarch64_be arch with -mbig-endian so I've rewritten isArmBigEndian to always return false if the architecture isn't Arm and have added some test cases to check that "--be8" doesn't sneak in.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:362
+    }
+    else if (Arch == llvm::Triple::aarch64_be)
+      EndianFlag = "-EB";
----------------
nickdesaulniers wrote:
> is having the `else if` on its own line what the formatter chose?
I'd forgot to run clang-format over that part of the code. I've adopted the snippet below which replaces it.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:703
   case llvm::Triple::aarch64_be: {
+    if (getToolChain().getTriple().isLittleEndian())
+      CmdArgs.push_back("-EL");
----------------
nickdesaulniers wrote:
> earlier (L362), you check the endianess of the triple with:
> 
> ```
> Arch == llvm::Triple::aarch64_be
> ```
> where `Arch` is `ToolChain.getArch()`.
> 
> I don't have a preference, but these two seem inconsistent.  Can we either check the explicit `llvm::Triple::` or call `getToolChain().getTriple().isLittleEndian()` in both, rather than mix?
I originally took that part from the Mips code, I've replaced it with a check against aarch64_be which is more consistent with the other Arm and AArch64 code.


https://reviews.llvm.org/D52784