Page MenuHomePhabricator

[libcxx] Add support for building and testing with an ABI library not along linker paths
ClosedPublic

Authored by EricWF on Aug 22 2014, 9:34 PM.

Details

Summary

This patch adds support for building/testing libc++ with an ABI library that the linker would not normally find.

  • CMAKE_LIBRARY_PATH is used to specify the list of search directories.
  • The ABI library is now found using find_library instead of assuming its along the linker's search path.
  • CMAKE_LIBRARY_PATH is passed to our LIT config as library_paths.
  • For each path in library_paths the following flags are added -L<path> -Wl,-rpath -Wl,<path>

Some changes in existing behavior were also added:

  • target_link_libraries is now passed the ABI library file instead of the library name. Ex target_link_libraries(cxx "/usr/lib/libc++abi.so") vs target_link_libraries(cxx "c++abi").
  • -Wl,-rpath -Wl,<path> is now used on OSX to link to libc++ instead of env['DYLD_LIBRARY_PATH'] if use_system_lib=False.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 12876.Aug 22 2014, 9:34 PM
EricWF retitled this revision from to [libcxx] Add support for building and testing with an ABI library not along linker paths.
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added reviewers: mclow.lists, danalbert.
EricWF added a subscriber: Unknown Object (MLST).
EricWF updated this revision to Diff 12877.Aug 22 2014, 10:46 PM

Added documentation on how to build libc++ against a non-installed ABI library.

Unfortunately not only does the user have to pass -DCMAKE_LIBRARY_PATH=/path/to/lib to CMake, but they also need to set RPATH=/path/to/lib when building libc++.
CMake does not have a mechanism for configuring relative paths via CMake variable.

We could manually add -Wl,-rpath -Wl,<lib-path> for all paths in CMAKE_LIBRARY_PATH but this also seems less than ideal.

One change I forgot to mention.

On linux when using libcxxrt it seems that we also need to link against -ldl. Support for this was added.

Sorry for the spam.

EricWF updated this object.Aug 22 2014, 11:06 PM
emaste added a subscriber: emaste.Aug 30 2014, 3:49 AM
danalbert edited edge metadata.Aug 30 2014, 9:56 AM
@EricWF wrote:

Unfortunately not only does the user have to pass -DCMAKE_LIBRARY_PATH=/path/to/lib to CMake, but they also need to set RPATH=/path/to/lib when building libc++.
CMake does not have a mechanism for configuring relative paths via CMake variable.

find_file() does this. Look at how I select libc++ header paths in the libc++abi CMakeLists.txt.

lib/CMakeLists.txt
46

libcxxrt is the one that should be linking libdl.

For a static libcxxrt we will need this though. Do we have a way to tell if we're linking a static abi lib?

48

What about FreeBSD? If the BSDs need it too, I think you actually just want to remove the second half of this conditional and let append_if work out this detail.

test/lit.cfg
342
[l for l in lpaths if l.strip()]
www/index.html
440

Then why are we doing this?

Assuming we do want to do this, should explain why this isn't recommended.

447

Aren't "configuring CMake" and "building libc++" the same thing from the (cmake) user's perspective?

More importantly, I'm pretty sure we only need one of these. See my comment about find_file().

452

Drop the -G part. It's the default.

456

Is this how we phrase this elsewhere in the page? I thought we did path/to/libcxx.

emaste added inline comments.Aug 30 2014, 6:52 PM
lib/CMakeLists.txt
48

libdl isn't needed on FreeBSD, but that case is already handled by append_if, is it not?

EricWF added a comment.Sep 1 2014, 6:37 PM
In D5038#9, @danalbert wrote:
@EricWF wrote:

Unfortunately not only does the user have to pass -DCMAKE_LIBRARY_PATH=/path/to/lib to CMake, but they also need to set RPATH=/path/to/lib when building libc++.
CMake does not have a mechanism for configuring relative paths via CMake variable.

find_file() does this. Look at how I select libc++ header paths in the libc++abi CMakeLists.txt.

Sorry, I could have been more clear. I'm using find_library but if the library is not along a standard path you need to specify CMAKE_LIBRARY_PATH. I incorrectly said relative paths but I meant runtime paths. Let me know if I got anything wrong or am missing anything.

I've respondeded to the inline comments and the changes will follow shortly after.

lib/CMakeLists.txt
46

Huh, Thanks for the correction. I didn't think of that. I don't know if we can tell the difference. I don't think i'll support that for now, but I'll strip this part out.

48

It's not handled by the append if, but the outer conditional which is only true on linux.

test/lit.cfg
342

*bows*. That is such a better way to write it.

www/index.html
440

Great note. I'll make the change. The reason we are doing this is

  1. Testing.
  2. The abililty to build and test on machines where libraries cannot be installed.
452

Will do.

456

I'll double check and make the change.

EricWF added inline comments.Sep 1 2014, 7:14 PM
www/index.html
447

Hi Dan,

Just wanted to be more clear. The difference between configuring and building is simply the difference between the cmake and the make command. For CMake we only need the CMAKE_LIBRARY_PATH to make find_library search along that path. Then we need RPATH when building so that the linker configures libc++.so to look for the ABI library along RPATH at runtime.

I can't actually find any instances of find_file under libcxxabi. Did you mean find_program?
Please let me know if I have misunderstood anything.

EricWF added a comment.Sep 1 2014, 7:23 PM

First, Sorry for the spam.

Second. @danalbert was right. RPATH is not needed at all. CMake correctly handles linking after only providing CMAKE_LIBRARY_PATH.

EricWF updated this revision to Diff 13148.Sep 1 2014, 7:26 PM
EricWF edited edge metadata.

Ok, really really really really sorry for all the spam. I've actually updated the code this time.

EricWF updated this object.Sep 1 2014, 7:27 PM
emaste added inline comments.Sep 2 2014, 9:06 AM
lib/CMakeLists.txt
48

But LIBCXX_HAS_DL_LIB should be false on FreeBSD (and other systems that don't have libdl)

EricWF updated this revision to Diff 14981.Oct 15 2014, 9:58 PM

Update and merge the patch with changes already upstream.

Just a few small changes.

test/lit.cfg
380

I don't think you meant to make this one a tuple.

www/index.html
472

Indentation mismatch.

481

linker's

danalbert added inline comments.Oct 17 2014, 6:10 PM
test/lit.cfg
380

Oh, generator, not a tuple. In that case, I don't think you can directly concatenate a generator to a list. Use a list comprehension.

danalbert added inline comments.Oct 17 2014, 6:13 PM
test/lit.cfg
380

Apparently += is allowed, + isn't. Carry on...

EricWF updated this revision to Diff 15110.Oct 17 2014, 6:23 PM

Addressed danalbert's comments.

EricWF accepted this revision.Oct 17 2014, 6:24 PM
EricWF added a reviewer: EricWF.

Accepting before commit.

This revision is now accepted and ready to land.Oct 17 2014, 6:24 PM
EricWF closed this revision.Oct 17 2014, 6:25 PM