[libc++] Simplify the visibility attributes

[ldionne]

libcxx/include/__config

558

If we want to eventually refactor the Windows side of availability macros, I think we should have _LIBCPP_VISIBILITY_HIDDEN and _LIBCPP_VISIBILITY_DEFAULT instead of a macro parameter.

[philnik]

• Use a new approach

libcxx/include/__config

558

There isn't really any common ground between non-COFF and COFF in terms of visibility macros. In MinGW only _LIBCPP_OVERRIDABLE_FUNC_VIS and _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS are even defined non-empty, so I don't think it makes a lot of sense to try to unify non-COFF and COFF visibility macros.

[philnik]

Rebased

[ldionne]

This is indeed much simpler.

[ayzhao]

libcxx/include/__config

572

This is causing breakages in Chromium because we're unable to set _LIBCPP_OVERRIDABLE_FUNC_VIS at build time - see https://crbug.com/1343370#c6

[philnik]

libcxx/include/__config

572

I'm not sure I understand why you set _LIBCPP_OVERRIDABLE_FUNC_VIS (which isn't customization point AFAIK). If you're building against the dylib the visibility should be default and if you don't you set _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS so the symbols become whatever the set visibility is. Why doesn't that work for your configuration?

[rnk]

libcxx/include/__config

572

Chromium is setting _LIBCPP_DISABLE_VISIBILITY_ANOTATIONS because it is linking its own copy of libc++ statically.

I think Chromium doesn't need overridability of this macro, it just needs to ensure that operator new is not given hidden visibility, it has to always have default visibility, even when libc++ visibility annotations are disabled. So, maybe an alternative fix here would be to always define _LIBCPP_OVERRIDEABLE_FUNC_VIS to __attribute__((visibility("default"))) instead of using _LIBCPP_VISIBILITY. That seems like the right thing to do when libc++ is being statically linked, which I think is the use case for disabling libc++ visibility annotations. WDYT?

[philnik]

libcxx/include/__config

572

Disabling the visibility annotations is interesting for people who build dynamic libraries and don't want to depend on libc++ dynamically. That is only possible by hiding all symbols, including operator new. AFAIK Chromium is linking everything statically into an executable and doesn't build dynamic libraries. There it should be irrelevant whether _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is set or not. Or am I missing something?

[rnk]

libcxx/include/__config

572

I read more Chromium comments, and it seems that this actually has to do with tcmalloc. If you link libc++ statically *and* use a malloc replacement library such as tcmalloc, the libc++ declarations must not be marked hidden in order for the override to work across dynamic libraries. tcmalloc needs to override operator new / delete in other dynamic libraries. Linking libc++ statically should not prevent users from exporting their own operator new override, right?

Since I don't think this was an intentional behavior change, can we please revert to the previous behavior by bringing back the ifndef? If you want to drop support for this override point, we need some other solution for this use case.

See the full comments if you like:

defines += [
        # This resets the visibility to default only for the various
        # flavors of operator new and operator delete.  These symbols
        # are weak and get overriden by Chromium-provided ones, but if
        # these symbols had hidden visibility, this would make the
        # Chromium symbols hidden too because elf visibility rules
        # require that linkers use the least visible form when merging,
        # and if this is hidden, then when we merge it with tcmalloc's
        # operator new, hidden visibility would win. However, tcmalloc
        # needs a visible operator new to also override operator new
        # references from system libraries.
        # TODO(lld): Ask lld for a --force-public-visibility flag or
        # similar to that overrides the default elf merging rules, and
        # make tcmalloc's gn config pass that to all its dependencies,
        # then remove this override here.
        "_LIBCPP_OVERRIDABLE_FUNC_VIS=__attribute__((__visibility__(\"default\")))",
      ]

[philnik]

libcxx/include/__config

572

What I don't understand is why Chromium disables the visibility macros in the first place and why the default visibility is hidden. AFAIK it shouldn't make any difference, since visibility is somewhat irrelevant for static linking.

[rnk]

libcxx/include/__config

572

Chrome does not export a C++ interface, so it compiles with -fvisibility=hidden and disables the visibility macros, which would make libc++ symbols visible outside the DSO.

It's not clear to me why tcmalloc needs to override operator new in other system DSOs, but that is, apparently, a requirement, and it interacts with these annotations in libc++.

[philnik]

libcxx/include/__config

572

But Chromium doesn't export any interface at all. Or is it itself a shared library?

[rnk]

libcxx/include/__config

572

I don't know the details, but compiling with -fvisibility=hidden and limiting default visibility symbols is a standard best practice, even if Chrome ultimately becomes an executable with no dynamic symbols. The old Drepper article comes to mind. Maybe to export tcmalloc operator new, Chrome uses the --export-dynamic linker flag.

So, to recap, I'm trying to argue that this use case is valid:

• Building a fully static binary with libc++ with ~no dynamic symbols
• Linking in tcmalloc to override all malloc and operator new symbols across the program

This is the use case which requires overriding the visibility of these symbols, but we/Chrome would be happy with any short term solution for this use case.

[philnik]

libcxx/include/__config

572

I'm not saying the use-case isn't valid. I'm saying that you can avoid your problem by

1. Not setting _LIBCPP_DISBALE_VISIBILITY_ANNOTATIONS which would give you the visibility attributes you want, or
2. Not using -fvisibility=hidden

I don't understand why these options (especially 1) don't work for you.

[hans]

libcxx/include/__config

572

I'm not familiar with these macros, but here is some archaeology:

Our use of _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS seems to have started at https://chromium-review.googlesource.com/c/chromium/buildtools/+/518311/

The scope was broadened by https://chromium-review.googlesource.com/c/chromium/src/+/601529/
I think the motivation of that one relates to this question:

AFAIK Chromium is linking everything statically into an executable and doesn't build dynamic libraries. There it should be irrelevant whether _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is set or not. Or am I missing something?

While we link our code statically, we do load dynamic libraries from the system, and we don't want them to use our libc++ symbols because that breaks things (from http://crbug.com/751360 "gl_unittests loads libGLESv2.so which also statically links libc++, but because of the visibility for libc++ symbols is set to default, the dynamic linker resolves libGLESv2.so's symbol for the vtable of 'std::__1::basic_streambuf<char>' with the one from gl_unittests body later causing the CFI unrelated cast exception")

.. except we do want default visibility for new/delete, I guess for tcmalloc as rnk mentioned above (the macro was first set in https://codereview.chromium.org/2937433003).

[philnik]

libcxx/include/__config

572

Are you saying that libGLES exports libc++ symbols? That sounds a lot like a libGLES bug to me. If they're linking statically they shouldn't export any symbols from libc++.

Anyways, I've uploaded 0f050528fd08 for now.

[rnk]

libcxx/include/__config

572

Are you saying that libGLES exports libc++ symbols? That sounds a lot like a libGLES bug to me.

That's possible, but I think it's worth focusing on the executable, gl_unittests, which is exporting the vtable symbol for std::__1::basic_streambuf<char>. It doesn't matter that gl_unittests is an executable, it still exports default visibility symbols, and that's why Chrome uses both -fvisibility=hidden + _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS.

Anyway, thank you for landing a workaround.

libcxx/include/__config

 #    define _LIBCPP_HIDDEN
 #    define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
 #    define _LIBCPP_TEMPLATE_VIS
 #    define _LIBCPP_TEMPLATE_DATA_VIS
 #    define _LIBCPP_ENUM_VIS
 
 #  endif // defined(_LIBCPP_OBJECT_FORMAT_COFF)
 
-#  ifndef _LIBCPP_HIDDEN
-#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#      define _LIBCPP_HIDDEN __attribute__((__visibility__("hidden")))
-#    else
-#      define _LIBCPP_HIDDEN
-#    endif
+#  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
+#    define _LIBCPP_VISIBILITY(vis) __attribute__((__visibility__(vis)))
+#  else
+#    define _LIBCPP_VISIBILITY(vis)
+#  endif
+
+#  ifndef _LIBCPP_HIDDEN
+#    define _LIBCPP_HIDDEN _LIBCPP_VISIBILITY("hidden")
 #  endif
 
 #  ifndef _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
 #    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 // The inline should be removed once PR32114 is resolved
 #      define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS inline _LIBCPP_HIDDEN
 #    else
 #      define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
 #    endif
 #  endif
 
 #  ifndef _LIBCPP_FUNC_VIS
-#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#      define _LIBCPP_FUNC_VIS __attribute__((__visibility__("default")))
-#    else
-#      define _LIBCPP_FUNC_VIS
-#    endif
+#    define _LIBCPP_FUNC_VIS _LIBCPP_VISIBILITY("default")
 #  endif
 
 #  ifndef _LIBCPP_TYPE_VIS
-#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#      define _LIBCPP_TYPE_VIS __attribute__((__visibility__("default")))
-#    else
-#      define _LIBCPP_TYPE_VIS
-#    endif
+#    define _LIBCPP_TYPE_VIS _LIBCPP_VISIBILITY("default")
 #  endif
 
 #  ifndef _LIBCPP_TEMPLATE_VIS
 #    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
 #      if __has_attribute(__type_visibility__)
 #        define _LIBCPP_TEMPLATE_VIS __attribute__((__type_visibility__("default")))
 #      else
 #        define _LIBCPP_TEMPLATE_VIS __attribute__((__visibility__("default")))
 #      endif
 #    else
 #      define _LIBCPP_TEMPLATE_VIS
 #    endif
 #  endif
 
 #  ifndef _LIBCPP_TEMPLATE_DATA_VIS
-#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#      define _LIBCPP_TEMPLATE_DATA_VIS __attribute__((__visibility__("default")))
-#    else
-#      define _LIBCPP_TEMPLATE_DATA_VIS
-#    endif
+#    define _LIBCPP_TEMPLATE_DATA_VIS _LIBCPP_VISIBILITY("default")
 #  endif
 
 #  ifndef _LIBCPP_EXPORTED_FROM_ABI
-#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#      define _LIBCPP_EXPORTED_FROM_ABI __attribute__((__visibility__("default")))
-#    else
-#      define _LIBCPP_EXPORTED_FROM_ABI
-#    endif
+#    define _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_VISIBILITY("default")
 #  endif
 
 #  ifndef _LIBCPP_OVERRIDABLE_FUNC_VIS
 #    define _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_FUNC_VIS
 #  endif
 
 #  ifndef _LIBCPP_EXCEPTION_ABI
-#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#      define _LIBCPP_EXCEPTION_ABI __attribute__((__visibility__("default")))
-#    else
-#      define _LIBCPP_EXCEPTION_ABI
-#    endif
+#    define _LIBCPP_EXCEPTION_ABI _LIBCPP_VISIBILITY("default")
 #  endif
 
 #  ifndef _LIBCPP_ENUM_VIS
 #    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS) && __has_attribute(__type_visibility__)
 #      define _LIBCPP_ENUM_VIS __attribute__((__type_visibility__("default")))
 #    else
 #      define _LIBCPP_ENUM_VIS
 #    endif
 #  endif
 
 #  ifndef _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
-#    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#      define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __attribute__((__visibility__("default")))
-#    else
-#      define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
-#    endif
+#    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS _LIBCPP_VISIBILITY("default")
 #  endif
 
 #  ifndef _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 #    define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 #  endif
 
 #  if __has_attribute(internal_linkage)
 #    define _LIBCPP_INTERNAL_LINKAGE __attribute__((internal_linkage))

 
 #  if __has_attribute(no_destroy)
 #    define _LIBCPP_NO_DESTROY __attribute__((__no_destroy__))
 #  else
 #    define _LIBCPP_NO_DESTROY
 #  endif
 
 #  ifndef _LIBCPP_HAS_NO_ASAN
-extern "C" _LIBCPP_FUNC_VIS void __sanitizer_annotate_contiguous_container(const void*, const void*, const void*,
-                                                                           const void*);
+    extern "C" _LIBCPP_FUNC_VIS void
+    __sanitizer_annotate_contiguous_container(const void*, const void*, const void*, const void*);
 #  endif
 
 // Try to find out if RTTI is disabled.
 #  if defined(_LIBCPP_COMPILER_CLANG_BASED) && !__has_feature(cxx_rtti)
 #    define _LIBCPP_NO_RTTI
 #  elif defined(__GNUC__) && !defined(__GXX_RTTI)
 #    define _LIBCPP_NO_RTTI
 #  elif defined(_LIBCPP_COMPILER_MSVC) && !defined(_CPPRTTI)