Message ID | alpine.DEB.2.02.1809011549480.12789@stedding.saclay.inria.fr |
---|---|
State | New |
Headers | show |
Series | Relocation (= move+destroy) | expand |
I forgot to attach the "diff -w" version of the patch, which may be a bit more readable.
On Sat, 1 Sep 2018, Marc Glisse wrote:
> this patch passed bootstrap+regtest on powerpc64le-unknown-linux-gnu.
I realized afterwards that for a C++17-only feature, that's not testing
much... So I changed it to apply in C++14 and fixed a minor issue. There
is now a single regression:
23_containers/vector/modifiers/push_back/49836.cc
The PR was about not using assignment for an operation that should only
use construction, and that's fine. But we ended up with a stricter
testcase using CopyConsOnlyType, where the type has a deleted move
constructor which, as far as I understand the standard, makes it an
invalid type for use in vector::push_back. Is that something we want to
keep supporting, or may I break it? What is happening is that the
definition of __use_relocate asks if some expression involving a move of
_Tp is noexcept, which causes a hard error. It would certainly be possible
to work around that, but it would complicate the code and seems quite
pointless to me.
On 01/09/18 21:56 +0200, Marc Glisse wrote: >On Sat, 1 Sep 2018, Marc Glisse wrote: > >>this patch passed bootstrap+regtest on powerpc64le-unknown-linux-gnu. > >I realized afterwards that for a C++17-only feature, that's not >testing much... So I changed it to apply in C++14 and fixed a minor >issue. There is now a single regression: > >23_containers/vector/modifiers/push_back/49836.cc > >The PR was about not using assignment for an operation that should >only use construction, and that's fine. But we ended up with a >stricter testcase using CopyConsOnlyType, where the type has a deleted >move constructor which, as far as I understand the standard, makes it >an invalid type for use in vector::push_back. Is that something we >want to keep supporting, or may I break it? What is happening is that I think you can break it. I'll look back over the history of the test case, but I don't think supporting deleted moves is intended. >the definition of __use_relocate asks if some expression involving a >move of _Tp is noexcept, which causes a hard error. It would certainly >be possible to work around that, but it would complicate the code and >seems quite pointless to me. Agreed.
On 2 September 2018 at 23:03, Jonathan Wakely <jwakely@redhat.com> wrote: > On 01/09/18 21:56 +0200, Marc Glisse wrote: >> >> On Sat, 1 Sep 2018, Marc Glisse wrote: >> >>> this patch passed bootstrap+regtest on powerpc64le-unknown-linux-gnu. >> >> >> I realized afterwards that for a C++17-only feature, that's not testing >> much... So I changed it to apply in C++14 and fixed a minor issue. There is >> now a single regression: >> >> 23_containers/vector/modifiers/push_back/49836.cc >> >> The PR was about not using assignment for an operation that should only >> use construction, and that's fine. But we ended up with a stricter testcase >> using CopyConsOnlyType, where the type has a deleted move constructor which, >> as far as I understand the standard, makes it an invalid type for use in >> vector::push_back. Is that something we want to keep supporting, or may I >> break it? What is happening is that > > > I think you can break it. I'll look back over the history of the test > case, but I don't think supporting deleted moves is intended. We have supported those occasionally; I did so in std::any, but the standard has explicitly been moving towards a direction where deleted moves (if corresponding copies are not deleted) are not a supported thing, so I concur with the suggestion of breaking such tests being okay.
On 01/09/18 17:00 +0200, Marc Glisse wrote: >_GLIBCXX_ASAN_ANNOTATE_REINIT: I am not familiar with those >annotations. It was convenient in one function to move this annotation >after _Destroy, to reduce code duplication. For consistency, I did the >same in the whole file. As far as I understand, the macro makes it ok >to access memory between _M_finish and _M_end_of_storage, and at the >end of the block marks again the region after the new _M_finish as >protected. Since _Destroy should stop at _M_finish, moving the macro >looks safe. But maybe the position of the macro was chosen to reduce >needless checking in ASAN? I don't remember if I had a specific reason for doing the REINIT before _Destroy, but your change looks fine. I'll keep reviewing the rest, but my first impression is that this is a nice optimisation, thanks for working on it.
On Sun, 2 Sep 2018, Jonathan Wakely wrote: > On 01/09/18 21:56 +0200, Marc Glisse wrote: >> On Sat, 1 Sep 2018, Marc Glisse wrote: >> >>> this patch passed bootstrap+regtest on powerpc64le-unknown-linux-gnu. >> >> I realized afterwards that for a C++17-only feature, that's not testing >> much... So I changed it to apply in C++14 and fixed a minor issue. There is >> now a single regression: >> >> 23_containers/vector/modifiers/push_back/49836.cc >> >> The PR was about not using assignment for an operation that should only use >> construction, and that's fine. But we ended up with a stricter testcase >> using CopyConsOnlyType, where the type has a deleted move constructor >> which, as far as I understand the standard, makes it an invalid type for >> use in vector::push_back. Is that something we want to keep supporting, or >> may I break it? What is happening is that > > I think you can break it. I'll look back over the history of the test > case, but I don't think supporting deleted moves is intended. Here is a version where I adapt the test. Bootstrap+testsuite on gcc112. 2018-10-15 Marc Glisse <marc.glisse@inria.fr> PR libstdc++/87106 * include/bits/alloc_traits.h (_S_construct, _S_destroy, construct, destroy): Add noexcept specification. * include/bits/allocator.h (construct, destroy): Likewise. * include/ext/alloc_traits.h (construct, destroy): Likewise. * include/ext/malloc_allocator.h (construct, destroy): Likewise. * include/ext/new_allocator.h (construct, destroy): Likewise. * include/bits/stl_uninitialized.h (__relocate, __relocate_a, __relocate_a_1): New functions. (__is_trivially_relocatable): New class. * include/bits/stl_vector.h (__use_relocate): New static member. * include/bits/vector.tcc (reserve, _M_realloc_insert, _M_default_append): Use __relocate_a. (reserve, _M_assign_aux, _M_realloc_insert, _M_fill_insert, _M_default_append, _M_range_insert): Move _GLIBCXX_ASAN_ANNOTATE_REINIT after _Destroy. * testsuite/23_containers/vector/modifiers/push_back/49836.cc: Replace CopyConsOnlyType with DelAnyAssign.
On 13/10/18 11:07 +0200, Marc Glisse wrote: >--- libstdc++-v3/include/bits/alloc_traits.h (revision 265131) >+++ libstdc++-v3/include/bits/alloc_traits.h (working copy) >@@ -233,38 +233,43 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > using type = decltype(__test<_Alloc>(0)); > }; > > template<typename _Tp, typename... _Args> > using __has_construct > = typename __construct_helper<_Tp, _Args...>::type; > > template<typename _Tp, typename... _Args> > static _Require<__has_construct<_Tp, _Args...>> > _S_construct(_Alloc& __a, _Tp* __p, _Args&&... __args) >+ noexcept(noexcept(__a.construct(__p, std::forward<_Args>(__args)...))) You could use std::declval<_Args>()... instead of forwarding __args... which would be slightly shorter (and might keep some of the others on a single line). No need to change it unless you want to, either is OK. I'll finish reviewing the rest on Monday.
On Sat, 13 Oct 2018, Jonathan Wakely wrote: > On 13/10/18 11:07 +0200, Marc Glisse wrote: >> --- libstdc++-v3/include/bits/alloc_traits.h (revision 265131) >> +++ libstdc++-v3/include/bits/alloc_traits.h (working copy) >> @@ -233,38 +233,43 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> using type = decltype(__test<_Alloc>(0)); >> }; >> >> template<typename _Tp, typename... _Args> >> using __has_construct >> = typename __construct_helper<_Tp, _Args...>::type; >> >> template<typename _Tp, typename... _Args> >> static _Require<__has_construct<_Tp, _Args...>> >> _S_construct(_Alloc& __a, _Tp* __p, _Args&&... __args) >> + noexcept(noexcept(__a.construct(__p, >> std::forward<_Args>(__args)...))) > > You could use std::declval<_Args>()... instead of forwarding __args... > which would be slightly shorter (and might keep some of the others on > a single line). No need to change it unless you want to, either is OK. In order to reduce the risk of bugs, my preference is 1) noexcept(auto): I am amazed that no compiler has implemented it yet, even as a private extension spelled __noexcept_auto or whatever, since it reduces significantly the burden on standard library developers. As a recent example, PR 87538 would never have had a chance to exist. 2) an exact copy-paste of the body of the function. 3) something else only if needed, for instance because 2) misses the conversion to the result type. Of course, since there is more chance *you* will have to maintain that mess, whatever style you find easier to maintain is more important.
On Sat, 13 Oct 2018, Marc Glisse wrote: > + template<typename _Tp> > + struct __is_trivially_relocatable > + : is_trivially_move_constructible<_Tp> { }; Oups, this part is wrong, sorry, it is supposed to be "is_trivial" instead of "is_trivially_move_constructible", to match what is done elsewhere in this file. Using is_trivially_move_constructible instead (like libc++) is a separate issue, and in the particular case of relocation would need to be combined with is_trivially_destructible. I'll retest with is_trivial, but I would be very surprised if that broke anything in the testsuite.
CCing gcc-patches On 19/10/18 07:33 +0200, Marc Glisse wrote: >On Thu, 18 Oct 2018, Marc Glisse wrote: > >>Uh, why didn't I notice that the function __relocate is unused? I >>guess I'll resend the same patch without __relocate once retesting >>has finished :-( Sorry for all the corrections, I guess I didn't >>check my patch carefully enough before sending it the first time. > >2018-10-19 Marc Glisse <marc.glisse@inria.fr> > > PR libstdc++/87106 > * include/bits/alloc_traits.h (_S_construct, _S_destroy, construct, > destroy): Add noexcept specification. > * include/bits/allocator.h (construct, destroy): Likewise. > * include/ext/alloc_traits.h (construct, destroy): Likewise. > * include/ext/malloc_allocator.h (construct, destroy): Likewise. > * include/ext/new_allocator.h (construct, destroy): Likewise. > * include/bits/stl_uninitialized.h (__relocate_a, __relocate_a_1): > New functions. > (__is_trivially_relocatable): New class. > * include/bits/stl_vector.h (__use_relocate): New static member. > * include/bits/vector.tcc (reserve, _M_realloc_insert, > _M_default_append): Use __relocate_a. > (reserve, _M_assign_aux, _M_realloc_insert, _M_fill_insert, > _M_default_append, _M_range_insert): Move _GLIBCXX_ASAN_ANNOTATE_REINIT > after _Destroy. > * testsuite/23_containers/vector/modifiers/push_back/49836.cc: > Replace CopyConsOnlyType with DelAnyAssign. > >-- >Marc Glisse The tricky stuff in <bits/stl_vector.h> all looks right, I only have some comments on the __relocate_a functions ... >Index: libstdc++-v3/include/bits/stl_uninitialized.h >=================================================================== >--- libstdc++-v3/include/bits/stl_uninitialized.h (revision 265289) >+++ libstdc++-v3/include/bits/stl_uninitialized.h (working copy) >@@ -872,14 +872,75 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > uninitialized_move_n(_InputIterator __first, _Size __count, > _ForwardIterator __result) > { > auto __res = std::__uninitialized_copy_n_pair > (_GLIBCXX_MAKE_MOVE_ITERATOR(__first), > __count, __result); > return {__res.first.base(), __res.second}; > } > #endif > >+#if __cplusplus >= 201402L What depends on C++14 here? Just enable_if_t? Because we have __enable_if_t for use in C++11. Both GCC and Clang will allow constexpr-if and static_assert with no message in C++11. >+ template<typename _Tp, typename _Up, typename _Allocator> >+ inline void >+ __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc) I find it a little surprising that this overload for single objects using the memmove argument ordering (dest, source) but the range overload below uses the STL ordering (source_begin, source_end, dest). But I wouldn't be surprised if we're already doing that somewhere that I've forgotten about. WOuld it make sense to either rename this overload, or to use consistent argument ordering for the two __relocate_a overloads? >+ noexcept(noexcept(__gnu_cxx::__alloc_traits<_Allocator>::construct(__alloc, Since this is C++14 (or maybe C++11) you could just use std::allocator_traits directly. __gnu_cxx::__alloc_traits is to provide equivalent functionality in C++98 code. >+ __dest, std::move(*__orig))) >+ && noexcept(__gnu_cxx::__alloc_traits<_Allocator>::destroy( >+ __alloc, std::__addressof(*__orig)))) >+ { >+ typedef __gnu_cxx::__alloc_traits<_Allocator> __traits; >+ __traits::construct(__alloc, __dest, std::move(*__orig)); >+ __traits::destroy(__alloc, std::__addressof(*__orig)); >+ } >+ >+ template<typename _Tp> >+ struct __is_trivially_relocatable >+ : is_trivial<_Tp> { }; It might be worth adding a comment that this type might be specialized in future, so that I don't forget and simplify it to an alias template later :-) >+ template <typename _Tp, typename _Up> >+ inline std::enable_if_t<std::__is_trivially_relocatable<_Tp>::value, _Tp*> >+ __relocate_a_1(_Tp* __first, _Tp* __last, >+ _Tp* __result, allocator<_Up>& __alloc) >+ { >+ ptrdiff_t __count = __last - __first; >+ __builtin_memmove(__result, __first, __count * sizeof(_Tp)); >+ return __result + __count; >+ } >+ >+ template <typename _InputIterator, typename _ForwardIterator, >+ typename _Allocator> >+ inline _ForwardIterator >+ __relocate_a_1(_InputIterator __first, _InputIterator __last, >+ _ForwardIterator __result, _Allocator& __alloc) >+ { >+ typedef typename iterator_traits<_InputIterator>::value_type >+ _ValueType; >+ typedef typename iterator_traits<_ForwardIterator>::value_type >+ _ValueType2; >+ static_assert(std::is_same<_ValueType, _ValueType2>::value); >+ static_assert(noexcept(std::__relocate_a(std::addressof(*__result), >+ std::addressof(*__first), >+ __alloc))); >+ _ForwardIterator __cur = __result; >+ for (; __first != __last; ++__first, (void)++__cur) >+ std::__relocate_a(std::__addressof(*__cur), >+ std::__addressof(*__first), __alloc); >+ return __cur; >+ } >+ >+ template <typename _InputIterator, typename _ForwardIterator, >+ typename _Allocator> >+ inline _ForwardIterator >+ __relocate_a(_InputIterator __first, _InputIterator __last, >+ _ForwardIterator __result, _Allocator& __alloc) >+ { >+ return __relocate_a_1(std::__niter_base(__first), >+ std::__niter_base(__last), >+ std::__niter_base(__result), __alloc); >+ } >+#endif >+ > _GLIBCXX_END_NAMESPACE_VERSION > } // namespace
On Tue, 23 Oct 2018, Jonathan Wakely wrote: > CCing gcc-patches It seems to have disappeared somehow during the discussion, sorry. > The tricky stuff in <bits/stl_vector.h> all looks right, I only have > some comments on the __relocate_a functions ... > > >> Index: libstdc++-v3/include/bits/stl_uninitialized.h >> =================================================================== >> --- libstdc++-v3/include/bits/stl_uninitialized.h (revision 265289) >> +++ libstdc++-v3/include/bits/stl_uninitialized.h (working copy) >> @@ -872,14 +872,75 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> uninitialized_move_n(_InputIterator __first, _Size __count, >> _ForwardIterator __result) >> { >> auto __res = std::__uninitialized_copy_n_pair >> (_GLIBCXX_MAKE_MOVE_ITERATOR(__first), >> __count, __result); >> return {__res.first.base(), __res.second}; >> } >> #endif >> >> +#if __cplusplus >= 201402L > > What depends on C++14 here? Just enable_if_t? Because we have > __enable_if_t for use in C++11. > > Both GCC and Clang will allow constexpr-if and static_assert with no > message in C++11. Probably it can be enabled in C++11 if you think that matters. I'll admit that I personally don't care at all about C++11, and the main motivation would be to enable a cleanup if we stop supporting C++03 (I am not very optimistic). >> + template<typename _Tp, typename _Up, typename _Allocator> >> + inline void >> + __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc) > > I find it a little surprising that this overload for single objects > using the memmove argument ordering (dest, source) but the range overload > below uses the STL ordering (source_begin, source_end, dest). > > But I wouldn't be surprised if we're already doing that somewhere that > I've forgotten about. > > WOuld it make sense to either rename this overload, or to use > consistent argument ordering for the two __relocate_a overloads? The functions were not meant as overloads, it just happened that I arrived at the same name for both, but it would make perfect sense to give them different names. I started from __relocate(dest, source) for one element, and later added an allocator to it. The other one corresponds to __uninitialized_move_a, and naming it __uninitialized_relocate_a would be silly since "uninitialized" is included in the definition of relocate. I think I'd rather rename than change the order. Do you have suggestions? __relocate_range_a? >> + >> noexcept(noexcept(__gnu_cxx::__alloc_traits<_Allocator>::construct(__alloc, > > Since this is C++14 (or maybe C++11) you could just use > std::allocator_traits directly. __gnu_cxx::__alloc_traits is to > provide equivalent functionality in C++98 code. Thanks, I was wondering what it was for. >> + __dest, std::move(*__orig))) >> + && noexcept(__gnu_cxx::__alloc_traits<_Allocator>::destroy( >> + __alloc, std::__addressof(*__orig)))) >> + { >> + typedef __gnu_cxx::__alloc_traits<_Allocator> __traits; >> + __traits::construct(__alloc, __dest, std::move(*__orig)); >> + __traits::destroy(__alloc, std::__addressof(*__orig)); >> + } >> + >> + template<typename _Tp> >> + struct __is_trivially_relocatable >> + : is_trivial<_Tp> { }; > > It might be worth adding a comment that this type might be specialized > in future, so that I don't forget and simplify it to an alias template > later :-) Ok.
On 23/10/18 23:17 +0200, Marc Glisse wrote: >On Tue, 23 Oct 2018, Jonathan Wakely wrote: >>What depends on C++14 here? Just enable_if_t? Because we have >>__enable_if_t for use in C++11. >> >>Both GCC and Clang will allow constexpr-if and static_assert with no >>message in C++11. > >Probably it can be enabled in C++11 if you think that matters. I'll >admit that I personally don't care at all about C++11, and the main >motivation would be to enable a cleanup if we stop supporting C++03 (I >am not very optimistic). Me neither. But even if enabling cleanup some day is unlikely, I think that for this code the only change you'd need is to replace enable_if_t with __enable_if_t and we get C++11 support for free, so we might as well do it. >>>+ template<typename _Tp, typename _Up, typename _Allocator> >>>+ inline void >>>+ __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc) >> >>I find it a little surprising that this overload for single objects >>using the memmove argument ordering (dest, source) but the range overload >>below uses the STL ordering (source_begin, source_end, dest). >> >>But I wouldn't be surprised if we're already doing that somewhere that >>I've forgotten about. >> >>WOuld it make sense to either rename this overload, or to use >>consistent argument ordering for the two __relocate_a overloads? > >The functions were not meant as overloads, it just happened that I >arrived at the same name for both, but it would make perfect sense to >give them different names. I started from __relocate(dest, source) for >one element, and later added an allocator to it. The other one >corresponds to __uninitialized_move_a, and naming it >__uninitialized_relocate_a would be silly since "uninitialized" is >included in the definition of relocate. Yes, my first thought was to add "uninitialized" and I rejected it for that same reason. >I think I'd rather rename than change the order. Do you have >suggestions? __relocate_range_a? I was thinking the single object one could be __relocate_1_a with the _1 being like copy_n, fill_n etc. but that's going to be confusing with __relocate_a_1 instead! It seems unfortunate to have to put "range" in the name when no other algos that work on ranges bother to say that in the name. Maybe the single object one could be __relocate_single_a? Or __do_relocate_a? __relocate_obj_a? None of them really makes me happy. >>>+ noexcept(noexcept(__gnu_cxx::__alloc_traits<_Allocator>::construct(__alloc, >> >>Since this is C++14 (or maybe C++11) you could just use >>std::allocator_traits directly. __gnu_cxx::__alloc_traits is to >>provide equivalent functionality in C++98 code. > >Thanks, I was wondering what it was for. It's just so I could write code in containers that works the same for C++11 allocators and C++98 allocators, always using __alloc_traits. In C++98 it just forwards everything straight to the allocator, because C++98 allocators are required to define all the members themselves.
On Tue, 23 Oct 2018, Jonathan Wakely wrote: >>>> + template<typename _Tp, typename _Up, typename _Allocator> >>>> + inline void >>>> + __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc) >>> >>> I find it a little surprising that this overload for single objects >>> using the memmove argument ordering (dest, source) but the range overload >>> below uses the STL ordering (source_begin, source_end, dest). >>> >>> But I wouldn't be surprised if we're already doing that somewhere that >>> I've forgotten about. >>> >>> WOuld it make sense to either rename this overload, or to use >>> consistent argument ordering for the two __relocate_a overloads? >> >> The functions were not meant as overloads, it just happened that I arrived >> at the same name for both, but it would make perfect sense to give them >> different names. I started from __relocate(dest, source) for one element, >> and later added an allocator to it. The other one corresponds to >> __uninitialized_move_a, and naming it __uninitialized_relocate_a would be >> silly since "uninitialized" is included in the definition of relocate. > > Yes, my first thought was to add "uninitialized" and I rejected it for > that same reason. > >> I think I'd rather rename than change the order. Do you have suggestions? >> __relocate_range_a? > > I was thinking the single object one could be __relocate_1_a with the > _1 being like copy_n, fill_n etc. but that's going to be confusing > with __relocate_a_1 instead! > > It seems unfortunate to have to put "range" in the name when no other > algos that work on ranges bother to say that in the name. > > Maybe the single object one could be __relocate_single_a? Or > __do_relocate_a? __relocate_obj_a? None of them really makes me happy. I went with __relocate_object_a, but that's easy to change. We may want to specialize / overload __relocate_object_a for some specific types in the future, although we would more likely have __relocate_object_a call __relocate_object (no allocator) when it receives the default allocator, and specialize that one. And this should anyway be less common that specializing __is_trivially_relocatable. I just realized that __reallocate_a and construct don't take the allocator argument in the same position... I can switch if it helps. Regtested on gcc112. 2018-10-25 Marc Glisse <marc.glisse@inria.fr> PR libstdc++/87106 * include/bits/alloc_traits.h (_S_construct, _S_destroy, construct, destroy): Add noexcept specification. * include/bits/allocator.h (construct, destroy): Likewise. * include/ext/alloc_traits.h (construct, destroy): Likewise. * include/ext/malloc_allocator.h (construct, destroy): Likewise. * include/ext/new_allocator.h (construct, destroy): Likewise. * include/bits/stl_uninitialized.h (__relocate_object_a, __relocate_a, __relocate_a_1): New functions. (__is_trivially_relocatable): New class. * include/bits/stl_vector.h (__use_relocate): New static member. * include/bits/vector.tcc (reserve, _M_realloc_insert, _M_default_append): Use __relocate_a. (reserve, _M_assign_aux, _M_realloc_insert, _M_fill_insert, _M_default_append, _M_range_insert): Move _GLIBCXX_ASAN_ANNOTATE_REINIT after _Destroy. * testsuite/23_containers/vector/modifiers/push_back/49836.cc: Replace CopyConsOnlyType with DelAnyAssign.
On 25/10/18 14:30 +0200, Marc Glisse wrote: >On Tue, 23 Oct 2018, Jonathan Wakely wrote: > >>>>>+ template<typename _Tp, typename _Up, typename _Allocator> >>>>>+ inline void >>>>>+ __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc) >>>> >>>>I find it a little surprising that this overload for single objects >>>>using the memmove argument ordering (dest, source) but the range overload >>>>below uses the STL ordering (source_begin, source_end, dest). >>>> >>>>But I wouldn't be surprised if we're already doing that somewhere that >>>>I've forgotten about. >>>> >>>>WOuld it make sense to either rename this overload, or to use >>>>consistent argument ordering for the two __relocate_a overloads? >>> >>>The functions were not meant as overloads, it just happened that I >>>arrived at the same name for both, but it would make perfect sense >>>to give them different names. I started from __relocate(dest, >>>source) for one element, and later added an allocator to it. The >>>other one corresponds to __uninitialized_move_a, and naming it >>>__uninitialized_relocate_a would be silly since "uninitialized" is >>>included in the definition of relocate. >> >>Yes, my first thought was to add "uninitialized" and I rejected it for >>that same reason. >> >>>I think I'd rather rename than change the order. Do you have >>>suggestions? __relocate_range_a? >> >>I was thinking the single object one could be __relocate_1_a with the >>_1 being like copy_n, fill_n etc. but that's going to be confusing >>with __relocate_a_1 instead! >> >>It seems unfortunate to have to put "range" in the name when no other >>algos that work on ranges bother to say that in the name. >> >>Maybe the single object one could be __relocate_single_a? Or >>__do_relocate_a? __relocate_obj_a? None of them really makes me happy. > >I went with __relocate_object_a, but that's easy to change. I'm OK with that. If anybody thinks of a better name we can change it. >We may want to specialize / overload __relocate_object_a for some >specific types in the future, although we would more likely have >__relocate_object_a call __relocate_object (no allocator) when it >receives the default allocator, and specialize that one. And this >should anyway be less common that specializing >__is_trivially_relocatable. > >I just realized that __reallocate_a and construct don't take the >allocator argument in the same position... I can switch if it helps. No, leaving it at the end is consistent with __uninitialized_copy_a et al, and it's closer in spirit to them. OK for trunk, thanks!
On 25/10/18 13:36 +0100, Jonathan Wakely wrote: >On 25/10/18 14:30 +0200, Marc Glisse wrote: >>On Tue, 23 Oct 2018, Jonathan Wakely wrote: >> >>>>>>+ template<typename _Tp, typename _Up, typename _Allocator> >>>>>>+ inline void >>>>>>+ __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc) >>>>> >>>>>I find it a little surprising that this overload for single objects >>>>>using the memmove argument ordering (dest, source) but the range overload >>>>>below uses the STL ordering (source_begin, source_end, dest). >>>>> >>>>>But I wouldn't be surprised if we're already doing that somewhere that >>>>>I've forgotten about. >>>>> >>>>>WOuld it make sense to either rename this overload, or to use >>>>>consistent argument ordering for the two __relocate_a overloads? >>>> >>>>The functions were not meant as overloads, it just happened that >>>>I arrived at the same name for both, but it would make perfect >>>>sense to give them different names. I started from >>>>__relocate(dest, source) for one element, and later added an >>>>allocator to it. The other one corresponds to >>>>__uninitialized_move_a, and naming it __uninitialized_relocate_a >>>>would be silly since "uninitialized" is included in the >>>>definition of relocate. >>> >>>Yes, my first thought was to add "uninitialized" and I rejected it for >>>that same reason. >>> >>>>I think I'd rather rename than change the order. Do you have >>>>suggestions? __relocate_range_a? >>> >>>I was thinking the single object one could be __relocate_1_a with the >>>_1 being like copy_n, fill_n etc. but that's going to be confusing >>>with __relocate_a_1 instead! >>> >>>It seems unfortunate to have to put "range" in the name when no other >>>algos that work on ranges bother to say that in the name. >>> >>>Maybe the single object one could be __relocate_single_a? Or >>>__do_relocate_a? __relocate_obj_a? None of them really makes me happy. >> >>I went with __relocate_object_a, but that's easy to change. > >I'm OK with that. If anybody thinks of a better name we can change it. > >>We may want to specialize / overload __relocate_object_a for some >>specific types in the future, although we would more likely have >>__relocate_object_a call __relocate_object (no allocator) when it >>receives the default allocator, and specialize that one. And this >>should anyway be less common that specializing >>__is_trivially_relocatable. >> >>I just realized that __reallocate_a and construct don't take the >>allocator argument in the same position... I can switch if it helps. > >No, leaving it at the end is consistent with __uninitialized_copy_a >et al, and it's closer in spirit to them. > >OK for trunk, thanks! Here's the fix for the regression this introduced. Tested x86_64-linux, committed to trunk. commit caf2adcb563d7dd62c3de7bb49f528753f958ba3 Author: Jonathan Wakely <jwakely@redhat.com> Date: Fri Nov 9 19:19:16 2018 +0000 PR libstdc++/87787 fix UBsan error in std::vector PR libstdc++/87787 * include/bits/stl_uninitialized.h (__relocate_a_1): Do not call memmove when there's nothing to copy (and pointers could be null). diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h index 94c7e151e29..8839bfdcc90 100644 --- a/libstdc++-v3/include/bits/stl_uninitialized.h +++ b/libstdc++-v3/include/bits/stl_uninitialized.h @@ -904,7 +904,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Tp* __result, allocator<_Up>& __alloc) { ptrdiff_t __count = __last - __first; - __builtin_memmove(__result, __first, __count * sizeof(_Tp)); + if (__count > 0) + __builtin_memmove(__result, __first, __count * sizeof(_Tp)); return __result + __count; }
On Fri, 9 Nov 2018, Jonathan Wakely wrote:
> Here's the fix for the regression this introduced.
Thanks. I was going to handle it, but I was waiting for you to review the
second relocation patch first to avoid having several patches in flight
over the same piece of code. Anyway, having the regression fixed is good.
Index: include/bits/alloc_traits.h =================================================================== --- include/bits/alloc_traits.h (revision 264027) +++ include/bits/alloc_traits.h (working copy) @@ -233,38 +233,43 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using type = decltype(__test<_Alloc>(0)); }; template<typename _Tp, typename... _Args> using __has_construct = typename __construct_helper<_Tp, _Args...>::type; template<typename _Tp, typename... _Args> static _Require<__has_construct<_Tp, _Args...>> _S_construct(_Alloc& __a, _Tp* __p, _Args&&... __args) + noexcept(noexcept(__a.construct(__p, std::forward<_Args>(__args)...))) { __a.construct(__p, std::forward<_Args>(__args)...); } template<typename _Tp, typename... _Args> static _Require<__and_<__not_<__has_construct<_Tp, _Args...>>, is_constructible<_Tp, _Args...>>> _S_construct(_Alloc&, _Tp* __p, _Args&&... __args) + noexcept(noexcept(::new((void*)__p) + _Tp(std::forward<_Args>(__args)...))) { ::new((void*)__p) _Tp(std::forward<_Args>(__args)...); } template<typename _Alloc2, typename _Tp> static auto _S_destroy(_Alloc2& __a, _Tp* __p, int) + noexcept(noexcept(__a.destroy(__p))) -> decltype(__a.destroy(__p)) { __a.destroy(__p); } template<typename _Alloc2, typename _Tp> static void _S_destroy(_Alloc2&, _Tp* __p, ...) + noexcept(noexcept(__p->~_Tp())) { __p->~_Tp(); } template<typename _Alloc2> static auto _S_max_size(_Alloc2& __a, int) -> decltype(__a.max_size()) { return __a.max_size(); } template<typename _Alloc2> static size_type @@ -333,33 +338,36 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @param __p Pointer to memory of suitable size and alignment for Tp * @param __args Constructor arguments. * * Calls <tt> __a.construct(__p, std::forward<Args>(__args)...) </tt> * if that expression is well-formed, otherwise uses placement-new * to construct an object of type @a _Tp at location @a __p from the * arguments @a __args... */ template<typename _Tp, typename... _Args> static auto construct(_Alloc& __a, _Tp* __p, _Args&&... __args) + noexcept(noexcept(_S_construct(__a, __p, + std::forward<_Args>(__args)...))) -> decltype(_S_construct(__a, __p, std::forward<_Args>(__args)...)) { _S_construct(__a, __p, std::forward<_Args>(__args)...); } /** * @brief Destroy an object of type @a _Tp * @param __a An allocator. * @param __p Pointer to the object to destroy * * Calls @c __a.destroy(__p) if that expression is well-formed, * otherwise calls @c __p->~_Tp() */ template<typename _Tp> static void destroy(_Alloc& __a, _Tp* __p) + noexcept(noexcept(_S_destroy(__a, __p, 0))) { _S_destroy(__a, __p, 0); } /** * @brief The maximum supported allocation size * @param __a An allocator. * @return @c __a.max_size() or @c numeric_limits<size_type>::max() * * Returns @c __a.max_size() if that expression is well-formed, * otherwise returns @c numeric_limits<size_type>::max() */ @@ -465,32 +473,34 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @brief Construct an object of type @a _Up * @param __a An allocator. * @param __p Pointer to memory of suitable size and alignment for Tp * @param __args Constructor arguments. * * Calls <tt> __a.construct(__p, std::forward<Args>(__args)...) </tt> */ template<typename _Up, typename... _Args> static void construct(allocator_type& __a, _Up* __p, _Args&&... __args) + noexcept(noexcept(__a.construct(__p, std::forward<_Args>(__args)...))) { __a.construct(__p, std::forward<_Args>(__args)...); } /** * @brief Destroy an object of type @a _Up * @param __a An allocator. * @param __p Pointer to the object to destroy * * Calls @c __a.destroy(__p). */ template<typename _Up> static void destroy(allocator_type& __a, _Up* __p) + noexcept(noexcept(__a.destroy(__p))) { __a.destroy(__p); } /** * @brief The maximum supported allocation size * @param __a An allocator. * @return @c __a.max_size() */ static size_type max_size(const allocator_type& __a) noexcept { return __a.max_size(); } Index: include/bits/allocator.h =================================================================== --- include/bits/allocator.h (revision 264027) +++ include/bits/allocator.h (working copy) @@ -81,25 +81,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #if __cplusplus >= 201103L // _GLIBCXX_RESOLVE_LIB_DEFECTS // 2103. std::allocator propagate_on_container_move_assignment typedef true_type propagate_on_container_move_assignment; typedef true_type is_always_equal; template<typename _Up, typename... _Args> void construct(_Up* __p, _Args&&... __args) + noexcept(noexcept(::new((void *)__p) + _Up(std::forward<_Args>(__args)...))) { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); } template<typename _Up> void - destroy(_Up* __p) { __p->~_Up(); } + destroy(_Up* __p) + noexcept(noexcept(__p->~_Up())) + { __p->~_Up(); } #endif }; /** * @brief The @a standard allocator, as per [20.4]. * * See https://gcc.gnu.org/onlinedocs/libstdc++/manual/memory.html#std.util.memory.allocator * for further details. * * @tparam _Tp Type of allocated object. Index: include/bits/stl_uninitialized.h =================================================================== --- include/bits/stl_uninitialized.h (revision 264027) +++ include/bits/stl_uninitialized.h (working copy) @@ -872,14 +872,85 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION uninitialized_move_n(_InputIterator __first, _Size __count, _ForwardIterator __result) { auto __res = std::__uninitialized_copy_n_pair (_GLIBCXX_MAKE_MOVE_ITERATOR(__first), __count, __result); return {__res.first.base(), __res.second}; } #endif +#if __cplusplus >= 201703L + template<typename _Tp, typename _Up> + inline void + __relocate(_Tp* __dest, _Up* __orig) + noexcept(is_nothrow_constructible<_Tp, _Up&&>::value) + { + ::new((void*)__dest) _Tp(std::move(*__orig)); + __orig->~_Up(); + } + + template<typename _Tp, typename _Up, typename _Allocator> + inline void + __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc) + // people who oppose noexcept(auto) clearly never write C++ themselves + noexcept(noexcept(__gnu_cxx::__alloc_traits<_Allocator>::construct(__alloc, + __dest, std::move(*__orig))) + && noexcept(__gnu_cxx::__alloc_traits<_Allocator>::destroy( + __alloc, std::__addressof(*__orig)))) + { + typedef __gnu_cxx::__alloc_traits<_Allocator> __traits; + __traits::construct(__alloc, __dest, std::move(*__orig)); + __traits::destroy(__alloc, std::__addressof(*__orig)); + } + + template<typename _Tp> + struct __is_trivially_relocatable + : is_trivially_move_constructible<_Tp> { }; + + template <typename _Tp, typename _Up> + inline std::enable_if_t<std::__is_trivially_relocatable<_Tp>::value, _Tp*> + __relocate_a_1(_Tp* __first, _Tp* __last, + _Tp* __result, allocator<_Up>& __alloc) + { + ptrdiff_t __count = __last - __first; + __builtin_memmove(__result, __first, __count * sizeof(_Tp)); + return __result + __count; + } + + template <typename _InputIterator, typename _ForwardIterator, + typename _Allocator> + inline _ForwardIterator + __relocate_a_1(_InputIterator __first, _InputIterator __last, + _ForwardIterator __result, _Allocator& __alloc) + { + typedef typename iterator_traits<_InputIterator>::value_type + _ValueType; + typedef typename iterator_traits<_ForwardIterator>::value_type + _ValueType2; + static_assert(std::is_same_v<_ValueType, _ValueType2>); + static_assert(noexcept(std::__relocate_a(std::addressof(*__result), + std::addressof(*__first), + __alloc))); + _ForwardIterator __cur = __result; + for (; __first != __last; ++__first, (void)++__cur) + std::__relocate_a(std::__addressof(*__cur), + std::__addressof(*__first), __alloc); + return __cur; + } + + template <typename _InputIterator, typename _ForwardIterator, + typename _Allocator> + inline _ForwardIterator + __relocate_a(_InputIterator __first, _InputIterator __last, + _ForwardIterator __result, _Allocator& __alloc) + { + return __relocate_a_1(std::__niter_base(__first), + std::__niter_base(__last), + std::__niter_base(__result), __alloc); + } +#endif + _GLIBCXX_END_NAMESPACE_VERSION } // namespace #endif /* _STL_UNINITIALIZED_H */ Index: include/bits/stl_vector.h =================================================================== --- include/bits/stl_vector.h (revision 264027) +++ include/bits/stl_vector.h (working copy) @@ -398,20 +398,26 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER "std::vector must have a non-const, non-volatile value_type"); # ifdef __STRICT_ANSI__ static_assert(is_same<typename _Alloc::value_type, _Tp>::value, "std::vector must have the same value_type as its allocator"); # endif #endif typedef _Vector_base<_Tp, _Alloc> _Base; typedef typename _Base::_Tp_alloc_type _Tp_alloc_type; typedef __gnu_cxx::__alloc_traits<_Tp_alloc_type> _Alloc_traits; +#if __cplusplus >= 201703L + static constexpr bool __use_relocate = + noexcept(std::__relocate_a(std::declval<_Tp*>(), + std::declval<_Tp*>(), + std::declval<_Tp_alloc_type&>())); +#endif public: typedef _Tp value_type; typedef typename _Base::pointer pointer; typedef typename _Alloc_traits::const_pointer const_pointer; typedef typename _Alloc_traits::reference reference; typedef typename _Alloc_traits::const_reference const_reference; typedef __gnu_cxx::__normal_iterator<pointer, vector> iterator; typedef __gnu_cxx::__normal_iterator<const_pointer, vector> const_iterator; Index: include/bits/vector.tcc =================================================================== --- include/bits/vector.tcc (revision 264027) +++ include/bits/vector.tcc (working copy) @@ -64,26 +64,39 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER template<typename _Tp, typename _Alloc> void vector<_Tp, _Alloc>:: reserve(size_type __n) { if (__n > this->max_size()) __throw_length_error(__N("vector::reserve")); if (this->capacity() < __n) { const size_type __old_size = size(); - pointer __tmp = _M_allocate_and_copy(__n, - _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR(this->_M_impl._M_start), - _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR(this->_M_impl._M_finish)); + pointer __tmp; +#if __cplusplus >= 201703L + if constexpr (__use_relocate) + { + __tmp = this->_M_allocate(__n); + std::__relocate_a(this->_M_impl._M_start, + this->_M_impl._M_finish, + __tmp, _M_get_Tp_allocator()); + } + else +#endif + { + __tmp = _M_allocate_and_copy(__n, + _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR(this->_M_impl._M_start), + _GLIBCXX_MAKE_MOVE_IF_NOEXCEPT_ITERATOR(this->_M_impl._M_finish)); + std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish, + _M_get_Tp_allocator()); + } _GLIBCXX_ASAN_ANNOTATE_REINIT; - std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish, - _M_get_Tp_allocator()); _M_deallocate(this->_M_impl._M_start, this->_M_impl._M_end_of_storage - this->_M_impl._M_start); this->_M_impl._M_start = __tmp; this->_M_impl._M_finish = __tmp + __old_size; this->_M_impl._M_end_of_storage = this->_M_impl._M_start + __n; } } #if __cplusplus >= 201103L @@ -288,23 +301,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER vector<_Tp, _Alloc>:: _M_assign_aux(_ForwardIterator __first, _ForwardIterator __last, std::forward_iterator_tag) { const size_type __len = std::distance(__first, __last); if (__len > capacity()) { _S_check_init_len(__len, _M_get_Tp_allocator()); pointer __tmp(_M_allocate_and_copy(__len, __first, __last)); - _GLIBCXX_ASAN_ANNOTATE_REINIT; std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish, _M_get_Tp_allocator()); + _GLIBCXX_ASAN_ANNOTATE_REINIT; _M_deallocate(this->_M_impl._M_start, this->_M_impl._M_end_of_storage - this->_M_impl._M_start); this->_M_impl._M_start = __tmp; this->_M_impl._M_finish = this->_M_impl._M_start + __len; this->_M_impl._M_end_of_storage = this->_M_impl._M_finish; } else if (size() >= __len) _M_erase_at_end(std::copy(__first, __last, this->_M_impl._M_start)); else @@ -436,44 +449,66 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER // [res.on.arguments]). _Alloc_traits::construct(this->_M_impl, __new_start + __elems_before, #if __cplusplus >= 201103L std::forward<_Args>(__args)...); #else __x); #endif __new_finish = pointer(); - __new_finish - = std::__uninitialized_move_if_noexcept_a - (__old_start, __position.base(), - __new_start, _M_get_Tp_allocator()); - - ++__new_finish; - - __new_finish - = std::__uninitialized_move_if_noexcept_a - (__position.base(), __old_finish, - __new_finish, _M_get_Tp_allocator()); +#if __cplusplus >= 201703L + if constexpr (__use_relocate) + { + __new_finish + = std::__relocate_a + (__old_start, __position.base(), + __new_start, _M_get_Tp_allocator()); + + ++__new_finish; + + __new_finish + = std::__relocate_a + (__position.base(), __old_finish, + __new_finish, _M_get_Tp_allocator()); + } + else +#endif + { + __new_finish + = std::__uninitialized_move_if_noexcept_a + (__old_start, __position.base(), + __new_start, _M_get_Tp_allocator()); + + ++__new_finish; + + __new_finish + = std::__uninitialized_move_if_noexcept_a + (__position.base(), __old_finish, + __new_finish, _M_get_Tp_allocator()); + } } __catch(...) { if (!__new_finish) _Alloc_traits::destroy(this->_M_impl, __new_start + __elems_before); else std::_Destroy(__new_start, __new_finish, _M_get_Tp_allocator()); _M_deallocate(__new_start, __len); __throw_exception_again; } +#if __cplusplus >= 201703L + if constexpr (!__use_relocate) +#endif + std::_Destroy(__old_start, __old_finish, _M_get_Tp_allocator()); _GLIBCXX_ASAN_ANNOTATE_REINIT; - std::_Destroy(__old_start, __old_finish, _M_get_Tp_allocator()); _M_deallocate(__old_start, this->_M_impl._M_end_of_storage - __old_start); this->_M_impl._M_start = __new_start; this->_M_impl._M_finish = __new_finish; this->_M_impl._M_end_of_storage = __new_start + __len; } template<typename _Tp, typename _Alloc> void vector<_Tp, _Alloc>:: @@ -555,23 +590,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER if (!__new_finish) std::_Destroy(__new_start + __elems_before, __new_start + __elems_before + __n, _M_get_Tp_allocator()); else std::_Destroy(__new_start, __new_finish, _M_get_Tp_allocator()); _M_deallocate(__new_start, __len); __throw_exception_again; } - _GLIBCXX_ASAN_ANNOTATE_REINIT; std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish, _M_get_Tp_allocator()); + _GLIBCXX_ASAN_ANNOTATE_REINIT; _M_deallocate(this->_M_impl._M_start, this->_M_impl._M_end_of_storage - this->_M_impl._M_start); this->_M_impl._M_start = __new_start; this->_M_impl._M_finish = __new_finish; this->_M_impl._M_end_of_storage = __new_start + __len; } } } @@ -596,41 +631,62 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER this->_M_impl._M_finish = std::__uninitialized_default_n_a(this->_M_impl._M_finish, __n, _M_get_Tp_allocator()); _GLIBCXX_ASAN_ANNOTATE_GREW(__n); } else { const size_type __len = _M_check_len(__n, "vector::_M_default_append"); pointer __new_start(this->_M_allocate(__len)); - pointer __destroy_from = pointer(); - __try +#if __cplusplus >= 201703L + if constexpr (__use_relocate) { - std::__uninitialized_default_n_a(__new_start + __size, - __n, _M_get_Tp_allocator()); - __destroy_from = __new_start + __size; - std::__uninitialized_move_if_noexcept_a( - this->_M_impl._M_start, this->_M_impl._M_finish, - __new_start, _M_get_Tp_allocator()); + __try + { + std::__uninitialized_default_n_a(__new_start + __size, + __n, _M_get_Tp_allocator()); + } + __catch(...) + { + _M_deallocate(__new_start, __len); + __throw_exception_again; + } + std::__relocate_a(this->_M_impl._M_start, + this->_M_impl._M_finish, + __new_start, _M_get_Tp_allocator()); } - __catch(...) + else +#endif { - if (__destroy_from) - std::_Destroy(__destroy_from, __destroy_from + __n, - _M_get_Tp_allocator()); - _M_deallocate(__new_start, __len); - __throw_exception_again; + pointer __destroy_from = pointer(); + __try + { + std::__uninitialized_default_n_a(__new_start + __size, + __n, _M_get_Tp_allocator()); + __destroy_from = __new_start + __size; + std::__uninitialized_move_if_noexcept_a( + this->_M_impl._M_start, this->_M_impl._M_finish, + __new_start, _M_get_Tp_allocator()); + } + __catch(...) + { + if (__destroy_from) + std::_Destroy(__destroy_from, __destroy_from + __n, + _M_get_Tp_allocator()); + _M_deallocate(__new_start, __len); + __throw_exception_again; + } + std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish, + _M_get_Tp_allocator()); } _GLIBCXX_ASAN_ANNOTATE_REINIT; - std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish, - _M_get_Tp_allocator()); _M_deallocate(this->_M_impl._M_start, this->_M_impl._M_end_of_storage - this->_M_impl._M_start); this->_M_impl._M_start = __new_start; this->_M_impl._M_finish = __new_start + __size + __n; this->_M_impl._M_end_of_storage = __new_start + __len; } } } @@ -735,23 +791,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER (__position.base(), this->_M_impl._M_finish, __new_finish, _M_get_Tp_allocator()); } __catch(...) { std::_Destroy(__new_start, __new_finish, _M_get_Tp_allocator()); _M_deallocate(__new_start, __len); __throw_exception_again; } - _GLIBCXX_ASAN_ANNOTATE_REINIT; std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish, _M_get_Tp_allocator()); + _GLIBCXX_ASAN_ANNOTATE_REINIT; _M_deallocate(this->_M_impl._M_start, this->_M_impl._M_end_of_storage - this->_M_impl._M_start); this->_M_impl._M_start = __new_start; this->_M_impl._M_finish = __new_finish; this->_M_impl._M_end_of_storage = __new_start + __len; } } } Index: include/ext/alloc_traits.h =================================================================== --- include/ext/alloc_traits.h (revision 264027) +++ include/ext/alloc_traits.h (working copy) @@ -73,29 +73,32 @@ template<typename _Alloc, typename = typ template<typename _Ptr> using __is_custom_pointer = std::__and_<std::is_same<pointer, _Ptr>, std::__not_<std::is_pointer<_Ptr>>>; public: // overload construct for non-standard pointer types template<typename _Ptr, typename... _Args> static typename std::enable_if<__is_custom_pointer<_Ptr>::value>::type construct(_Alloc& __a, _Ptr __p, _Args&&... __args) + noexcept(noexcept(_Base_type::construct(__a, std::__to_address(__p), + std::forward<_Args>(__args)...))) { _Base_type::construct(__a, std::__to_address(__p), std::forward<_Args>(__args)...); } // overload destroy for non-standard pointer types template<typename _Ptr> static typename std::enable_if<__is_custom_pointer<_Ptr>::value>::type destroy(_Alloc& __a, _Ptr __p) + noexcept(noexcept(_Base_type::destroy(__a, std::__to_address(__p)))) { _Base_type::destroy(__a, std::__to_address(__p)); } static _Alloc _S_select_on_copy(const _Alloc& __a) { return _Base_type::select_on_container_copy_construction(__a); } static void _S_on_swap(_Alloc& __a, _Alloc& __b) { std::__alloc_on_swap(__a, __b); } static constexpr bool _S_propagate_on_copy_assign() { return _Base_type::propagate_on_container_copy_assignment::value; } Index: include/ext/malloc_allocator.h =================================================================== --- include/ext/malloc_allocator.h (revision 264027) +++ include/ext/malloc_allocator.h (working copy) @@ -138,25 +138,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { std::free(static_cast<void*>(__p)); } size_type max_size() const _GLIBCXX_USE_NOEXCEPT { return size_t(-1) / sizeof(_Tp); } #if __cplusplus >= 201103L template<typename _Up, typename... _Args> void construct(_Up* __p, _Args&&... __args) + noexcept(noexcept(::new((void *)__p) + _Up(std::forward<_Args>(__args)...))) { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); } template<typename _Up> void - destroy(_Up* __p) { __p->~_Up(); } + destroy(_Up* __p) + noexcept(noexcept(__p->~_Up())) + { __p->~_Up(); } #else // _GLIBCXX_RESOLVE_LIB_DEFECTS // 402. wrong new expression in [some_] allocator::construct void construct(pointer __p, const _Tp& __val) { ::new((void *)__p) value_type(__val); } void destroy(pointer __p) { __p->~_Tp(); } #endif Index: include/ext/new_allocator.h =================================================================== --- include/ext/new_allocator.h (revision 264027) +++ include/ext/new_allocator.h (working copy) @@ -129,25 +129,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } size_type max_size() const _GLIBCXX_USE_NOEXCEPT { return size_t(-1) / sizeof(_Tp); } #if __cplusplus >= 201103L template<typename _Up, typename... _Args> void construct(_Up* __p, _Args&&... __args) + noexcept(noexcept(::new((void *)__p) + _Up(std::forward<_Args>(__args)...))) { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); } template<typename _Up> void - destroy(_Up* __p) { __p->~_Up(); } + destroy(_Up* __p) + noexcept(noexcept( __p->~_Up())) + { __p->~_Up(); } #else // _GLIBCXX_RESOLVE_LIB_DEFECTS // 402. wrong new expression in [some_] allocator::construct void construct(pointer __p, const _Tp& __val) { ::new((void *)__p) _Tp(__val); } void destroy(pointer __p) { __p->~_Tp(); } #endif