[PATCH] D154786: [Clang][Driver] Pass through the --be8 endian flag to linker in BareMetal driver For Arm.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:48
+  case llvm::Triple::thumb:
+    if (Arg *A = Args.getLastArg(options::OPT_mlittle_endian,
+                                 options::OPT_mbig_endian))
----------------
We can check the options first and do an early return, then check `getArch`. We can then avoid `[[fallthrough]]`.

`arm::setArchNameInTriple` has very similar code. We should factor it out.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.h:17
 #include "llvm/TargetParser/Triple.h"
+//#include "llvm/Option/ArgList.h"
 #include <string>
----------------
This should not be commented out. IWYU we need this header.


================
Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:433
   auto &TC = static_cast<const toolchains::BareMetal &>(getToolChain());
 
+  const llvm::Triple::ArchType Arch = TC.getArch();
----------------
no blank line


================
Comment at: clang/test/Driver/baremetal.cpp:121
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     --target=armebv7-none-eabi \
+// RUN:     --sysroot=%S/Inputs/baremetal_arm \
----------------
I think we can use a more dense style by packing more options on one line.

I often find the large number of lines impacts readability.


================
Comment at: clang/test/Driver/baremetal.cpp:126
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     --target=armv7-none-eabi \
----------------
Delete `-no-canonical-prefixes`.

https://maskray.me/blog/2021-03-28-compiler-driver-and-cross-compilation#misc

> -no-canonical-prefixes uses the dereferenced absolute path for the -cc1 command. For most tests "-cc1" is sufficient to identify the command line, no need to specifically test the "clang" command, and -no-canonical-prefixes can be removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154786/new/

https://reviews.llvm.org/D154786