[libcxx] Support building static library with hidden visibility

[phosek]

Summary

This is useful when static libc++ library is being linked into
shared libraries that may be used in combination with libraries.
We want to avoid we exporting libc++ symbols in those cases where
this option is useful. This is provided as a CMake option and can
be enabled by libc++ vendors as needed.

[ldionne]

So... the way we planned on tackling the hidden visibility problem is https://reviews.llvm.org/D54810 (which is blocked on a couple of Clang bugs w.r.t. visibility attributes). Would https://reviews.llvm.org/D54810 solve your problem?

Also, if your intent is to make _all_ symbols hidden, I don't think this patch achieves it because we still explicitly annotate symbols in the dylib as having default visibility. Am I mistaken?

[phosek]

In D55404#1323155, @ldionne wrote:

So... the way we planned on tackling the hidden visibility problem is https://reviews.llvm.org/D54810 (which is blocked on a couple of Clang bugs w.r.t. visibility attributes). Would https://reviews.llvm.org/D54810 solve your problem?

Also, if your intent is to make _all_ symbols hidden, I don't think this patch achieves it because we still explicitly annotate symbols in the dylib as having default visibility. Am I mistaken?

Our use case is static library while D54810 seems to focusing on shared case. Specifically, we need to link statically libc++ into various shared libraries and executables (e.g. drivers in Fuchsia) which might co-exist with other shared libraries and we need to ensure that no libc++ symbols are being exported. I suspect this might be also useful for other cases of "private libc++" such as libFuzzer.

The way this change achieves that is if you set the LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY option, it defines _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS which should disable all existing annotations and then explicitly sets -fvisibility=hidden -fvisibility-global-new-delete-hidden (only for static library, shared library is unaffected). Does this make sense?

[mclow.lists]

The current libc++ initialization mechanism depends on it being in a shared library.
You'll want to make sure stuff like this still works (will probably require code changes):

	#include <iostream>
	struct Foo {
	    Foo () { std::cout << "ctor"; }
	    ~Foo() { std::cout << "dtor"; }
	};

	Foo f;

	int main () {}

[ldionne]

In D55404#1323611, @phosek wrote:

In D55404#1323155, @ldionne wrote:

So... the way we planned on tackling the hidden visibility problem is https://reviews.llvm.org/D54810 (which is blocked on a couple of Clang bugs w.r.t. visibility attributes). Would https://reviews.llvm.org/D54810 solve your problem?

Also, if your intent is to make _all_ symbols hidden, I don't think this patch achieves it because we still explicitly annotate symbols in the dylib as having default visibility. Am I mistaken?

Our use case is static library while D54810 seems to focusing on shared case. Specifically, we need to link statically libc++ into various shared libraries and executables (e.g. drivers in Fuchsia) which might co-exist with other shared libraries and we need to ensure that no libc++ symbols are being exported. I suspect this might be also useful for other cases of "private libc++" such as libFuzzer.

Ok, this is the use case I imagined. Basically, you don't want the symbols from the libc++ static library to pollute what's exported by a shared library it might be linked into. That's perfectly reasonable.

The way this change achieves that is if you set the LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY option, it defines _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS which should disable all existing annotations and then explicitly sets -fvisibility=hidden -fvisibility-global-new-delete-hidden (only for static library, shared library is unaffected). Does this make sense?

Ahhh, this part is what I had missed. I didn't see you were disabling the visibility annotation. In that case everything is indeed hidden. Instead, I think a cleaner approach would be:

1. Build with hidden visibility by default at namespace scope (basically https://reviews.llvm.org/D54810), and

2. generalize https://github.com/llvm-mirror/libcxx/blob/master/CMakeLists.txt#L742 so that we can remove visibility annotations using a CMake option.

This way, we'd avoid adding yet another configuration option and instead leverage something we already (apparently) need and use on Windows. What do you think?

Also, I'd be curious about the relationship between this and the _LIBCPP_HIDE_FROM_ABI_PER_TU setting, which gives internal linkage to everything. Right now, it's possible to have both default visibility and internal linkage, but I'm not sure what that even means. Maybe both options should actually be merged under something more general like _LIBCPP_ALLOW_DISTRIBUTING_STATIC_LIBRARY, which would hide all symbols _and_ give internal linkage to everything. Not sure.

[phosek]

In D55404#1323828, @ldionne wrote:

Ahhh, this part is what I had missed. I didn't see you were disabling the visibility annotation. In that case everything is indeed hidden. Instead, I think a cleaner approach would be:

1. Build with hidden visibility by default at namespace scope (basically https://reviews.llvm.org/D54810), and

2. generalize https://github.com/llvm-mirror/libcxx/blob/master/CMakeLists.txt#L742 so that we can remove visibility annotations using a CMake option.

This way, we'd avoid adding yet another configuration option and instead leverage something we already (apparently) need and use on Windows. What do you think?

In general that sounds fine me, problem with generalizing https://github.com/llvm-mirror/libcxx/blob/master/CMakeLists.txt#L742 is that we also want to make sure that new and delete operators are hidden as well. Those operators may be implicitly declared by Clang with default visibility which defeats -fvisibility=hidden or explicit annotations which is why we've introduced -fvisibility-global-new-delete-hiddenwhich is being set in this change as well. I don't know if that's something that would affect Windows, and in general I don't know if it's something that should be enabled automatically for static libc++?

Also, I'd be curious about the relationship between this and the _LIBCPP_HIDE_FROM_ABI_PER_TU setting, which gives internal linkage to everything. Right now, it's possible to have both default visibility and internal linkage, but I'm not sure what that even means. Maybe both options should actually be merged under something more general like _LIBCPP_ALLOW_DISTRIBUTING_STATIC_LIBRARY, which would hide all symbols _and_ give internal linkage to everything. Not sure.

Do we even need _LIBCPP_HIDE_FROM_ABI_PER_TU now that we have __attribute__((__exclude_from_explicit_instantiation))? The only case I can think of are older versions of Clang or other compilers that don't support this attribute.

[ldionne]

In D55404#1324269, @phosek wrote:

In D55404#1323828, @ldionne wrote:

Ahhh, this part is what I had missed. I didn't see you were disabling the visibility annotation. In that case everything is indeed hidden. Instead, I think a cleaner approach would be:

1. Build with hidden visibility by default at namespace scope (basically https://reviews.llvm.org/D54810), and

2. generalize https://github.com/llvm-mirror/libcxx/blob/master/CMakeLists.txt#L742 so that we can remove visibility annotations using a CMake option.

This way, we'd avoid adding yet another configuration option and instead leverage something we already (apparently) need and use on Windows. What do you think?

In general that sounds fine me, problem with generalizing https://github.com/llvm-mirror/libcxx/blob/master/CMakeLists.txt#L742 is that we also want to make sure that new and delete operators are hidden as well. Those operators may be implicitly declared by Clang with default visibility which defeats -fvisibility=hidden or explicit annotations which is why we've introduced -fvisibility-global-new-delete-hiddenwhich is being set in this change as well. I don't know if that's something that would affect Windows, and in general I don't know if it's something that should be enabled automatically for static libc++?

I was going to say "why didn't you just fix -fvisibility=hidden to do the right thing for new and delete"? But then I read the discussion in https://reviews.llvm.org/D53787. However, after reading that, I'm strongly questioning whether you actually want new and delete to have hidden visibility. And if you actually do, it seems to me like the arguments in https://reviews.llvm.org/D53787 are somewhat defeated, no?

Also, I'd be curious about the relationship between this and the _LIBCPP_HIDE_FROM_ABI_PER_TU setting, which gives internal linkage to everything. Right now, it's possible to have both default visibility and internal linkage, but I'm not sure what that even means. Maybe both options should actually be merged under something more general like _LIBCPP_ALLOW_DISTRIBUTING_STATIC_LIBRARY, which would hide all symbols _and_ give internal linkage to everything. Not sure.

Do we even need _LIBCPP_HIDE_FROM_ABI_PER_TU now that we have __attribute__((__exclude_from_explicit_instantiation))? The only case I can think of are older versions of Clang or other compilers that don't support this attribute.

Yes, we do (at least in theory). _LIBCPP_HIDE_FROM_ABI_PER_TU makes sure that inline functions are not ODR-merged across translation units or static libraries, since we don't know whether they are token-by-token equivalent (for example if one TU has an older definition of the function and the other TU has a newer definition of the function). exclude_from_explicit_instantiation was added to workaround a completely different problem, which is that we were promising that some functions were defined in the dylib/static library when they really were not defined in the dylib/static library, which would cause link errors.

[phosek]

In D55404#1325906, @ldionne wrote:

I was going to say "why didn't you just fix -fvisibility=hidden to do the right thing for new and delete"? But then I read the discussion in https://reviews.llvm.org/D53787. However, after reading that, I'm strongly questioning whether you actually want new and delete to have hidden visibility. And if you actually do, it seems to me like the arguments in https://reviews.llvm.org/D53787 are somewhat defeated, no?

I'm not sure what you mean, can you elaborate on the last sentence? Without -fvisibility-global-new-delete-hidden we would end up with new and delete symbols being exported from the shared library.

[ldionne]

In D55404#1326554, @phosek wrote:

I'm not sure what you mean, can you elaborate on the last sentence? Without -fvisibility-global-new-delete-hidden we would end up with new and delete symbols being exported from the shared library.

Quoting richard in https://reviews.llvm.org/D53787#1282975:

These symbols really are special. Other symbols are introduced explicitly by a declaration, whereas these are declared implicitly by the compiler. Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program. Other symbols are generally provided in one library and consumed by users of that library, whereas these symbols are typically provided by the main binary and consumed by the libraries that it uses. And so on.

Basically, what I'm saying is that giving hidden visibility to new and delete seemed like a bad idea to Richard, and I follow his argument: those functions should be overridable by users but that won't work if you give them hidden visibility (right?).

The reason why I'm pushing back on this change as-is is that it makes our build logic even more complex than it already is (and it's way too complex as-is). TBH, I'm fine with the use case but I'd like us to find a better solution than just adding yet another build mode with different options. Finding a way to express this in the source would be better (because the source doesn't depend on a build mode), but like you point out it falls short for marking new and delete as hidden (which might not be a good idea, but whatever). Instead of doing visibility like we do today (in the source), perhaps we could instead have an explicit list of symbols that we export from the shared library? We would get rid of all visibility annotations from the sources and would do it externally, all in the build system. I've been told this is what libstdc++ has been doing and I think they're doing better than libc++ in that area.

[phosek]

In D55404#1327114, @ldionne wrote:

In D55404#1326554, @phosek wrote:

I'm not sure what you mean, can you elaborate on the last sentence? Without -fvisibility-global-new-delete-hidden we would end up with new and delete symbols being exported from the shared library.

Quoting richard in https://reviews.llvm.org/D53787#1282975:

These symbols really are special. Other symbols are introduced explicitly by a declaration, whereas these are declared implicitly by the compiler. Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program. Other symbols are generally provided in one library and consumed by users of that library, whereas these symbols are typically provided by the main binary and consumed by the libraries that it uses. And so on.

Basically, what I'm saying is that giving hidden visibility to new and delete seemed like a bad idea to Richard, and I follow his argument: those functions should be overridable by users but that won't work if you give them hidden visibility (right?).

Users can still override new and delete within the same shared library but not across the binary boundary. That's our intention, our goal is to make each shared library that statically links libc++ hermetic.

This makes me think that the name LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY I've chosen for this option may be misleading and causing confusion. I should probably change it to LIBCXX_HERMETIC_STATIC_LIBRARY to convey the intention. WDYT?

The reason why I'm pushing back on this change as-is is that it makes our build logic even more complex than it already is (and it's way too complex as-is). TBH, I'm fine with the use case but I'd like us to find a better solution than just adding yet another build mode with different options. Finding a way to express this in the source would be better (because the source doesn't depend on a build mode), but like you point out it falls short for marking new and delete as hidden (which might not be a good idea, but whatever). Instead of doing visibility like we do today (in the source), perhaps we could instead have an explicit list of symbols that we export from the shared library? We would get rid of all visibility annotations from the sources and would do it externally, all in the build system. I've been told this is what libstdc++ has been doing and I think they're doing better than libc++ in that area.

I agree that this is making the build logic more complicated and I can try to to simplify it a bit more. I also agree that your suggestion would be an improvement, but I think it's orthogonal to what we're trying to achieve. If we changed libc++ to use explicit list, then in the hermetic case the list would be empty but we would still want to use -fvisibility=hidden -fvisibility-global-new-delete-hidden, so the only difference compared to current patch would be omitting the _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS definition which is still an improvement but fairly minor compared to the rest of this change.

[phosek]

Do you have more opinions on this?

[ldionne]

In D55404#1328449, @phosek wrote:

This makes me think that the name LIBCXX_HIDDEN_VISIBILITY_IN_STATIC_LIBRARY I've chosen for this option may be misleading and causing confusion. I should probably change it to LIBCXX_HERMETIC_STATIC_LIBRARY to convey the intention. WDYT?

Yes, I think something along the lines of LIBCXX_HERMETIC_STATIC_LIBRARY would be better, along with some documentation for what this means and what the intent is.

The reason why I'm pushing back on this change as-is is that it makes our build logic even more complex than it already is (and it's way too complex as-is). TBH, I'm fine with the use case but I'd like us to find a better solution than just adding yet another build mode with different options. Finding a way to express this in the source would be better (because the source doesn't depend on a build mode), but like you point out it falls short for marking new and delete as hidden (which might not be a good idea, but whatever). Instead of doing visibility like we do today (in the source), perhaps we could instead have an explicit list of symbols that we export from the shared library? We would get rid of all visibility annotations from the sources and would do it externally, all in the build system. I've been told this is what libstdc++ has been doing and I think they're doing better than libc++ in that area.

I agree that this is making the build logic more complicated and I can try to to simplify it a bit more. I also agree that your suggestion would be an improvement, but I think it's orthogonal to what we're trying to achieve. If we changed libc++ to use explicit list, then in the hermetic case the list would be empty but we would still want to use -fvisibility=hidden -fvisibility-global-new-delete-hidden,

I don't understand this -- I don't think we'd export new and delete if they are not part of the export list, no? I would think that you could build normally (without any flags) and just use an empty list of exported symbols?

[ldionne]

Like I said -- I don't like that we're making the build more complex but I don't want to block you over that for the whole holiday break. LGTM but I think we should try to simplify the build (I know you agree, we just need to figure out how).

[phosek]

In D55404#1337691, @ldionne wrote:

Yes, I think something along the lines of LIBCXX_HERMETIC_STATIC_LIBRARY would be better, along with some documentation for what this means and what the intent is.

Done, I've also updated the documentation.

I don't understand this -- I don't think we'd export new and delete if they are not part of the export list, no? I would think that you could build normally (without any flags) and just use an empty list of exported symbols?

They would be exported because they're defined by the compiler which uses default visibility by default. I tried it locally and it's the case without the -fvisibility-global-new-delete-hidden option.

[ldionne]

In D55404#1339724, @phosek wrote:

In D55404#1337691, @ldionne wrote:

Yes, I think something along the lines of LIBCXX_HERMETIC_STATIC_LIBRARY would be better, along with some documentation for what this means and what the intent is.

Done, I've also updated the documentation.

I don't understand this -- I don't think we'd export new and delete if they are not part of the export list, no? I would think that you could build normally (without any flags) and just use an empty list of exported symbols?

They would be exported because they're defined by the compiler which uses default visibility by default. I tried it locally and it's the case without the -fvisibility-global-new-delete-hidden option.

Well.. We'd also have to provide an explicit list of symbols that should _not_ be exported from the dylib (there's probably a linker option for that). Meh

libcxx/trunk/CMakeLists.txt

 
 set(LIBCXX_CONFIGURE_IDE_DEFAULT OFF)
 if (XCODE OR MSVC_IDE)
   set(LIBCXX_CONFIGURE_IDE_DEFAULT ON)
 endif()
 option(LIBCXX_CONFIGURE_IDE "Configure libcxx for use within an IDE"
       ${LIBCXX_CONFIGURE_IDE_DEFAULT})
 
+option(LIBCXX_HERMETIC_STATIC_LIBRARY
+  "Do not export any symbols from the static library." OFF)
+
 #===============================================================================
 # Check option configurations
 #===============================================================================
 
 if (LIBCXX_ENABLE_FILESYSTEM AND NOT LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY)
   message(FATAL_ERROR
     "LIBCXX_ENABLE_FILESYSTEM cannot be turned on when LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF")
 endif()

libcxx/trunk/docs/BuildingLibcxx.rst

.. option:: LIBCXX_LIBDIR_SUFFIX:STRING
   This option overrides `LLVM_LIBDIR_SUFFIX`.
 
 .. option:: LIBCXX_INSTALL_PREFIX:STRING
 
   **Default**: ``""``
 
   Define libc++ destination prefix.
 
+.. option:: LIBCXX_HERMETIC_STATIC_LIBRARY:BOOL
+
+  **Default**: ``OFF``
+
+  Do not export any symbols from the static libc++ library. This is useful when
+  This is useful when the static libc++ library is being linked into shared
+  libraries that may be used in with other shared libraries that use different
+  C++ library. We want to avoid avoid exporting any libc++ symbols in that case.
+
 .. _libc++experimental options:
 
 libc++experimental Specific Options
 ------------------------------------
 
 .. option:: LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY:BOOL
 
   **Default**: ``ON``

libcxx/trunk/lib/CMakeLists.txt

if (NOT LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS)
       add_link_flags("-Wl,-reexported_symbols_list,\"${CMAKE_CURRENT_SOURCE_DIR}/libc++abi-new-delete.exp\"")
     endif()
   endif()
 endif()
 
 split_list(LIBCXX_COMPILE_FLAGS)
 split_list(LIBCXX_LINK_FLAGS)
 
+macro(cxx_object_library name)
+  cmake_parse_arguments(ARGS "" "" "DEFINES;FLAGS" ${ARGN})
+
   # Add an object library that contains the compiled source files.
-add_library(cxx_objects OBJECT ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
+  add_library(${name} OBJECT ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
   if(LIBCXX_CXX_ABI_HEADER_TARGET)
-  add_dependencies(cxx_objects ${LIBCXX_CXX_ABI_HEADER_TARGET})
+    add_dependencies(${name} ${LIBCXX_CXX_ABI_HEADER_TARGET})
   endif()
   if(WIN32 AND NOT MINGW)
-  target_compile_definitions(cxx_objects
+    target_compile_definitions(${name}
                                PRIVATE
                                  # Ignore the -MSC_VER mismatch, as we may build
                                  # with a different compatibility version.
                                  _ALLOW_MSC_VER_MISMATCH
                                  # Don't check the msvcprt iterator debug levels
                                  # as we will define the iterator types; libc++
                                  # uses a different macro to identify the debug
                                  # level.
                                  _ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH
                                  # We are building the c++ runtime, don't pull in
                                  # msvcprt.
                                  _CRTBLD
                                  # Don't warn on the use of "deprecated"
                                  # "insecure" functions which are standards
                                  # specified.
                                  _CRT_SECURE_NO_WARNINGS
                                  # Use the ISO conforming behaviour for conversion
                                  # in printf, scanf.
                                  _CRT_STDIO_ISO_WIDE_SPECIFIERS)
   endif()
 
-set_target_properties(cxx_objects
+  if(ARGS_DEFINES)
+    target_compile_definitions(${name} PRIVATE ${ARGS_DEFINES})
+  endif()
+
+  set_target_properties(${name}
     PROPERTIES
-    COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
+      COMPILE_FLAGS ${LIBCXX_COMPILE_FLAGS}
   )
 
+  if(ARGS_FLAGS)
+    target_compile_options(${name} PRIVATE ${ARGS_FLAGS})
+  endif()
+endmacro()
+
+if(LIBCXX_HERMETIC_STATIC_LIBRARY)
+  append_flags_if_supported(CXX_STATIC_OBJECTS_FLAGS -fvisibility=hidden)
+  append_flags_if_supported(CXX_STATIC_OBJECTS_FLAGS -fvisibility-global-new-delete-hidden)
+  cxx_object_library(cxx_static_objects
+    DEFINES _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS
+    FLAGS ${CXX_STATIC_OBJECTS_FLAGS})
+  cxx_object_library(cxx_shared_objects)
+  set(cxx_static_sources $<TARGET_OBJECTS:cxx_static_objects>)
+  set(cxx_shared_sources $<TARGET_OBJECTS:cxx_shared_objects>)
+else()
+  cxx_object_library(cxx_objects)
+  set(cxx_static_sources $<TARGET_OBJECTS:cxx_objects>)
+  set(cxx_shared_sources $<TARGET_OBJECTS:cxx_objects>)
+endif()
+
 # Build the shared library.
 if (LIBCXX_ENABLE_SHARED)
-  add_library(cxx_shared SHARED $<TARGET_OBJECTS:cxx_objects>)
+  add_library(cxx_shared SHARED ${cxx_shared_sources})
   if(COMMAND llvm_setup_rpath)
     llvm_setup_rpath(cxx_shared)
   endif()
   target_link_libraries(cxx_shared ${LIBCXX_LIBRARIES})
   set_target_properties(cxx_shared
     PROPERTIES
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       OUTPUT_NAME   "c++"
if(WIN32 AND NOT MINGW AND NOT "${CMAKE_HOST_SYSTEM_NAME}" STREQUAL "Windows")
     # will attempt to use the manifest tool to generate the bundled manifest
     set_target_properties(cxx_shared PROPERTIES
                           APPEND_STRING PROPERTY LINK_FLAGS " /MANIFEST:NO")
   endif()
 endif()
 
 # Build the static library.
 if (LIBCXX_ENABLE_STATIC)
-  add_library(cxx_static STATIC $<TARGET_OBJECTS:cxx_objects>)
+  add_library(cxx_static STATIC ${cxx_static_sources})
   target_link_libraries(cxx_static ${LIBCXX_LIBRARIES})
   set(CMAKE_STATIC_LIBRARY_PREFIX "lib")
   set_target_properties(cxx_static
     PROPERTIES
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       OUTPUT_NAME   "c++"
   )
   list(APPEND LIBCXX_BUILD_TARGETS "cxx_static")