This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: debug container patch


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]