[PATCH] D10841: [Shave]: allow Clang to run the target linker.

[Douglas Katzman via cfe-commits]

dougk abandoned this revision.

================
Comment at: lib/Driver/Tools.cpp:9110
@@ +9109,3 @@
+
+  const char *ToolsRoot = ::getenv("MV_TOOLS_DIR");
+  // The version is numbered 'n.n.n.n' for arbitrary values that are opaque
----------------
jyknight wrote:
> Perhaps this should be a default plus a command-line argument, instead of an environment variable?
How about if '--sysdir' provides the value, falling back to the environment var, and then a fixed default?
There is ample precedent for use of getenv() in some of the other toolchains.

================
Comment at: lib/Driver/Tools.cpp:9144
@@ +9143,3 @@
+    ToolsRoot = "/usr/local/myriad/tools";
+    ToolsVersion = "00.50.62.5";
+    ToolsCommonDir = "/usr/local/mdk/tools/" + ToolsVersion + "/common";
----------------
jyknight wrote:
> Doesn't seem right to hardcode this particular version. Using a default for ToolsRoot if MV_TOOLS_DIR isn't set sounds okay, but presumably should be done up above before the loop.
> 
I could see a couple ways around hardcoding a version - if neither --sysroot nor MV_TOOLS_DIR is used, then the path is arbitrary anyway, so doesn't need a versioned subdir. And arguably is sysroot is given, then it could be an already-versioned pathname. Otoh, detecting the version works fine in that case.

For reference, the example sub-Makefile has hardcoded:
MV_TOOLS_VERSION ?= 00.50.62.5
MV_TOOLS_DIR ?= $(HOME)/WORK/Tools/Releases/General

================
Comment at: lib/Driver/Tools.cpp:9160
@@ +9159,3 @@
+  CmdArgs.push_back("-EL"); // Endianness = little
+  CmdArgs.push_back("-O9"); // Optimization
+  CmdArgs.push_back("--gc-sections");
----------------
jyknight wrote:
> "-O9"? That seems weird, and is not actually a thing.
This was cargo-cultism.   I've no objection to taking it out, but who's to say that their linker doesn't use it for something, as why else would they bother to put it in the examples?


http://reviews.llvm.org/D10841