This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Framework to allow testing of static libc++abi
ClosedPublic

Authored by bcraig on Jan 25 2016, 11:10 AM.

Details

Summary
NOTE: related libc++abi changes are up for review here: http://reviews.llvm.org/D16545 .

Tested with static libcxx and libcxxabi against custom embedded target and Linux x64.
Tested with shared libcxx and libcxxabi against Linux x64.
Really hoping that I didn't accidentally break Mac.

These changes make linking against static libraries more explicit. Instead
of using -lc++ and -lc++abi in the tests, an absolute path to the library is
provided instead.

The choices of shared vs. static, and the choices of library paths for both
libcxx and libcxxabi needed to be exchanged for this to work. In other words,
libcxx tests need to know the library path of libcxxabi, and whether libcxxabi
is a static or shared library.

Some Mac specific logic for testing against libc++abi had to be moved from
libcxxabi's config.py, as it was overriding choices made in libcxx's config.py.
That logic is now in libcxx's target_info.py.

Testing a static libcxx on Linux will now automatically link in librt most of
the time. Previously, lots of pthread tests would fail because of an
unresolved clock_gettime.

Diff Detail

Event Timeline

bcraig updated this revision to Diff 45885.Jan 25 2016, 11:10 AM
bcraig retitled this revision from to [libcxx] Framework to allow testing of static libc++abi.
bcraig updated this object.
bcraig added reviewers: mclow.lists, EricWF, jroelofs.
bcraig updated this object.
bcraig added a subscriber: cfe-commits.
bcraig updated this object.Jan 25 2016, 11:13 AM
EricWF edited edge metadata.Feb 11 2016, 9:36 AM

Did you consider simply wrapping the library in "-Wl,-Bstatic" and "-Wl,-Bdynamic" linker flags? It seems like that would work almost as well as explicitly naming the target.

test/libcxx/test/config.py
463–472
libcxx_shared = self.get_lit_bool('enable_shared', default=True)
469

Why not just wrap it in '-B,-static'?

482–491

Same as above.

test/libcxx/test/target_info.py
173

Hmm... this would have a different default compared to elsewhere after my suggested edits.

bcraig added inline comments.Feb 11 2016, 1:53 PM
test/libcxx/test/config.py
469

I know exactly which binary I want to link against. Running through the linker search path can't make this work better, but it can make it work worse.

test/libcxx/test/target_info.py
173

When testing libcxx, it's going to be difficult to get an unset enable_shared, so the default doesn't matter that much in that situation. It might matter more with libcxxabi. I will investigate.

Regardless, I'm pretty sure "False" is the default here due to copy / paste rather than long, careful consideration.

bcraig updated this revision to Diff 48113.Feb 16 2016, 2:42 PM
bcraig edited edge metadata.

Addressed EricWF's feedback (hopefully).

bcraig marked 6 inline comments as done.Feb 16 2016, 2:43 PM

Just spitballing but have you considered simply passing the full path, including the library name, to LIT? Instead of needing enable_shared variables we would simply use the named library, be it DSO or archive.

test/libcxx/test/config.py
488

os.path.join

test/libcxx/test/target_info.py
113

This part makes me uncomfortable. Half the reason this was disabled in the first place was to help find bugs like this.

Make the comment a FIXME and file a bug and I'm OK with this going in for now.

If we choose not to use full library paths could you please remove the 'libcxx_library' option? It just does what your trying here but worse, no need to keep it around.

Just spitballing but have you considered simply passing the full path, including the library name, to LIT? Instead of needing enable_shared variables we would simply use the named library, be it DSO or archive.

I agree that this would be nice. I don't really like the nest of if / else statements that I currently have in place. However, I have now attempted to do this, and failed. My cmake-fu just isn't strong enough.

My approach was to populate variables like so...

get_target_property(LIBCXXABI_FULL_LIB_PATH cxxabi_static LOCATION)
get_target_property(LIBCXXABI_FULL_LIB_PATH cxxabi_shared LOCATION)
get_target_property(LIBCXX_FULL_LIB_PATH cxx LOCATION)

... and then to plumb that through lit.site.cfg.in. This would work great if the core of cxxabi and cxx were processed first, followed by their respective tests. Unfortunately, cxxabi and it's tests get processed before cxx gets processed. This means that I can't rely on cxx's CMakeLists.txt to populate a path on behalf of cxxabi. I could attempt to rebuild the path from the pieces of information that I do have access to, but that seems really hacky.

Note that getting a full path in doesn't remove all the if / elif complexity in getting cxx[abi] to the link line. If no desired path exists in the config, then something like -lc++ or -lc++abi still needs to be passed in. This is to handle the case where someone is building one of libcxx[abi] without the counterpart being available. We don't know the absolute path to the respective library, but it is (presumably) in the library path.

Just spitballing but have you considered simply passing the full path, including the library name, to LIT? Instead of needing enable_shared variables we would simply use the named library, be it DSO or archive.

I agree that this would be nice. I don't really like the nest of if / else statements that I currently have in place. However, I have now attempted to do this, and failed. My cmake-fu just isn't strong enough.

OK. Thanks for looking into this. I figured CMake would stand in our way. Last I looked into it CMake doesn't actually know the name of the library it's going to produce until way to late in the configuration process.

Lets go ahead with this.

bcraig updated this revision to Diff 54112.Apr 18 2016, 1:48 PM
bcraig updated this object.

EricWF's feedback. Removed libcxx_library option. I plan on submitting this tomorrow morning.

bcraig marked 2 inline comments as done.Apr 18 2016, 1:50 PM
EricWF accepted this revision.Apr 18 2016, 5:11 PM
EricWF edited edge metadata.

LGTM. It's up to you if you want to address the inline comments before committing.

Thanks for updating the Doc. I really need to kill one version of it.

test/libcxx/test/config.py
454

In this fallback case do we want to explicitly ask for static linkage using -Wl,-Bstatic -lc++ -Wl,-Bdynamic? I think that will cause the linker to diagnose that your not actually getting a static library.

473

In this fallback case do we want to explicitly ask for static linkage using -Wl,-Bstatic -lc++abi -Wl,-Bdynamic?

This revision is now accepted and ready to land.Apr 18 2016, 5:11 PM
bcraig closed this revision.Apr 19 2016, 5:55 AM

r266730

test/libcxx/test/config.py
454

While I would like to get a the diagnosis, I don't plan on making this change. The hexagon linker (and maybe others?) default to -Bstatic, so the "restoration" half of that snippet does the wrong thing. If there were a way to save / push the linker state, then restore / pop it then I would do that.