This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Allow setting the default linker during build
ClosedPublic

Authored by phosek on Oct 4 2016, 6:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek updated this revision to Diff 73589.Oct 4 2016, 6:53 PM
phosek retitled this revision from to [Driver] Allow setting the default linker during build.
phosek updated this object.
phosek set the repository for this revision to rL LLVM.
phosek added a subscriber: cfe-commits.
mgorny added inline comments.Oct 10 2016, 2:43 AM
CMakeLists.txt
198

Is there a reason not to allow using the absolute path here, like for the command-line option?

bruno added a subscriber: bruno.Oct 10 2016, 1:54 PM
bruno added inline comments.
CMakeLists.txt
198

I agree here, if we're adding a cmake options for this, it should accept full paths to the linker to be used (without any need for its type like gold, bfd, etc) as well.

Additionally, if "" maps to "ld", plain CLANG_DEFAULT_LINKER="ld" should also work here.

Hahnfeld edited edge metadata.Oct 17 2016, 12:36 AM

Have you run all tests with CLANG_DEFAULT_LINKER not being the platform default? I imagine there might be some tests that expect ld to be used...

CMakeLists.txt
198

I agree with both points here.

lib/Driver/ToolChain.cpp
352

I wonder whether this is really correct: If DefaultLinker is not ld (it is lld for some ToolChains), -fuse-ld= with an empty argument should probably not use ld but rather whatever DefaultLinker says...

emaste added a subscriber: emaste.Oct 30 2016, 9:05 AM
dtzWill added a subscriber: dtzWill.Nov 2 2016, 4:17 AM
phosek updated this revision to Diff 76720.Nov 2 2016, 9:19 AM
phosek edited edge metadata.
phosek marked 3 inline comments as done.

I have two high level remarks here:

  1. CLANG_DEFAULT_LINKER should override whatever the platform default is. So ToolChain::getDefaultLinker() should return ld as the variable DefaultLinker currently says and the logic should be in ToolChain::GetLinkerPath().
  2. The CMake variable is documented to be empty for the platform default. I think this will currently not work when used by GetProgramPath(getDefaultLinker()), will it?
lib/Driver/ToolChain.cpp
367

You can leave this return because Clang will exit anyway after emitting the error (as I had to learn...)

710–713

I think this could go into the header

sfertile added inline comments.Nov 30 2016, 11:33 AM
lib/Driver/ToolChain.cpp
710–713

The CLANG_DEFAULT_LINKER macro is getting defined in "clang/Config/config.h" which isn't meant to be included in other headers.

Hahnfeld added inline comments.Nov 30 2016, 11:35 PM
lib/Driver/ToolChain.cpp
710–713

Right, that does not work for the current version of the patch.

It was meant in conjunction with this comment:

CLANG_DEFAULT_LINKER should override whatever the platform default is. So ToolChain::getDefaultLinker() should return ld as the variable DefaultLinker currently says and the logic should be in ToolChain::GetLinkerPath().

phosek updated this revision to Diff 80934.Dec 9 2016, 12:28 PM
phosek marked 4 inline comments as done.
phosek added inline comments.
lib/Driver/ToolChain.cpp
352

I'm wandering whether we shouldn't use "platform" instead of "ld" here to match what we do for -rtlib= and -stdlib=?

Hahnfeld accepted this revision.Dec 11 2016, 11:46 PM
Hahnfeld edited edge metadata.

LGTM with one nit

lib/Driver/ToolChain.cpp
352

I'd say this is fine for compatibility

356

I think this will give an error for -fuse-ld= with no argument. Please revert the condition above to UseLinker.empty() || UseLinker == "ld" to match the current behaviour

This revision is now accepted and ready to land.Dec 11 2016, 11:46 PM
phosek updated this revision to Diff 81276.Dec 13 2016, 11:58 AM
phosek edited edge metadata.
phosek marked 3 inline comments as done.
This revision was automatically updated to reflect the committed changes.