Page MenuHomePhabricator

[ToolChain] Use default linker if the toolchain uses a custom one
Needs ReviewPublic

Authored by phosek on Oct 13 2018, 6:25 PM.

Details

Reviewers
rsmith
Summary

When the toolchain overrides default linker, use it rather than the
default Clang one which is unlikely to be useable for that toolchain
anyway. This avoids the need to explicitly specify -fuse-ld in tests
for all targets that use custom linkers.

Diff Detail

Repository
rC Clang

Event Timeline

phosek created this revision.Oct 13 2018, 6:25 PM

This should avoid workarounds such as D49899 or D53249.

If I read that patch correctly, this will render -fuse-ld with non-absolute paths useless if a toolchain has DefaultLinker != "ld". I don't think that's what we want to do if the user explicitly sets a different linker.

This should avoid workarounds such as D49899 or D53249.

I agree that these changes are ugly, but we do the same with -stdlib=platform and -rtlib=platform for tests that check Clang's choices for a particular platform. For this case it's what you did in D53249: Adding -fuse-ld=ld to tests that check the default linker for a platform. (because there is a case for UseLinker == "ld")

If I read that patch correctly, this will render -fuse-ld with non-absolute paths useless if a toolchain has DefaultLinker != "ld". I don't think that's what we want to do if the user explicitly sets a different linker.

You're right, I missed that case, I'll try to address it.

I agree that these changes are ugly, but we do the same with -stdlib=platform and -rtlib=platform for tests that check Clang's choices for a particular platform. For this case it's what you did in D53249: Adding -fuse-ld=ld to tests that check the default linker for a platform. (because there is a case for UseLinker == "ld")

I agree about the test case but I think the current logic is broken even for regular use. If the platform specifies a custom linker (e.g. hegaxon-link), CLANG_DEFAULT_LINKER should not override it because it's unlikely that it'll be usable on that platform. Today we treat CLANG_DEFAULT_LINKER as a default value for -fuse-ld= but I think we should instead treat it as a default value for ld (i.e. default system linker) which will be used if -fuse-ld= is unspecified or the platform doesn't specify its own linker. Would you agree with that?

The reason why this is not a problem of -stdlib= and -rtlib= is because many toolchains override ToolChain::GetRuntimeLibType and ToolChain::GetCXXStdlibType (e.g. Fuchsia or WebAssembly) completely bypassing the default logic. The reason why this is not being used for linker selection is because -fuse-ld=/path/to/linker is generally useful even for these targets that use custom linkers (e.g. D53038 explicitly mentions that) so I think we either need to make ToolChain::GetLinkerPath smarter about handling platform-specific linkers or we need to extract the logic for handling linker specified via absolute path into a helper method so toolchains can override ToolChain::GetLinkerPath without re-implementing chunk of its logic.

Going through my old revisions, is this still something that we want?

Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2019, 9:47 AM

Sorry about the late response, I was mostly out last week. This recently came up again so I'd like to look into it but I'll have to rethink the change and come up with a better approach.