[libc++] Remove the option to change _LIBCPP_OVERRIDABLE_FUNC_VIS

[philnik]

Summary

Chromium currently overrides this even though it isn't a customization point. Remove the option again once Chromium fixes the bugs this option circumvents.

[Mordante]

Is there anything in the release notes that it's the intention to remove this?
We know it broke Chromium, but we don't know how many other might be broken.
Instead I would suggest to follow our normal removal route once Chromium is fixed:

• Remove it, but add an opt out
• Add information in the release notes, requesting people to reach out when they use it
• Remove the opt out in a later release

[philnik]

Is there anything in the release notes that it's the intention to remove this?

No, there isn't. Normally we don't release-note refactoring internal code.

We know it broke Chromium, but we don't know how many other might be broken.

I suspect there aren't a lot of people who do this. You have to link libc++ statically, define _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS, use -fvisibility=hidden and want a custom operator new/operator delete that is in a (different) dylib.

Instead I would suggest to follow our normal removal route once Chromium is fixed:

• Remove it, but add an opt out
• Add information in the release notes, requesting people to reach out when they use it
• Remove the opt out in a later release

Do you have a suggestion of how that opt-out should look like?

[rnk]

"... once Chromium fixes the bugs this option circumvents"

When this issue is ultimately resolved, I think it will require a libc++ customization point. I don't see a good way to use fvisibility=hidden, hide libc++ symbols, but override operator new & delete with default visibility symbols, without an ifdef here. Certainly, we can come up with a better customization point than this ifdef. I just mean that it's highly unlikely that this patch should land as is. Instead there will be other libc++ changes.

libcxx/include/__config

 #      define _LIBCPP_VISIBILITY(vis)
 #    endif
 
 #    define _LIBCPP_HIDDEN _LIBCPP_VISIBILITY("hidden")
 #    define _LIBCPP_FUNC_VIS _LIBCPP_VISIBILITY("default")
 #    define _LIBCPP_TYPE_VIS _LIBCPP_VISIBILITY("default")
 #    define _LIBCPP_TEMPLATE_DATA_VIS _LIBCPP_VISIBILITY("default")
 #    define _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_VISIBILITY("default")
+#    define _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_VISIBILITY("default")
 #    define _LIBCPP_EXCEPTION_ABI _LIBCPP_VISIBILITY("default")
 #    define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS _LIBCPP_VISIBILITY("default")
 #    define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
 
-// TODO: Make this a proper customization point or remove the option to override it.
-#    ifndef _LIBCPP_OVERRIDABLE_FUNC_VIS
-#      define _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_VISIBILITY("default")
-#    endif
-
 #    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
 
 #    if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)