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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Thanks for the addition of the flags to the linker. Interesting note about `-m*-endian` only being applicable for armv7.  Just some minor nits left.

With the current version of the patch, I can now assemble, link, and boot in a virtualized environment a big-endian armv8 Linux kernel with Clang. :)



================
Comment at: lib/Driver/ToolChains/Gnu.cpp:231
 
+// On Arm and endianness of the output file is determined by the target and
+// can be overridden by the pseudo-target flags '-mlittle-endian'/'-EL' and
----------------
> // On Arm and endianness
drop first `and`


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:261-264
+    if (isArmBigEndian(T, Args))
+      return "armelfb_linux_eabi";
+    else
+      return "armelf_linux_eabi";
----------------
would a `?:` ternary fit on one line here?

> return isArmBigEndian(T, Args) ? "armelfb_linux_eabi" : "armelf_linux_eabi";


================
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";
----------------
```
bool IsBigEndian = isArmBigEndian(Triple, Args);
if (IsBigEndian)
  arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
IsBigEndian |= Arch == llvm::Triple::aarch64_be;
CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
```


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:362
+    }
+    else if (Arch == llvm::Triple::aarch64_be)
+      EndianFlag = "-EB";
----------------
is having the `else if` on its own line what the formatter chose?


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:667-670
+    if (isArmBigEndian(Triple2, Args))
+      CmdArgs.push_back("-EB");
+    else
+      CmdArgs.push_back("-EL");
----------------
Can we fit a ternary in one line here as well?

```
CmdArgs.push_back(isArmBigEndian(Triple2, Args) ? "-EB" : "-EL");
```


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:703
   case llvm::Triple::aarch64_be: {
+    if (getToolChain().getTriple().isLittleEndian())
+      CmdArgs.push_back("-EL");
----------------
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?


https://reviews.llvm.org/D52784