debug container patch

François Dumont frs.dumont@gmail.com
Fri May 2 20:33:00 GMT 2014


Hi Jonathan

     I just wanted to make sure that you are aware that I preferred to 
wait for another validation of the small modification I have done.

François


On 28/04/2014 23:07, François Dumont wrote:
> On 27/04/2014 15:39, Jonathan Wakely wrote:
>> On 17/04/14 22:43 +0200, François Dumont wrote:
>>> Hi
>>>
>>>    Here is a patch to globally enhance debug containers implementation.
>>
>> François, sorry for the delay, this is a large patch and I wanted to
>> give it the time it deserves to review properly.
>
> No problem, I see that there are a lot of proposals lately.
>
>>
>> I understand why this is needed, but it changes the layout of the
>> classes in very significant ways, meaning the debug containers will
>> not be compatible across GCC releases. I'm OK with that now, but from
>> the next major GCC release I'd like to avoid that in future.
>
> I remember Paolo saying that there is no abi guaranty for debug mode 
> this is why I didn't hesitate in making this proposal. Will there be 
> one in the future ? I plan also breaking changes for profile mode to 
> fix its very bad performance.
>
>>
>>>    I noticed also that in std/c++11/debug.cc we have some methods 
>>> qualified with noexcept while in a C++03 user code those methods 
>>> will have a throw() qualification. Is that fine ?
>>
>> As I said in my last mail, yes, those specifications are compatible.
>> But I don't think your changes are doing what you think they are doing
>> in all cases. Using _GLIBCXX_NOEXCEPT does not expand to throw() in
>> C++03 mode, it expands to nothing.
>
> Yes, I discover this difference in one of your recent mail.
>
>>
>>>    * include/debug/safe_unordered_container.tcc
>>
>> N.B. This file has no changes listed in the changelog entry.
>
> I reviewed the ChangeLog and limit modifications like in this file. 
> Note however that patch have been generated with '-x -b' option to 
> hide white space modifications. I clean usage of white chars in 
> impacted files, replaced some white spaces with tabs and remove 
> useless white spaces.
>
>>
>>> @@ -69,8 +75,26 @@
>>>
>>>       // 23.2.1.1 construct/copy/destroy:
>>>
>>> -      deque() : _Base() { }
>>> +#if __cplusplus < 201103L
>>> +      deque()
>>> +      : _Base() { }
>>>
>>> +      deque(const deque& __x)
>>> +      : _Base(__x) { }
>>> +
>>> +      ~deque() _GLIBCXX_NOEXCEPT { }
>>
>> In C++03 mode the _GLIBCXX_NOEXCEPT macro expands to an empty string,
>> so it is useless in this chunk of code, which is only compiled for
>> C++03 mode. It should probably just be removed here (and in all the
>> other debug containers which use it in C++03-only code).
>
> Ok, I cleaned those. Did you mean removing the whole explicit 
> destructor ? Is it a coding Standard to always explicitly implement 
> the destructor or just a way to have Doxygen generate ?
>
>> + * before-begin ownership.*/
>>> +   template<typename _SafeSequence>
>>> +    void
>>> +    _Safe_forward_list<_SafeSequence>::
>>> +    _M_swap(_Safe_sequence_base& __other) noexcept
>>> +    {
>>> +      __gnu_cxx::__scoped_lock sentry(_M_this()._M_get_mutex());
>>
>> Shouldn't we be locking both containers' mutexes here?
>> As we do in src/c++11/debug.cc
>
> Good point, not a regression but nice to fix in this patch.
>
>>
>>> + forward_list(forward_list&& __list, const allocator_type& __al)
>>> +    : _Safe(std::move(__list), __al),
>>> +      _Base(std::move(__list), __al)
>>> +      { }
>>
>> This makes me feel uneasy, seeing a moved-from object being used
>> again, but I don't think changing it to use static_casts to the two
>> base classes would look better, so let's leave it like that.
>
> That indeed looks scary, we replaced with:
>
>       forward_list(forward_list&& __list, const allocator_type& __al)
>     : _Safe(std::move(__list._M_safe()), __al),
>       _Base(std::move(__list._M_base()), __al)
>       { }
>
> it makes clearer the fact that we move each part.
>
>>
>>> Index: include/debug/safe_base.h
>>> ===================================================================
>>> --- include/debug/safe_base.h    (revision 209446)
>>> +++ include/debug/safe_base.h    (working copy)
>>> @@ -188,22 +188,18 @@
>>>
>>>   protected:
>>>     // Initialize with a version number of 1 and no iterators
>>> -    _Safe_sequence_base()
>>> +    _Safe_sequence_base() _GLIBCXX_NOEXCEPT
>>
>> This use of _GLIBCXX_NOEXCEPT are correct, if the intention is to be
>> noexcept in C++11 and have no exception specification in C++98/C++03.
>
> Yes, I preferred to use default implementation for special function in 
> C++11 so I qualified as many things as possible noexcept so that 
> resulting noexcept qualification depends only on the normal mode 
> noexcept qualification.
>
>>
>>>     : _M_iterators(0), _M_const_iterators(0), _M_version(1)
>>>     { }
>>>
>>> #if __cplusplus >= 201103L
>>>     _Safe_sequence_base(const _Safe_sequence_base&) noexcept
>>>       : _Safe_sequence_base() { }
>>> -
>>> -    _Safe_sequence_base(_Safe_sequence_base&& __x) noexcept
>>> -      : _Safe_sequence_base()
>>> -    { _M_swap(__x); }
>>> #endif
>>>
>>>     /** Notify all iterators that reference this sequence that the
>>>     sequence is being destroyed. */
>>> -    ~_Safe_sequence_base()
>>> +    ~_Safe_sequence_base() _GLIBCXX_NOEXCEPT
>>
>> This is redundant. In C++03 the macro expands to nothing, and in C++11
>> destructors are noexcept by default anyway, so the macro adds nothing.
>
> Ok, I didn't knew, I cleaned those then.
>
>>
>>>     { this->_M_detach_all(); }
>>>
>>>     /** Detach all iterators, leaving them singular. */
>>> @@ -231,7 +227,7 @@
>>>      *  one container now reference the other container.
>>>      */
>>>     void
>>> -    _M_swap(_Safe_sequence_base& __x);
>>> +    _M_swap(_Safe_sequence_base& __x) _GLIBCXX_NOEXCEPT;
>>
>> This is OK, if the intention is to only have an exc eption spec in
>> C++11.
>
>     This is used in noexcept constructors so I prefer to make it 
> noexcept too.
>
>     I replaced this one with _GLIBCXX_USE_NOEXCEPT cause in 
> src/c++11/debug.cc we always build it as noexcept, just in case it 
> impacts the mangled name. I saw _GLIBCXX_NOTHROW in c++config but 
> doesn't seem to be used.
>
>>
>>
>>> +#if __cplusplus >= 201103L
>>> +      _Safe_container&
>>> +      operator=(_Safe_container&& __x) noexcept
>>> +      {
>>> +    __glibcxx_check_self_move_assign(__x);
>>
>> N.B. I'm writing a paper for the next committee meeting to make it
>> clear that self-move-assignment is not undefined behaviour, so this
>> check will have to be removed in the future. At least your change
>> means there are fewer places to remove it from :-)
>> See http://gcc.gnu.org/PR59603 for some background.
>>
>> I only looked over the testsuite changes quickly, but they look good.
>>
>> The changes above are only minor, overall I like the proposal very
>> much. Thanks for working on this, one day our debug mode containers
>> will be perfect! :-)
>>
>
> Not yet, I will start submitting patches for the debug algos soon :-)
>
> Here is the patch again with all your remarks. Ok to commit with the 
> following ChangeLog ?
>
> 2014-04-29  François Dumont <fdumont@gcc.gnu.org>
>
>     * include/debug/macros.h [__glibcxx_check_equal_allocs]: Add
>     parameter to pass the 2 instances to check allocator equality.
>     * include/debug/safe_container.h: New, define _Safe_container<>.
>     * include/Makefile.am: Add previous.
>     * include/debug/deque (std::__debug::deque<>): Inherit
>     _Safe_container<>. Use default implementation for all special
>     functions.
>     * include/debug/forward_list (std::__debug::forward_list<>):
>     Likewise.
>     * include/debug/list (std::__debug::list<>): Likewise.
>     * include/debug/map.h (std::__debug::map<>): Likewise.
>     * include/debug/multimap.h (std::__debug::multimap<>): Likewise.
>     * include/debug/set.h (std::__debug::set<>): Likewise.
>     * include/debug/multiset.h (std::__debug::multiset<>): Likewise.
>     * include/debug/string (std::__debug::basic_string<>): Likewise.
>     * include/debug/unordered_map
>     (std::__debug::unordered_map<>): Likewise.
>     (std::__debug::unordered_multimap<>): Likewise.
>     * include/debug/unordered_set
>     (std::__debug::unordered_set<>): Likewise.
>     (std::__debug::unordered_multiset<>): Likewise.
>     * include/debug/vector (std::__debug::vector<>): Likewise.
>     * include/debug/safe_base.h (_Safe_sequence_base()): Add
>     noexcept.
>     (_Safe_sequence_base(_Safe_sequence_base&&): Remove.
>     (~_Safe_sequence_base()): Add noexcept.
>     * include/debug/safe_sequence.h
>     (std::__debug::_Safe_node_sequence<>): New.
>     * include/debug/safe_unordered_base.h
>     (_Safe_unordered_container_base()): Add noexcept.
>     (~_Safe_unordered_container_base()): Likewise.
>     (_M_swap(_Safe_unordered_container_base&)): Likewise.
>     * include/debug/safe_unordered_container.h:
>     (_Safe_unordered_container<>::_M_invalidate_locals()): New.
>     (_Safe_unordered_container<>::_M_invalidate_all()): New.
>     * src/c++11/debug.cc: Limit includes, adapt methods noexcept
>     qualifications.
>     * testsuite/util/debug/checks.h (check_construct1): Just implement
>     an invalid constructor invocation  and no other operations
>     potentially not supported by some types of container.
>     (check_construct2): Likewise.
>     (check_construct3): Likewise.
>     * testsuite/23_containers/forward_list/allocator/move.cc: Add
>     check on iterators to make sure they are correctly moved in debug
>     mode.
>     * testsuite/23_containers/forward_list/allocator/move_assign.cc:
>     Likewise.
>     * testsuite/23_containers/map/allocator/move.cc: Likewise.
>     * testsuite/23_containers/map/allocator/move_assign.cc: Likewise.
>     * testsuite/23_containers/multimap/allocator/move.cc: Likewise.
>     * testsuite/23_containers/multimap/allocator/move_assign.cc:
>     Likewise.
>     * testsuite/23_containers/multiset/allocator/move.cc: Likewise.
>     * testsuite/23_containers/multiset/allocator/move_assign.cc:
>     Likewise.
>     * testsuite/23_containers/set/allocator/move.cc: Likewise.
>     * testsuite/23_containers/set/allocator/move_assign.cc: Likewise.
>     * testsuite/23_containers/unordered_map/allocator/move.cc:
>     Likewise.
>     * testsuite/23_containers/unordered_map/allocator/move_assign.cc:
>     Likewise.
>     * testsuite/23_containers/unordered_multimap/allocator/move.cc:
>     Likewise.
>     * 
> testsuite/23_containers/unordered_multimap/allocator/move_assign.cc:
>     Likewise.
>     * testsuite/23_containers/unordered_multiset/allocator/move.cc:
>     Likewise.
>     * 
> testsuite/23_containers/unordered_multiset/allocator/move_assign.cc:
>     Likewise.
>     * testsuite/23_containers/unordered_set/allocator/move.cc:
>     Likewise.
>     * testsuite/23_containers/unordered_set/allocator/move_assign.cc:
>     Likewise.
>     * testsuite/23_containers/forward_list/debug/construct1_neg.cc:
>     New.
>     * testsuite/23_containers/forward_list/debug/construct2_neg.cc:
>     New.
>     * testsuite/23_containers/forward_list/debug/construct3_neg.cc:
>     New.
>     * testsuite/23_containers/forward_list/debug/construct4_neg.cc:
>     New.
>     * testsuite/23_containers/forward_list/debug/move_assign_neg.cc:
>     New.
>     * testsuite/23_containers/forward_list/debug/move_neg.cc: New.
>     * testsuite/23_containers/map/debug/construct5_neg.cc: New.
>     * testsuite/23_containers/map/debug/move_assign_neg.cc: New.
>     * testsuite/23_containers/map/debug/move_neg.cc: New.
>     * testsuite/23_containers/multimap/debug/construct5_neg.cc: New.
>     * testsuite/23_containers/multimap/debug/move_assign_neg.cc: New.
>     * testsuite/23_containers/multimap/debug/move_neg.cc: New.
>     * testsuite/23_containers/multiset/debug/construct5_neg.cc: New.
>     * testsuite/23_containers/multiset/debug/move_assign_neg.cc: New.
>     * testsuite/23_containers/multiset/debug/move_neg.cc: New.
>     * testsuite/23_containers/set/debug/construct5_neg.cc: New.
>     * testsuite/23_containers/set/debug/move_assign_neg.cc: New.
>     * testsuite/23_containers/set/debug/move_neg.cc: New.
>     * testsuite/23_containers/unordered_map/debug/construct5_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_map/debug/move_assign_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_map/debug/move_neg.cc: New.
>     * testsuite/23_containers/unordered_multimap/debug/construct5_neg.cc:
>     New.
>     * 
> testsuite/23_containers/unordered_multimap/debug/move_assign_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_multimap/debug/move_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_multiset/debug/construct5_neg.cc:
>     New.
>     * 
> testsuite/23_containers/unordered_multiset/debug/move_assign_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_multiset/debug/move_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_set/debug/construct5_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_set/debug/move_assign_neg.cc:
>     New.
>     * testsuite/23_containers/unordered_set/debug/move_neg.cc: New.
>     * testsuite/23_containers/vector/debug/move_neg.cc: New.
>
>
> François
>



More information about the Libstdc++ mailing list