Suggested _LIBCPP_OVERRIDABLE_FUNC_VIS replacement doesn't work

[nico]

#113139 removed _LIBCPP_OVERRIDABLE_FUNC_VIS in favor of -fvisibility-global-new-delete=. But -fvisibility-global-new-delete= doesn't work.

The feature exists to allow binaries to intercept global operator new / operator delete and route them to their own allocators.

Imagine an executable that has these as public symbols, and then shared libraries loaded into that executable pick up those symbols.

Problems with -fvisibility-global-new-delete=, in order or severity:

1. It only affects the overloads of op new / delete in https://eel.is/c++draft/basic.stc.dynamic.general from the language part, while libc++ declares all the overloads in https://eel.is/c++draft/new.delete . With _LIBCPP_OVERRIDABLE_FUNC_VIS=__attribute__((visibiliity("default"))), all the overloads became public. With -fvisibility-global-new-delete=default, the nothrow overloads stay hidden (when generally building with -fvisibility=hidden). Since libc++ itself calls these overloads sometimes, a shared library loaded into an executable built with the suggested replacement doesn't work: The executable will only have the no-nothrow overloads exported, and the nothrow overloads private, and so the allocator gets partially overriden for the shared library, breaking things. (This is the main concern.) I'll attach a minimal repro for this, but I hope the description here is pretty clear.

2. _LIBCPP_OVERRIDABLE_FUNC_VIS allows applying dllexport / dllimport on Windows; -fvisibility-global-new-delete doesn't. (This doesn't affect us, but it might affect others.)

3. -fvisibility-global-new-delete= doesn't override visibility on op new / delete, it only adds additional declarations. So if the declarations from that flag don't match the attribute from (now-libc++-internal) _LIBCPP_OVERRIDABLE_FUNC_VIS, you get a build error. See [libc++] Remove workaround which allows setting _LIBCPP_OVERRIDABLE_FUNC_VIS externally #113139 (comment) for a repro.

Since these all need changes to the -fvisibility-global-new-delete= flag, I suggest we revert #113139 for now.

(See also #113139 (comment) onwards.)

[philnik777]

Sorry that I haven't responded so far. I'm not against a revert, but I'd ask for the Clang folks (@AaronBallman) to comment on this as well, so they're aware of these problems and can give their thoughts. If there's a clear "yes, these are bugs and we should fix them", we can go ahead with reverting. Otherwise I'd like to see some thoughts on how to move forward with this, since _LIBCPP_OVERRIDABLE_FUNC_VIS isn't actually the appropriate tool for what you're describing, since it applies to more than just operator new/delete.

[AaronBallman]

Keep in mind that I am not an expert with the visibility attribute and its effects. With that caveat out of the way: (2) definitely seems like a bug to me. (3) surprises me also and feels like a bug. (1) doesn't feel like a bug with Clang but does seem like it's an issue (this one I'm less certain on).

CC @bd1976bris who did the original work on -fvisibility-global-new-delete= and @compnerd for opinions on visibility.

[compnerd]

I agree with @AaronBallman that bullet 2 feels like a "bug" (more so a limitation of the implementation that should be improved) and that (3) is surprising and feels like it should change the declaration not add new ones.

For (1) I think that @philnik777 is correct that _LIBCPP_OVERRIDABLE_FUNC_VIS is not the right tool - any overridable function needs to have default visibility as it must participate in dynamic linking to be overridden (otherwise, you have a local definition which should be preferred when linking and you are done). It sounds like we need to ensure that the nothrow variants of operator new and operator delete are also given the same visibility? The spec does indicate that they are replaceable, so we should ensure that the user is able to replace them, which does require that they are given default visibility.

CC: @ldionne

[ldionne]

I would suggest temporarily reverting #113139 while the Clang flag gets fixed. It looks like it works differently enough from what it should that we shouldn't depend on the flag. In fact, I'd also support removing the Clang flag until it does what its name suggests (unless we are confident that it can be fixed in a timely fashion). But that's not my call to make.

[AaronBallman]

I would not be opposed to rolling back the clang flag except for the fact that it was released in Clang 18 and we've already released Clang 19 and are on to Clang 20. Fixing forward would be less disruptive (users would otherwise get "unknown argument" fatal errors: https://godbolt.org/z/44EbzsrbE), but then again, there don't appear to be a ton of uses of this flag in the wild either once you filter out things we plausibly own: https://sourcegraph.com/search?q=context:global+-fvisibility-global-new-delete%3D+-file:libc%5C%2B%5C%2B+-file:libcxx+-file:libunwind+-file:.*clang.*&patternType=keyword&sm=0

[compnerd]

Removing the flag to restore it feels very disruptive. You now need additional version checks were someone to use it. Even in the case that we "remove" a driver level flag, we tend to emit a warning that it has no effect. Changing it such that the flag has no effect would be better than fully reverting the flag, even though it would still be disruptive.

[ldionne]

Just to clarify, I'm not asking for the flag to be removed :-). I'm just saying that the flag is currently broken and in a state of being nearly unusable. So if we don't have any plans to fix it in a timely fashion, we might just as well remove it to avoid shipping a broken feature. Fixing forward in a timely fashion would of course be the best course of action by far, though.

[compnerd]

Consider the following situation: we are unable to fix it in a timely fashion. It was introduced in Clang 18, shipped in 19. We remove it in 20 and are unable to restore it until Clang 23. Removing the flag gives you a [18,20) accepts the flag, [20, 23) disallows the flag, and [23, ...] accepts the flag. If instead, we simply warn that the flag is being ignored for [20, 23), at least we do not break the command line invocations while cleaning up the behaviour is what I was suggesting. Of course, if we can fix it within the release that is the preferred approach by far as you stated.

[ldionne]

If instead, we simply warn that the flag is being ignored for [20, 23), at least we do not break the command line invocations while cleaning up the behaviour is what I was suggesting.

Ack, that does seem like a better approach than just removing the flag (if we even need to go there). I had not understood your suggestion.

[AaronBallman]

If instead, we simply warn that the flag is being ignored for [20, 23), at least we do not break the command line invocations while cleaning up the behaviour is what I was suggesting.

Ack, that does seem like a better approach than just removing the flag (if we even need to go there). I had not understood your suggestion.

Agreed, that's in line with what I was thinking as well.