[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 inline comments.


================
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:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > 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.
> > Phrasing it differently: If nothing is defined, any reference to symbols provided by libcxxabi, in code generated within libcxx, is generated as if it is going to be imported from a different DLL. If `_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS` is defined, the references are as if the code is going to be within the same DLL (which is true). 
> > 
> > If `_LIBCXXABI_BUILDING_LIBRARY` is defined, any reference to the symbols are as if they are in the same DLL (same as with `_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS`), but any time such a symbol is defined (which only would happen for inline functions in headers), it also generates an annotation in the object file, saying that this particular symbol should be exported out from the DLL.
> My point was that `_LIBCXXABI_BUILDING_LIBRARY` is not used anywhere in libc++, except when checking whether to enable availability attributes on Apple (which is unrelated to what you're trying to achieve). So adding this definition is effectively a no-op. Am I mistaken?
The libc++ code itself doesn't check `_LIBCXXABI_BUILDING_LIBRARY`, but it does affect what `_LIBCXXABI_FUNC_VIS` expands to, in the libcxxabi headers that are included by the libc++ sources.

If nothing is defined, `_LIBCXXABI_FUNC_VIS` expands to `__declspec(dllimport)`, which makes the generated libc++ code reference e.g. `__imp___cxa_uncaught_exceptions` instead of `__cxa_uncaught_exceptions` directly. With `_LIBCXXABI_BUILDING_LIBRARY` defined, it expands to `__declspec(dllexport)` instead, and the generated code references `__cxa_uncaught_exceptions`, likewise with `_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS`.


================
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:
> mstorsjo wrote:
> > 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.
> Right. I would still prefer that we go down the route I laid out above, since it simplifies things in addition to making them work for your use case.
Ok, I'll give that a shot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90021