[libc++] v2 ABI silently introduces new non-conforming behaviour for smart pointer destruction

[jrtc27]

_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI and _LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI are enabled for libc++'s v2 ABI and above (and unstable ABI). As documented in libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst (but that does not mention it's enabled by default for v2 and above), this is a non-conforming implementation that can lead to destructors being called in an order not permitted by the standard. Whilst I understand the appeal of the performance gain these options give, libc++ should not deliberately and silently be regressing standards conformance in the name of performance as part of opting into its v2 ABI. I would consider this a blocker for FreeBSD ever adopting that new ABI; we've had enough pain from adopting libc++ before it had a conforming std::pair implementation and having to maintain and keep providing that non-conformance for ABI compatibility despite it breaking real-world software. Maybe this one is harmless in practice, but any non-conformance has the potential to cause problems, and this would be a strict regression over v1.

These options should be split out from the ABI version, as an explicit opt-in for platforms to use should they view the performance gain worth the non-conformance. libc++'s v2 ABI should by default conform to the specification as fully as possible.

Cc @dim @emaste as it pertains to libc++ in FreeBSD

[jrtc27]

On top of that it requires a Clang language extension that GCC does not implement, and so would break FreeBSD's ability to allow users to use GCC over Clang if they really want to. This is something a significant fraction of FreeBSD's ports (packages) do (often for no good reason other than never having been properly made more portable) and so they would be broken by this.

[frederick-vs-ja]

It seems that too many things are collected into v2. Perhaps TRIVIAL_ABI should be carved out and enabled via an independent option.

[ldionne]

@jrtc27 Can you please describe clearly what is the non-conforming behavior along with an example? I am not doubting you, but it would be nice to have something concrete that we can look at.

FWIW, my current thinking is that we should not be offering ABI options that make us non-conforming in the first place, although "non-conforming" is a spectrum and that is why I'd like to have an example in front of me.

It seems that too many things are collected into v2. Perhaps TRIVIAL_ABI should be carved out and enabled via an independent option.

In fact, @philnik777 and I were recently having a discussion about the usefulness of ABI v2 at all. We believe that we should have ABI pre-sets that can be selected from, but we are not convinced that the notion of an ABI version specifically is useful, since there's basically no plan to ever "migrate" to that ABI and drop support for the "old" one.

[jrtc27]

@jrtc27 Can you please describe clearly what is the non-conforming behavior along with an example? I am not doubting you, but it would be nice to have something concrete that we can look at.

It's explicitly documented by the author of the patch to introduce the feature in UniquePtrTrivialAbi.rst as I referenced in the body of the issue. To quote:

Consequently, this may change the destruction order for function parameters to an order that is non-conforming to the standard. For example:

struct A { ~A(); };
struct B { ~B(); };
struct C { C(A, unique_ptr<B>, A) {} };
C c{{}, make_unique<B>, {}};

In a conforming implementation, the destruction order for C::C’s parameters is required to be ~A(), ~B(), ~A() but with this mode enabled, we’ll instead see ~B(), ~A(), ~A().

If you want a convenient link, it's part of libc++'s formal documentation and rendered at https://libcxx.llvm.org/DesignDocs/UniquePtrTrivialAbi.html.

FWIW, my current thinking is that we should not be offering ABI options that make us non-conforming in the first place, although "non-conforming" is a spectrum and that is why I'd like to have an example in front of me.

I think having such options is fine when there is a good reason for them existing, such as "it significantly improves performance and we're willing to manage the implications" (e.g. I can see it making sense for someone like Google to use for Fuchsia, which I believe is in fact what's happening today?). Just not enabled unless you explicitly and knowingly turn on an option that's signposted as being non-conforming.

It seems that too many things are collected into v2. Perhaps TRIVIAL_ABI should be carved out and enabled via an independent option.

In fact, @philnik777 and I were recently having a discussion about the usefulness of ABI v2 at all. We believe that we should have ABI pre-sets that can be selected from, but we are not convinced that the notion of an ABI version specifically is useful, since there's basically no plan to ever "migrate" to that ABI and drop support for the "old" one.

I guess as a downstream that one day will have to break ABI to deal with our historic std::pair ABI, we just want an option that is "enable the ABI that libc++ developers recommend is used to replace v1 and will have the same stability as v1 going forwards". Tying that up with the soname is convenient as it stops people screwing up by building two ABI-incompatible libc++.so.X for the same X, but I suppose there's no reason it has to be done. Were libc++'s ABIs not tied to the soname, we might have gone through an ABI transition a while back to adopt v1 with the fixed std::pair and call that libc++.so.2, but we never did that, in part at least because then we'd be diverging from upstream in having a libc++.so.2 that isn't ABI v2.

[philnik777]

I guess as a downstream that one day will have to break ABI to deal with our historic std::pair ABI, we just want an option that is "enable the ABI that libc++ developers recommend is used to replace v1 and will have the same stability as v1 going forwards". Tying that up with the soname is convenient as it stops people screwing up by building two ABI-incompatible libc++.so.X for the same X, but I suppose there's no reason it has to be done.

I think the real problem with that approach is that different vendors will want a different ABIv2. Depending on what you care about (as this very issue demonstrates) you want different ABI configurations, which a single number simply cannot express.

[ldionne]

It's explicitly documented by the author of the patch to introduce the feature in UniquePtrTrivialAbi.rst as I referenced in the body of the issue. To quote:

Consequently, this may change the destruction order for function parameters to an order that is non-conforming to the standard. For example:

struct A { ~A(); };
struct B { ~B(); };
struct C { C(A, unique_ptr<B>, A) {} };
C c{{}, make_unique<B>, {}};

In a conforming implementation, the destruction order for C::C’s parameters is required to be ~A(), ~B(), ~A() but with this mode enabled, we’ll instead see ~B(), ~A(), ~A().

If you want a convenient link, it's part of libc++'s formal documentation and rendered at https://libcxx.llvm.org/DesignDocs/UniquePtrTrivialAbi.html.

Thanks for the link. It actually seems that I reviewed the patch that added it 5 years ago, but it's difficult to keep track of everything happening in the project in one's head. Since 2020, we've gained experience with adding configuration options to libc++ and we've strengthened our stance on several topics as we formalized what libc++ should do and why. If the same patch were in front of me right now and someone explained why it was non-conforming, I'm pretty sure I'd push back immediately against this being added to the library. We don't really want to open the door to changes that make us non-conforming.

On the other hand, this non-conformism is slightly minor (although that can be argued against). I'd be curious to see if there's a way to tweak the standard and make this implementation of smart pointers conforming -- that seems like a pretty important missed optimization otherwise.

FWIW, my current thinking is that we should not be offering ABI options that make us non-conforming in the first place, although "non-conforming" is a spectrum and that is why I'd like to have an example in front of me.

I think having such options is fine when there is a good reason for them existing, such as "it significantly improves performance and we're willing to manage the implications" (e.g. I can see it making sense for someone like Google to use for Fuchsia, which I believe is in fact what's happening today?). Just not enabled unless you explicitly and knowingly turn on an option that's signposted as being non-conforming.

I'm not certain I agree with that. We usually don't intentionally do things that make us non-conforming, since that strips away a large part of the purpose of this library.

I guess as a downstream that one day will have to break ABI to deal with our historic std::pair ABI, we just want an option that is "enable the ABI that libc++ developers recommend is used to replace v1 and will have the same stability as v1 going forwards".

Those are almost exactly the same words we used to describe what we think libc++ should be providing going forward when I spoke about this with @philnik777 in our 1:1 a few days ago. I think we're probably aligned on what we think (OS) vendors need.

I think the real problem with that approach is that different vendors will want a different ABIv2. Depending on what you care about (as this very issue demonstrates) you want different ABI configurations, which a single number simply cannot express.

Right, but we could come up with an ABI pre-set that represents what we think is the best ABI to use for stability going forward at a given point in time. It might just not be named v2, and it wouldn't be the only pre-set we provide. For example in that stable pre-set, we'd enable all of the UB correcting breakages, the better string representation, etc. But we wouldn't enable other stuff that is more specific or more exploratory like the bounded iterators and these smart pointer trivial_abi.

We'd also have another preset for hardening where we enable the bounded iterators, etc.

[jyknight]

W.r.t. the standards conformance of marking types with [[clang::trivial_abi]], here's some comments I wrote on this subject a years ago, https://lists.llvm.org/pipermail/cfe-dev/2020-June/065845.html

IMO, the upshot of "is it standards-conforming" is...kinda. At first I thought it was fully conforming. But that was based on pre-C++17 understanding of the standard.

It absolutely conforms to the "spirit" of what the standard is saying, but I think is not 100% conforming to the text.

The thing we need is that we are "permitted to create temporary objects to hold the function parameter or result object", from https://eel.is/c++draft/class.temporary#3. As that says, "This latitude is granted to allow objects to be passed to or returned from functions in registers" -- precisely what we're doing. The only issue is that clause has a list of requirements for when you have such permission. And the wording "Or, the type has the [[clang::trivial_abi]] attribute on it" isn't there (of course).

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p2786r11.html is also highly relevant, since it adds the concept of trivial-relocatability to the standard. It does not extend a permission to use trivial relocatability for function calling, but is clearly envisioned as future work, https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p2786r11.html#can-the-compiler-transform-argument-passing-with-trivial-relocation

[ldionne]

Thanks a lot for the writeup @jyknight, this is extremely useful.

@cor3ntin It seems like the spec might want to add trivially relocatable types to this list of exceptions?
Also, does Clang's __builtin_is_trivially_relocatable(T) return true for a T that is marked as [[trivial_abi]]? If that were the case and we had the addition in the exception list in the spec, then this behaviour would be undoubtedly conforming (insofar as [[trivial_abi]] is a non-standard attribute).

@jrtc27 I think this is an interesting thread to pull on, and definitely makes me want to say that this behavior is not really non-conforming. The intent of the Standard is pretty clear on the right for compilers to pass objects in registers and therefore elide some operations -- it seems like there is a loop to close on how [[trivial_abi]] ties into this, but it's more of a specification challenge than an actual design question. Do you have thoughts based on the latest information provided by @jyknight ?

[philnik777]

@jrtc27 Any more thoughts? If not, let's close this, since libc++ can't fix the standard wording.

[jrtc27]

From my perspective there are two possible (satisfactory) outcomes:

1. It is determined, perhaps via a DR if needed, that this behaviour is in fact a conforming implementation (or was intended to be one but the specification failed to permit it). Then nothing needs to change in libc++.
2. It is determined that this behaviour is not conforming, as currently documented. Then this option should be removed from the unstable/v2/whatever ABI option and be given a separate configuration flag that labels it non-conforming. Or remove it entirely, but that's likely rather undesirable for those using it in production.

I don't see a definitive move towards either; it sounds like people are conflicted about which is right. I should also note, whilst I know a lot of the corners of C, I am far from the same when it comes to C++, so I can only provide limited opinions on interpretation of the standard.