[PATCH] D33259: Don't defer to the GCC driver for linking arm-baremetal

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.


================
Comment at: cmake/caches/BaremetalARM.cmake:1
+set(LLVM_TARGETS_TO_BUILD ARM CACHE STRING "")
+
----------------
Please rename this file to `BareMetalARMv6.cmake`.  (I'm interested in the suffix primarily).


================
Comment at: lib/Driver/ToolChains/BareMetal.cpp:68
+  SmallString<128> Dir(getDriver().ResourceDir);
+  llvm::sys::path::append(Dir, "lib", "baremetal");
+  return Dir.str();
----------------
Why not just the standard `arm` directory?


================
Comment at: lib/Driver/ToolChains/BareMetal.cpp:74
+  SmallString<128> Dir(getDriver().ResourceDir);
+  llvm::sys::path::append(Dir, "include", "c++", "v1");
+  return Dir.str();
----------------
This seems wrong.  The libc++ headers should *not* be in the resource dir.  This should be based off of the sysroot IMO.


================
Comment at: lib/Driver/ToolChains/BareMetal.cpp:107-108
+                                    ArgStringList &CmdArgs) const {
+  CmdArgs.push_back("-lc++");
+  CmdArgs.push_back("-lc++abi");
+  CmdArgs.push_back("-lunwind");
----------------
I think that this is a bit extreme.  We already have `-stdlib=libc++` and `-stdlib=libstdc++`.  Why not just honor that?


================
Comment at: lib/Driver/ToolChains/BareMetal.h:42
+
+  const char *getDefaultLinker() const override { return "ld.lld"; }
+
----------------
I think that this really should be `ld` still, as that is the canonical name for the linker.


================
Comment at: lib/Driver/ToolChains/BareMetal.h:57
+                         llvm::opt::ArgStringList &CmdArgs) const;
+protected:
+};
----------------
Unnecessary?


================
Comment at: lib/Driver/ToolChains/BareMetal.h:67
+public:
+  Linker(const ToolChain &TC) : Tool("baremetal::Linker", "ld.lld", TC) {}
+  bool isLinkJob() const override { return true; }
----------------
Update this accordingly.


https://reviews.llvm.org/D33259