[PATCH] D125611: DWARFVerifier: Change vector of IntervalMap to vector of unique_ptrs

[Dimitry Andric via Phabricator via llvm-commits]

dim added a comment.

In D125611#3520595 <https://reviews.llvm.org/D125611#3520595>, @dblaikie wrote:

> In D125611#3519571 <https://reviews.llvm.org/D125611#3519571>, @dim wrote:
>
>> 

...

>> Ultimately, this is caused by `_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR` in `/usr/include/c++/v1/__config` (this was last modified by @EricWF, but a long time ago). If this setting is enabled (to preserve the old ABI), `std::pair` will always have a non-trivial copy constructor, and since `IntervalMap` contains a `std::pair` (via `RootLeaf` -> `IntervalMapImpl::LeafNode` -> `NodeBase<std::pair<KeyT, KeyT>, ValT, N>`), `IntervalMap` itself is also non-trivially copyable. `std::vector` in turn requires its value type to be copy-constructible (at least certainly when you use the constructor with a size).
>
> I was hoping something I could reproduce without having FreeBSD myself - either on godbolt (though godbolt might not have different OS platforms as a primary feature) or something like that. Or a brief example that emulates the quirk of FreeBSD to better understand what we're working around here.

The original problem was more accurately described by Howard Hinnant in rL194536 <https://reviews.llvm.org/rL194536> (rGccad8c32e088 <https://reviews.llvm.org/rGccad8c32e08857d0a3ac6a05a1d3c69e7dd25255>):

> Unfortunately (for libc++1) the Itanium ABI specifies different calling conventions for trivial and non-trivial copy constructors.  Therefore currently the C++03 libc++ copy constructor for pair<int, int> is ABI incompatible with the C++11 libc++ copy constructor for pair<int, int>. This is Bad(tm).

This is the original reason for changing the std::pair copy constructor to be trivial in all cases. In a follow-up commit rL194742 <https://reviews.llvm.org/rL194742> (rGf9fd0d6d113c <https://reviews.llvm.org/rGf9fd0d6d113ce53783ee78fc58476172df79d249>) Howard added a `_LIBCPP_TRIVIAL_PAIR_COPY_CTOR` define to be able to toggle this triviality on and off. Initially it was only turned off on Apple platforms, but later FreeBSD was added too.

Eventually the `_LIBCPP_TRIVIAL_PAIR_COPY_CTOR` define got renamed a few times, and then deprecated, but it is still carried around; I guess only libc++ ABI 2 will finally solve this for all downstream consumers. :-)

In any case, you can test this fairly easily on other platforms, assuming you can use libc++ headers, by compiling e.g. `DWARFVerifier.cpp` with `-D_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR` on the compiler command line. This should give you the same error as shown earlier.

>> I think this patch, using a unique_ptr sidesteps this issue by letting unique_ptr taking care of things. Another way would be to provide a hand-written copy constructor or move constructor for `IntervalMap`.
>
> That seems likely to be a better solution - if that's closer to the root cause, less chance someone else will introduce a similarly problematic use of IntervalMap that way (since the use will be no longer problematic).

I guess a copy constructor for `IntervalMap` is not too hard, indeed.

> Oh, OK, so piecing this together from the various discussions: FreeBSD opted to not pickup an ABI-breaking fix to libc++'s std::pair that would've made std::pair of two trivially copyable types itself trivially copyable. And so without that fix, IntervalMap is not trivially copyable/implicitly copyable because it has a union over a non-trivial type (`RootLeaf` -> `LeafNode` -> `NodeBase<pair<KeyT, KeyT>...` -> which has a member of that `pair<KeyT, KeyT>` type (well, an array of them))

Indeed, when `_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR` is defined (either manually, or in libc++'s `__config` header), all `std::pair` instances are non-trivially copyable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125611