Page MenuHomePhabricator

[ToolChains] Link to compiler-rt with -L + -l when possible
Needs ReviewPublic

Authored by mstorsjo on Aug 29 2018, 9:57 AM.

Details

Summary

This avoids a libtool issue (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866), where libtool fails to pick up the default linked libraries when they are referred to via a direct path to the static library, instead of as a -L + -l pair, as for libgcc.

Or is there some reason not to add the clang_rt dir to the linker path with -L?

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 29 2018, 9:57 AM
beanz added a subscriber: phosek.

Looping in @phosek.

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

Since the libraries will no longer be specified with their full path, how will you know that the right library will be picked, the one compiler intended?

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

I guess I could do that, although we don't know the path in the test, so we can only check for -L.*.

Since the libraries will no longer be specified with their full path, how will you know that the right library will be picked, the one compiler intended?

We don't; the same goes for libgcc. I guess the risk for unintentional clashes is rather low; if the linker found another lib with the same name, I would expect it to be intentional, and using it wouldn't be wrong.

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

I guess I could do that, although we don't know the path in the test, so we can only check for -L.*.

You can add a testcase that passes a known path via -resource-dir=path argument similar to --sysroot argument.

What about other compiler-rt runtimes? Shouldn't we use the same approach for those?

In case of multiarch runtime layout, we already add the runtime directory to the new LibraryPaths list, which is added to the list of library paths. We could consider doing the same for the default layout, in which case all getCompilerRT would need to return is the name of the library rather than a path, but we would need to use -l for all compiler-rt runtimes, not just builtins. That's going to require more changes, but I think it's going to result in a cleaner design.

@beanz would that work for Darwin as well?

lib/Driver/ToolChains/CommonArgs.cpp
1164

Rather than parsing the return string below, wouldn't it be cleaner to modify ToolChain::getCompilerRT to return a tuple (ToolChain::getCompilerRTArgString could still return a string) and then use if here?

1168

When the multiarch runtime layout is being used, this will result in this path being added twice, once in the ToolChain constructor and once here.

What about other compiler-rt runtimes? Shouldn't we use the same approach for those?

Yes, I guess so.

In case of multiarch runtime layout, we already add the runtime directory to the new LibraryPaths list, which is added to the list of library paths. We could consider doing the same for the default layout, in which case all getCompilerRT would need to return is the name of the library rather than a path, but we would need to use -l for all compiler-rt runtimes, not just builtins. That's going to require more changes, but I think it's going to result in a cleaner design.

Yes, so it sounds.

I'll see if I can get to looking at that sometime soon. I had this patch lying around as an attempt to work around the libtool issue in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 which doesn't seem to be going anywhere (and even if it did, it probably takes quite a bit of time before a new libtool release is made and it gets propagated to most places where I'd like to use this), and was curious if there was any specific reason for having this the way it was, or just the usual; historical reasons that it has started out like this and haven't had a need to change until now. If you otherwise would happen to be touching the same areas, feel free to pick it up ;-) otherwise I'll try to look at addressing your points in a few days.

I'll see if I can get to looking at that sometime soon. I had this patch lying around as an attempt to work around the libtool issue in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 which doesn't seem to be going anywhere (and even if it did, it probably takes quite a bit of time before a new libtool release is made and it gets propagated to most places where I'd like to use this), and was curious if there was any specific reason for having this the way it was, or just the usual; historical reasons that it has started out like this and haven't had a need to change until now. If you otherwise would happen to be touching the same areas, feel free to pick it up ;-) otherwise I'll try to look at addressing your points in a few days.

I'd be happy to pick this up. I already planned to do more refactoring/cleanup of this part of the driver. @beanz is improving the Darwin toolchain driver to make more like other Clang drivers, so I want to first sync up with him to make sure this is also going to work for them.

I'll see if I can get to looking at that sometime soon. I had this patch lying around as an attempt to work around the libtool issue in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 which doesn't seem to be going anywhere (and even if it did, it probably takes quite a bit of time before a new libtool release is made and it gets propagated to most places where I'd like to use this), and was curious if there was any specific reason for having this the way it was, or just the usual; historical reasons that it has started out like this and haven't had a need to change until now. If you otherwise would happen to be touching the same areas, feel free to pick it up ;-) otherwise I'll try to look at addressing your points in a few days.

I'd be happy to pick this up. I already planned to do more refactoring/cleanup of this part of the driver. @beanz is improving the Darwin toolchain driver to make more like other Clang drivers, so I want to first sync up with him to make sure this is also going to work for them.

Oh, great - thanks a lot!

Unfortunately I can't make this kind of policy decision about whether or not this would be acceptable for Darwin. That would probably need to be @dexonsmith.

I do prefer the current approach especially on Darwin. Some concerns of switching to use "-L + -l" are:

  1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from resource-dir and passing to linker with the full path can prevent mistakes of mixing them up.
  2. This change does change linker semantics on Darwin. ld64 treats the inputs from command line with highest priority. Currently ld64 will use compiler-rt symbols before any other libraries passed in with -l flag. If clang decide to pass compiler-rt with -l flag, any other libraries (static or dynamic) that passed with -l flag will takes the priority over compiler-rt. This can lead to unexpected behavior.

I do prefer the current approach especially on Darwin. Some concerns of switching to use "-L + -l" are:

  1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from resource-dir and passing to linker with the full path can prevent mistakes of mixing them up.
  2. This change does change linker semantics on Darwin. ld64 treats the inputs from command line with highest priority. Currently ld64 will use compiler-rt symbols before any other libraries passed in with -l flag. If clang decide to pass compiler-rt with -l flag, any other libraries (static or dynamic) that passed with -l flag will takes the priority over compiler-rt. This can lead to unexpected behavior.

I tend to agree with Steven. I'd rather avoid a semantic change here.