[libcxx-commits] [PATCH] D90021: [libcxx] [libcxxabi] Set flags for visibility when statically linking libcxxabi into libcxx for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

In D90021#2354009 <https://reviews.llvm.org/D90021#2354009>, @rnk wrote:

> I read the commit message, and what you said makes sense to me. As I understand it, there are two supported configs on Windows:
>
> - Shared: libc++ and libc++abi as one DLL.
> - Static: both libc++ and libc++abi are static libraries.

... with an option in libcxx to merge the static libc++abi into the static libc++

But yeah, if using both libraries together, this pretty much are the only working combinations. Additionally, there's also the option to use libc++ standalone on top of MSVC's ABI runtime, both as static and dynamic library.

> Is it possible to simplify the cmake conditionals if we add some checks for that?

I think that might be possible yeah, but not sure how big variance there is in how this all is hooked up, as most existing setups probably have done more or less of adhoc hackery to get it to work. (My builds so far amounted to building them each standalone, by passing these flags manually with `CMAKE_CXX_FLAGS`.)



================
Comment at: libcxx/CMakeLists.txt:874
+    # instead of dllimport.
+    add_definitions(-D_LIBCXXABI_BUILDING_LIBRARY)
+  elseif(LIBCXX_ENABLE_STATIC AND LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY)
----------------
ldionne wrote:
> Why does `_LIBCXXABI_BUILDING_LIBRARY` have an impact when building libc++?
In practice, `_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS` works essentially the same. If there's inline definitions in headers that would need to be exported, I think `_LIBCXXABI_BUILDING_LIBRARY` would be a bit more consistent (as the statically linked libcxxabi would essentially be a part of the final libc++ DLL), but I'm not sure it matters.


================
Comment at: libcxxabi/CMakeLists.txt:290
+    # Define to the empty string (if just defined as -DFOO, it defines it to
+    # the value 1) as we have a number of "#define _LIBCPP_BUILDING_LIBRARY"
+    # in certain source files.
----------------
ldionne wrote:
> IMO, the right thing to do here is to get rid of the places where we `#define _LIBCPP_BUILDING_LIBRARY` in `libcxxabi`, and define `_LIBCPP_BUILDING_LIBRARY` unconditionally when building libc++abi. Then, as a follow-up change, we should clarify the status of these source files that are shared/duplicated between libc++ and libc++abi (that's `stdlib_new_delete.cpp` and `stdlib_exception.cpp` AFAICT).
fallback_malloc.cpp also has an instance of it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90021/new/

https://reviews.llvm.org/D90021