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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> In D125611#3517574 <https://reviews.llvm.org/D125611#3517574>, @dblaikie wrote:
>
>> Some more details on the specifics in the patch description would be good - I'm still not 100% clear on the reproduction case to understand what we're working around, exactly. Do you have a godbolt reproduction/small example, by chance?
>
> Assuming there's a directory with llvm includes, this is enough to reproduce:
>
>   #include "llvm/ADT/IntervalMap.h"
>   #include <vector>
>   
>   void f(std::size_t size)
>   {
>     using namespace llvm;
>   
>     IntervalMap<uint32_t, uint64_t>::Allocator Alloc;
>     std::vector<IntervalMap<uint32_t, uint64_t>> Sections(size, IntervalMap<uint32_t, uint64_t>(Alloc));
>   }
>
> which will (on FreeBSD at least, where libc++ is the standard C++ library) result in something like:
>
>   $ clang++ -I llvm/include -c intervalmap.cpp
>   In file included from intervalmap.cpp:1:
>   In file included from llvm/include/llvm/ADT/IntervalMap.h:108:
>   In file included from llvm/include/llvm/ADT/SmallVector.h:19:
>   In file included from /usr/include/c++/v1/algorithm:643:
>   /usr/include/c++/v1/memory:1809:31: error: call to implicitly-deleted copy constructor of 'llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>'
>               ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
>                                 ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   /usr/include/c++/v1/memory:1687:21: note: in instantiation of function template specialization 'std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, const llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>> &>' requested here
>                   __a.construct(__p, _VSTD::forward<_Args>(__args)...);
>                       ^
>   /usr/include/c++/v1/memory:1538:14: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::__construct<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, const llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>> &>' requested here
>               {__construct(__has_construct<allocator_type, _Tp*, _Args...>(),
>                ^
>   /usr/include/c++/v1/vector:1066:25: note: in instantiation of function template specialization 'std::__1::allocator_traits<std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, const llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>> &>' requested here
>           __alloc_traits::construct(this->__alloc(), _VSTD::__to_address(__pos), __x);
>                           ^
>   /usr/include/c++/v1/vector:1159:9: note: in instantiation of member function 'std::__1::vector<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::__construct_at_end' requested here
>           __construct_at_end(__n, __x);
>           ^
>   intervalmap.cpp:9:48: note: in instantiation of member function 'std::__1::vector<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>, std::__1::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>>>::vector' requested here
>     std::vector<IntervalMap<uint32_t, uint64_t>> Sections(size, IntervalMap<uint32_t, uint64_t>(Alloc));
>                                                  ^
>   llvm/include/llvm/ADT/IntervalMap.h:970:14: note: copy constructor of 'IntervalMap<unsigned int, unsigned long, 12, llvm::IntervalMapInfo<unsigned int>>' is implicitly deleted because variant field 'leaf' has a non-trivial copy constructor
>       RootLeaf leaf;
>                ^
>   1 error generated.
>
> 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.

> 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).

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))

So, yeah, I guess adding a copy and/or move constructor to IntervalMap would be more suitable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125611