[libcxx-commits] [PATCH] D128007: [libc++] Simplify the visibility attributes

Wed Jul 13 11:15:04 PDT 2022

philnik added inline comments.

================
Comment at: libcxx/include/__config:543
+#    define _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_VISIBILITY("default")
+#    define _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_VISIBILITY("default")
+#    define _LIBCPP_EXCEPTION_ABI _LIBCPP_VISIBILITY("default")
----------------
rnk wrote:
> philnik wrote:
> > ayzhao wrote:
> > > 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
> > 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?
> 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?
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128007