This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Do not share an object library to create the static/shared libraries
ClosedPublic

Authored by ldionne on Mar 12 2019, 6:19 AM.

Details

Summary

The problem with using an object library for doing this is that it prevents
the shared library and the static library from being built with the right
default flags. For example, CMake will build shared libraries with -fPIC
by default, but not static libraries. Using an object library to create
the shared library will prevent the right default flags for shared
libraries from being used.

As a side effect, this patch also localizes the logic related to building
a hermetic static library to the static library case, making clear that
this has no effect on the shared library.

Event Timeline

ldionne created this revision.Mar 12 2019, 6:19 AM
phosek accepted this revision.Mar 12 2019, 11:06 AM
phosek added a reviewer: smeenai.

LGTM, this looks great, I think we should do the same for libc++abi and libunwind. I've added @smeenai since he has CMake expertise.

libcxx/lib/CMakeLists.txt
209

CMake documentation says that [COMPILE_FLAGS is deprecated](https://cmake.org/cmake/help/v3.4/prop_tgt/COMPILE_FLAGS.html) and that we should be using COMPILE_OPTIONS or target_compile_options.

237

ditto

This revision is now accepted and ready to land.Mar 12 2019, 11:06 AM
ldionne marked an inline comment as done.Mar 12 2019, 1:22 PM
ldionne added inline comments.
libcxx/lib/CMakeLists.txt
209

If that's okay, I'd like to do that as a follow-up change. I'd do:

target_compile_options(cxx_shared PRIVATE ${LIBCXX_COMPILE_FLAGS})
target_link_options(cxx_shared PRIVATE ${LIBCXX_LINK_FLAGS})

The reason is that if this somehow breaks something (I'm worried that target_compile_options is additive where setting the property might overwrite the compile flags for that target, which could be different), at least I won't have to revert the whole patch.

smeenai added inline comments.Mar 12 2019, 2:56 PM
libcxx/lib/CMakeLists.txt
169

I think this can trivially be a function instead of a macro, which avoids potential future variable pollution.

209

One difference to be aware of (and this is really annoying behavior on CMake's part IMO) is that COMPILE_OPTIONS are deduplicated, such that if you e.g. tried setting COMPILE_OPTIONS to -isystem foo -isystem bar, CMake would transform into -isystem foo bar, which of course breaks horribly. Hopefully you won't run into that, but it's something to be aware of.

The downside, of course, is that we end up compiling the same files twice for the static and shared libraries, but it makes sense if they're gonna have different flags.

ldionne updated this revision to Diff 190408.Mar 13 2019, 6:54 AM
ldionne marked 2 inline comments as done.

Transform macro into function.

The downside, of course, is that we end up compiling the same files twice for the static and shared libraries, but it makes sense if they're gonna have different flags.

Yes, they do have different flags. But you're right, we're doing a bit more work now. Fortunately, building libc++ is very fast.

libcxx/lib/CMakeLists.txt
169

Good point.

209

I didn't know that! It's good to know.

This revision was automatically updated to reflect the committed changes.