[libcxx-commits] [PATCH] D127444: [libc++] Use ABI tags instead of internal linkage to provide per-TU insulation

Tue Jun 14 15:26:57 PDT 2022

ldionne added a comment.

(consolidating here in a single thread)

In D127444#3579773 <https://reviews.llvm.org/D127444#3579773>, @EricWF wrote:

> You're right. The linker will choose the symbol in an SO if it absolutely must. But that's only if we don't mark it with `__attribute__((visibility("hidden"))`, which we should be using, since it's both necessary and sufficient to solve the export problem.

We agree here. We do mark implementation-detail symbols with `visibility("hidden")` for that purpose (via `_LIBCPP_HIDDEN`).

> Which limits the utility of this to cases where
>
> 1. a user **statically links** object files produced against two different versions of libc++, AND:
>
>   A. a non-template inline function `F` changes its return type, without changing anything else, so that the mangling stays the same, but the ABI changes; Or,
>
>   B. a inline function `G` changes it's pre-conditions/post-conditions without changing the signature, so that input that's valid to `G_0` is not valid to `G_1` or vice-versa.

We're in agreement here. Solving this problem has been the goal of this whole endeavour with `internal_linkage` and `always_inline` from the beginning. Initially, libc++ marked everything with `always_inline` so as to "avoid" this problem entirely (if you don't emit symbols, you can't have ODR issues!). This was true until I made changes to start using `internal_linkage` circa 2018. The goal of that change was to stop inlining everything, which was bad for code size and debugging.

> Arguably, the user shouldn't be doing (1), and we shouldn't make changes of the kind required for (A) or (B) (that don't also change mangling).
> If a user does to (1), but we're careful to not do (A) or (B), then we have a no ODR violation at all -- because either:
>
> 1. all the available definitions of a function `F` are the same, or
> 2. we have a "benign" ODR violation where there are multiple different definitions of `F`, but selecting either definition is OK because the pre/post conditions haven't changed.

We're in partial agreement. The ability for users to mix statically linked versions of libc++ is something that we used to support, until I changed it to be conditionally supported by vendors (based on whether `LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT` is used when configuring the library). We still support it on Apple platforms, and AFAICT some users might rely on it.

Where we disagree is when it comes to what is reasonable from libc++ to avoid these ODR violations or make them benign. We currently don't pay attention to how we change implementation-detail functions, and I don't think it is realistic to do it robustly. Think about it -- it means that we would need to know the full set of function signatures (name + parameter list) to have ever been used in a shipping libc++, and we should either never reuse any of these signatures, or if we do, then make sure we have compatible pre/post conditions for that function. For example, can we guarantee that we haven't previously shipped a function named `__copy_impl(char*, char*, char*)` with pre/post conditions incompatible with the ones we have in our code today <https://github.com/llvm/llvm-project/blob/1a19abf38c3afb66b680cea69c8e61ea152e1514/libcxx/include/__algorithm/copy.h#L31>? Even back in e.g. LLVM 7?

As you mention, the only way to do it robustly is to never change any function but in minor ways that would make an ODR mismatch effectively a benign ODR violation. However, that places a significant restriction on how we can evolve libc++'s code, and it also increases the barrier to contributing for reasons intangible to anyone not having read this discussion. Since we have a clear technical solution instead, I think we should go for it.

In D127444#3579745 <https://reviews.llvm.org/D127444#3579745>, @EricWF wrote:

> OK, so i'm convinced this change should be OK assuming we continue to ensure `_LIBCPP_HIDE_FROM_ABI` is only ever applied to inline functions.

Agreed, that's a requirement, and it was also a requirement prior to this change -- we never use `_LIBCPP_HIDE_FROM_ABI` on non-functions.

> It would also need to be applied to every single inline function the library produces, otherwise a tagged function could call an untagged function,
> since the untagged definition can be selected at random from the available definitions.

Agreed, and we already do that except:

1. For a few functions that have been forgotten (we sometimes catch missing `_LIBCPP_HIDE_FROM_ABI` and we add them), and
2. for a few functions that were large enough that we didn't want to mark with `internal_linkage` or `always_inline`, and for which we do go through the trouble of making sure we don't introduce ODR violations. That being said, with this proposed patch, we could mark those functions as well without problem.

> The other consideration is the cost of this change. While small, it does increase the mangling of every single inline function in libc++ by ~5 bytes.
> After a little testing, this appears to translate into an object file size increase of ~1%  when debug information is enabled.
>
> My current opinion is that the binary size increase outweighs the limited benefits the change provides.
> But I'm open to being convinced.

I actually think that's the crux of this change.

1. For configurations where we use `LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT=ON`, the benefit will be that we stop having duplicate instantiations of things like pop_heap <https://github.com/llvm/llvm-project/blob/8224fb7ef9aa6b8b42621578a967fca6fc517310/libcxx/include/__algorithm/pop_heap.h#L29> in each translation unit where it is used. This is just a random one, but all functions with `_LIBCPP_HIDE_FROM_ABI` will get that benefit. For this configuration, this is a net win, there is no cost I can think of.
2. For configurations of the library where `LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT=OFF` (e.g. the LLVM release), this will provide the ability for users to statically link against mixed versions of libc++ safely (which some users probably do without even knowing it, but is currently unsafe). The cost will be the ABI tag, which is roughly 5 bytes per symbol, as you mention.

For (2), the cost should only affect the size of object files, since these symbols should be stripped from shipping executables. Given that, my inclination would be to raise correctness above object size, and to try to push efforts such as ABI compression mangling <https://github.com/itanium-cxx-abi/cxx-abi/issues/70> instead, which could deliver absolutely insane improvements to symbol sizes, much beyond saving 5 bytes.

> @ldionne maybe you could produce a compiling example of a situation this change intends to fix?

Like I said, this is not really to fix any single specific situation -- it's to (1) reduce per-TU duplication when per-TU insulation is provided, (2) provide an ABI safety guarantee to users in all cases, and (3) simplify the library.

Another thing I can do is keep the `LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT` switch and not provide the ABI safety guarantee for vendors who don't care about it (that way we don't have the ABI tag when we previously wouldn't have had `internal_linkage`). However, this keeps existing complexity around, and to quote someone I respect a lot: "most users don't know what their ABI needs are, and we need to pick what's right for them". IMO this is such a case.

In D127444#3579773 <https://reviews.llvm.org/D127444#3579773>, @EricWF wrote:

> In D127444#3579756 <https://reviews.llvm.org/D127444#3579756>, @philnik wrote:
>
>> Just out of interest: Does this remove the problem that we `always_inline` a lot of functions with GCC?
>
> At this point I hope we're only `always_inlining` functions for performance/code size reasons, rather than
> as ABI management. Though it may still be needed to battle with extern template semantics.
>
> Specifically which `always inline` usages are you talking about?

It does not entirely remove the need for `always_inline`, because GCC doesn't support `exclude_from_explicit_instantiation`. However, it does take us one step closer to not needing `always_inline` for ABI management. In particular, I plan to do a follow-up patch that would create an inline namespace that gets rev'd at each release for implementation-detail functions, and that would remove the need to use `_LIBCPP_HIDE_FROM_ABI` on declarations in that namespace (and that would allow ditching `always_inline` for many functions on GCC).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127444