Page MenuHomePhabricator

[libcxx] Have separate abi library flags for static and shared builds
Needs ReviewPublic

Authored by martell on May 28 2017, 3:28 PM.

Details

Summary

While we can enable both LIBCXX_ENABLE_SHARED and LIBCXX_ENABLE_STATIC because we only build one set of objects this inadvertently ends up messing up the flags of one of the two builds.
This is because _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS should be set for a static build but not for a shared build.

Also on that note static and shared builds need to be able to decide which type of abi library to link to independently so
I have introduced LIBCXX_ENABLE_STATIC_ABI_LIBRARY_FOR_STATIC and LIBCXX_ENABLE_STATIC_ABI_LIBRARY_FOR_SHARED
(better naming suggestions are also welcome)

The default is currently off for both but I would like to suggest we set LIBCXX_ENABLE_STATIC_ABI_LIBRARY_FOR_STATIC to ON by default.
I believe static builds by default should try to use a static libc++abi and combine them into one library.
Shared builds should remain using a shared libc++abi as the default.

This gives the power at the cmake level to override this and link shared with static and static with shared if desired.

I also cleaned up some small nits also.
llvm-ar exports COFF objects as .obj so the python script to combine the libs should allow not just .o
compiler-rt is added to the default libs in config-ix.cmake so we should not manually add -rtlib=compiler-rt to the link flags

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.May 28 2017, 3:28 PM
martell updated this revision to Diff 100573.May 28 2017, 3:59 PM

refactor commit to only intended changes

@mstorsjo: I created this patch so we could build static and shared libcxx for windows in the one build.
This also addresses issues so that we can combine libcxxabi and libcxx into the one libcxx library using llvm-ar.

There is also one other patch for unwind support, while I understand exceptions in general and a little at the assembly level implementing that piece is currently outside my skillset.
I will add you as a reviewer there also, hopefully I can find time to address the comments from others over the weekend.

mstorsjo edited edge metadata.Aug 30 2017, 1:58 PM

@martell - I forgot to look at this patch - I've been experimenting with the same, with building libc++ and libc++abi for mingw now.

Full static builds run fine (requiring quite a bit of cmake fiddling though, and a few minor patches that I've sent separately now, that I see that you've fixed similarly more or less here, bundled up into the big change), but the shared builds fail with some symbols that aren't properly dllexported/imported across the border between libc++ and libc++abi. Did you run into the same issues, and are they fixed by doing what you suggest here (linking in libc++abi statically into libc++.dll)? To do this, did you need to build libc++abi with some extra cmake flags, like _LIBCPP_BUILDING_LIBRARY, to pretend it's all part of the same library build?

martell updated this revision to Diff 114759.Sep 11 2017, 8:21 PM

update to HEAD

@mstorsjo sorry I missed that comment before. I currently succeed in building shared and static versions by just changing
# define _LIBCPP_DLL_VIS __declspec(dllimport) to # define _LIBCPP_DLL_VIS in __config and applying D33620

The correct fix is probably by editing libcxxabi to correctly export symbols.
The real issue here though is when we are using a static libc++ clang needs to be able to set _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS when using static at compile time which is messy.
I am avoiding that by doing that one line change above and the patch linked.

@mstorsjo sorry I missed that comment before. I currently succeed in building shared and static versions by just changing
# define _LIBCPP_DLL_VIS __declspec(dllimport) to # define _LIBCPP_DLL_VIS in __config and applying D33620

The correct fix is probably by editing libcxxabi to correctly export symbols.
The real issue here though is when we are using a static libc++ clang needs to be able to set _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS when using static at compile time which is messy.
I am avoiding that by doing that one line change above and the patch linked.

Hmm, maybe that's the same issue that I'm working around by passing -flto-visibility-public-std, which makes it skip the implicit dllimport for some implicitly defined C++ symbols.

Hmm, maybe that's the same issue that I'm working around by passing -flto-visibility-public-std, which makes it skip the implicit dllimport for some implicitly defined C++ symbols.

Sorry, that's actually -Xclang -flto-visibility-public-std. In current testing (without building libcxx as a DLL), I'm building calling code with these flags:

-fno-exceptions -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -Xclang -flto-visibility-public-std
martell added a subscriber: compnerd.EditedSep 12 2017, 3:39 PM

Hmm, maybe that's the same issue that I'm working around by passing -flto-visibility-public-std, which makes it skip the implicit dllimport for some implicitly defined C++ symbols.

Sorry, that's actually -Xclang -flto-visibility-public-std. In current testing (without building libcxx as a DLL), I'm building calling code with these flags:

-fno-exceptions -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -Xclang -flto-visibility-public-std

Thanks for the update, I will try this out soon.

This patch resolves the issue surrounding the objects having to be built separately for static and dll builds when trying to do both in the one cmake run.
Let's try and get this reviewed and merged first and then dig back into why we have issues with dll import and -flto-visibility-public-std.

We can create a new thread on the mailing list and ask @compnerd to weigh in on what he thinks is best to do about that.

Hey, @EricWF, @rnk would wither of you be in a position to do a review of this :)

martell added inline comments.Sep 12 2017, 3:44 PM
utils/merge_archives.py
121

It seems this landed separately in rL313072 so I can remove this line.
Please ignore this line for the review

I don't think llvm-ar should output COFF objects as .obj just because they are COFF anyway.
It is a gnu style tool and should output all objects as .o

mstorsjo added inline comments.Sep 12 2017, 9:21 PM
utils/merge_archives.py
121

I don't think llvm-ar should output COFF objects as .obj just because they are COFF anyway.
It is a gnu style tool and should output all objects as .o

It's not something that llvm-ar decides, but when creating the static library, the input files are named *.obj (which is something that cmake decides, whenever the target is windows, regardless of whether using gnu style or msvc style tools).

mstorsjo added inline comments.Sep 13 2017, 2:04 AM
lib/CMakeLists.txt
108

This line isn't necessary any longer, something similar was committed in SVN r312498; see line 94 above.