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

[Reid Kleckner via Phabricator via libcxx-commits]

rnk 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")
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128007