Page MenuHomePhabricator

[runtimes] Support ELF dependent libraries feature
ClosedPublic

Authored by phosek on May 17 2019, 4:45 PM.

Details

Summary

As of r360984, LLD supports dependent libraries feature for ELF.
libunwind and libc++ have library dependencies: libdl and libpthread
respectively, which means that when libunwind and libc++ are being
statically linked (using -static-libstdc++ flag), user has to manually
specify -ldl -lpthread which is onerous. This change includes the
lib pragma to specify the library dependencies directly in the source
that uses those libraries. This doesn't make any difference when using
linkers that don't support dependent libraries. However, when using
LLD that has dependent libraries feature, users no longer have to
manually specifying library dependencies when using static linking,
linker will pick them up automatically. The use of this feature is
conditionalized on CMake build option and can be enabled by vendors.

Diff Detail

Repository
rCXX libc++

Event Timeline

phosek created this revision.May 17 2019, 4:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 4:45 PM
phosek updated this revision to Diff 200343.May 20 2019, 12:23 PM

Would it be possible to take a look? I believe this is a better alternative to both D54724 and D54726 which doesn't require any changes to the driver or special linker scripts.

I guess I don't have any strong objection, but I think I had a preference for handling it in the build system per https://reviews.llvm.org/D54726. This would conceivably work on other platforms that support linker scripts and it's less intrusive to the source code. But if you feel strongly that this is the right way to go, I'm OK with it.

Great to see someone wanting to use this. During the RFC we decided not to set SHF_EXCLUDE on the .deplibs ELF section that is used to store the strings from these pragmas. This means that other linkers that don't support dependent libraries will not remove the sections and they will end up in the output. This shouldn't cause any functional issues; however, it might be noticed by developers and commented on. Perhaps we should change this decision? Also, have you considered the other option of having the build system invoke the clang with the --dependent-library=<specifier> switch. This is an alternative approach where modifying the source code is not appropriate, an example use is: https://reviews.llvm.org/D61742.

I guess I don't have any strong objection, but I think I had a preference for handling it in the build system per https://reviews.llvm.org/D54726. This would conceivably work on other platforms that support linker scripts and it's less intrusive to the source code. But if you feel strongly that this is the right way to go, I'm OK with it.

I have a slight preference for this approach for two reasons: (1) with linker scripts we have to rename the static library which may lead to some confusion, e.g. simply running readelf -sW libc++.a will fail because libc++.a is now a linker script; (2) it localizes the dependency information to where it's needed, e.g. the pthread library dependency is only included if _LIBCPP_HAS_THREAD_API_PTHREAD is defined.

Great to see someone wanting to use this. During the RFC we decided not to set SHF_EXCLUDE on the .deplibs ELF section that is used to store the strings from these pragmas. This means that other linkers that don't support dependent libraries will not remove the sections and they will end up in the output. This shouldn't cause any functional issues; however, it might be noticed by developers and commented on. Perhaps we should change this decision?

I think we should consider that.

Also, have you considered the other option of having the build system invoke the clang with the --dependent-library=<specifier> switch. This is an alternative approach where modifying the source code is not appropriate, an example use is: https://reviews.llvm.org/D61742.

That might be an option as well and is something that can be done from CMake invocation via CFLAGS. The downside is AFAICT that the information will no longer be localized to where it's needed and instead applied to all translation units inside the archive e.g. even if you use part of libc++ that doesn't need pthread, you'll still get the dependency. An alternative would be to set that flag on a per translation unit basis but I'm not sure whether that's better than having the pragma directly in the source.

ldionne accepted this revision.May 23 2019, 8:05 AM

I don't have any objections to this (but it doesn't impact my platform).

This revision is now accepted and ready to land.May 23 2019, 8:05 AM
phosek updated this revision to Diff 201374.May 24 2019, 5:39 PM
phosek edited the summary of this revision. (Show Details)

I've conditionalized the use of this feature on CMake build option which defaults to OFF and can be turned on by vendors as needed. Hopefully that's cleaner than the previous version where it was enabled unconditionally.

phosek updated this revision to Diff 201812.May 28 2019, 7:45 PM
phosek retitled this revision from [libcxx][libunwind] Support ELF dependent libraries feature on Linux to [runtimes] Support ELF dependent libraries feature on Linux.
phosek retitled this revision from [runtimes] Support ELF dependent libraries feature on Linux to [runtimes] Support ELF dependent libraries feature.

It's not entirely clear to me that adding an option to control this is better than just doing it unconditionally. Have you run into a scenario where you didn't want to enable it (and if so, why?).

If that's "just in case", I'd suggest doing it unconditionally and adding an option later if a vendor wants to disable it.

libcxx/CMakeLists.txt
547 ↗(On Diff #201812)

Please use the per-target form. We've done a lot of work to remove global definitions and flags, let's keep working in that direction.

phosek updated this revision to Diff 202010.May 29 2019, 11:44 AM
phosek marked an inline comment as done.

It's not entirely clear to me that adding an option to control this is better than just doing it unconditionally. Have you run into a scenario where you didn't want to enable it (and if so, why?).

If that's "just in case", I'd suggest doing it unconditionally and adding an option later if a vendor wants to disable it.

That makes sense, I've removed the option but I've moved the pragma from headers into source files to avoid it being injected into users code through includes.

ldionne accepted this revision.May 29 2019, 12:49 PM
This revision was automatically updated to reflect the committed changes.