Re: [Bug libstdc++/89608] Undetected iterator invalidations on unordered containers in debug mode

Re: [Bug libstdc++/89608] Undetected iterator invalidations on unordered containers in debug mode

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

Hi

     I consider the implementation to decide to invalidate iterators or 
not. As nodes are not deallocated and only slghtly impacted during the 
rehash process I consider that they shouldn't be invalidated appart from 
the local iterators. I should have just consider the Standard.

     Here is the complete patch which is Max Sistemich proposal but 
extended to the other unordered containers with also the test adapted 
for the testsuite.

PR libstdc++/89608
* include/debug/unordered_map (unordered_map<>::_M_check_rehashed):
   Invalidate all iterators in case of rehash.
   (unordered_multimap<>::_M_check_rehashed): Likewise.
* include/debug/unordered_set (unordered_set<>::_M_check_rehashed):
    Likewise.
    (unordered_multiset<>::_M_check_rehashed): Likewise.

* testsuite/23_containers/unordered_set/debug/89608_neg.cc: New.

     I run all unordered tests so far in Debug mode. I have 4 unrelated 
failures that I'll fix through another patch. Ok to commit once all 
tests run ?

François


[-- Attachment #2: 89608.patch --]
[-- Type: text/x-patch, Size: 2842 bytes --]

diff --git a/libstdc++-v3/include/debug/unordered_map b/libstdc++-v3/include/debug/unordered_map
index 7b0ea6d768b..163acd7f7ec 100644
--- a/libstdc++-v3/include/debug/unordered_map
+++ b/libstdc++-v3/include/debug/unordered_map
@@ -611,7 +611,7 @@ namespace __debug
       _M_check_rehashed(size_type __prev_count)
       {
 	if (__prev_count != this->bucket_count())
-	  this->_M_invalidate_locals();
+	  this->_M_invalidate_all();
       }
 
       void
@@ -1210,7 +1210,7 @@ namespace __debug
       _M_check_rehashed(size_type __prev_count)
       {
 	if (__prev_count != this->bucket_count())
-	  this->_M_invalidate_locals();
+	  this->_M_invalidate_all();
       }
 
       void
diff --git a/libstdc++-v3/include/debug/unordered_set b/libstdc++-v3/include/debug/unordered_set
index 8f44bd92f2a..22e0abd3283 100644
--- a/libstdc++-v3/include/debug/unordered_set
+++ b/libstdc++-v3/include/debug/unordered_set
@@ -496,7 +496,7 @@ namespace __debug
       _M_check_rehashed(size_type __prev_count)
       {
 	if (__prev_count != this->bucket_count())
-	  this->_M_invalidate_locals();
+	  this->_M_invalidate_all();
       }
 
       void
@@ -1050,7 +1050,7 @@ namespace __debug
       _M_check_rehashed(size_type __prev_count)
       {
 	if (__prev_count != this->bucket_count())
-	  this->_M_invalidate_locals();
+	  this->_M_invalidate_all();
       }
 
       void
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/debug/89608_neg.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/debug/89608_neg.cc
new file mode 100644
index 00000000000..871b1c3381d
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/debug/89608_neg.cc
@@ -0,0 +1,37 @@
+// Copyright (C) 2019 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+//
+// { dg-do run { target c++11 xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+// PR libstdc++/89608
+
+#include <unordered_set>
+
+int main()
+{
+  std::unordered_set<int> myset;
+  myset.reserve(2);
+  myset.insert(0);
+  myset.insert(1);
+
+  int i = 2;
+  for (auto it = myset.begin(), end = myset.end(); it != end; ++it)
+    myset.insert(i++);
+
+  return 0;
+}

Re: [Bug libstdc++/89608] Undetected iterator invalidations on unordered containers in debug mode

[Jonathan Wakely]

On 07/03/19 22:22 +0100, François Dumont wrote:
>Hi
>
>    I consider the implementation to decide to invalidate iterators or 
>not. As nodes are not deallocated and only slghtly impacted during the 
>rehash process I consider that they shouldn't be invalidated appart 
>from the local iterators. I should have just consider the Standard.
>
>    Here is the complete patch which is Max Sistemich proposal but 
>extended to the other unordered containers with also the test adapted 
>for the testsuite.
>
>PR libstdc++/89608
>* include/debug/unordered_map (unordered_map<>::_M_check_rehashed):
>  Invalidate all iterators in case of rehash.
>  (unordered_multimap<>::_M_check_rehashed): Likewise.
>* include/debug/unordered_set (unordered_set<>::_M_check_rehashed):
>   Likewise.
>   (unordered_multiset<>::_M_check_rehashed): Likewise.
>
>* testsuite/23_containers/unordered_set/debug/89608_neg.cc: New.
>
>    I run all unordered tests so far in Debug mode. I have 4 unrelated 
>failures that I'll fix through another patch. Ok to commit once all 
>tests run ?

Yes, this is OK for trunk, thanks!

Re: [Bug libstdc++/89608] Undetected iterator invalidations on unordered containers in debug mode

[Jonathan Wakely]

On 07/03/19 23:57 +0000, Jonathan Wakely wrote:
>On 07/03/19 22:22 +0100, François Dumont wrote:
>>Hi
>>
>>    I consider the implementation to decide to invalidate iterators 
>>or not. As nodes are not deallocated and only slghtly impacted 
>>during the rehash process I consider that they shouldn't be 
>>invalidated appart from the local iterators. I should have just 
>>consider the Standard.
>>
>>    Here is the complete patch which is Max Sistemich proposal but 
>>extended to the other unordered containers with also the test 
>>adapted for the testsuite.
>>
>>PR libstdc++/89608
>>* include/debug/unordered_map (unordered_map<>::_M_check_rehashed):
>>  Invalidate all iterators in case of rehash.
>>  (unordered_multimap<>::_M_check_rehashed): Likewise.
>>* include/debug/unordered_set (unordered_set<>::_M_check_rehashed):
>>   Likewise.
>>   (unordered_multiset<>::_M_check_rehashed): Likewise.
>>
>>* testsuite/23_containers/unordered_set/debug/89608_neg.cc: New.
>>
>>    I run all unordered tests so far in Debug mode. I have 4 
>>unrelated failures that I'll fix through another patch. Ok to commit 
>>once all tests run ?
>
>Yes, this is OK for trunk, thanks!

I guess it's probably OK for the branches too, but let's wait a bit
and commit it to the branches later.