[review] gdb_vecs.h: Avoid self move assign

Message ID gerrit.1573259780000.I80247b20cd5212038117db7412865f5e6a9257cd@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Nov. 9, 2019, 12:36 a.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/601
......................................................................

gdb_vecs.h: Avoid self move assign

While working on another patch I ran into an issue with
unordered_remove (in gdb_vecs.h), where removing the last item of the
vector can cause a self move assign.

When compiling the C++ standard library in debug mode (with
-D_GLIBCXX_DEBUG=1) this causes an error to trigger.

I've fixed the issue in this patch and provided a unit test.

The provided unit test includes an assignment operator which checks
for self move assign, this removes the need to compile with
-D_GLIBCXX_DEBUG=1 in order to spot the bug.  If you're keen to see
the error reported from the C++ standard library then remove operator=
from the unit test and recompile GDB with -D_GLIBCXX_DEBUG=1.

gdb/ChangeLog:

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add new file to the list.
	* unittests/vec-utils-selftests.c: New file.
	* gdbsupport/gdb_vecs.h (unordered_remove): Avoid self move assign.

Change-Id: I80247b20cd5212038117db7412865f5e6a9257cd
---
M gdb/ChangeLog
M gdb/Makefile.in
M gdb/gdbsupport/gdb_vecs.h
A gdb/unittests/vec-utils-selftests.c
4 files changed, 75 insertions(+), 1 deletion(-)
  

Comments

Simon Marchi (Code Review) Nov. 9, 2019, 5:03 p.m. UTC | #1
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/601
......................................................................


Patch Set 1: Code-Review+2

Thank you.  This looks good to me.
  
Tom de Vries Nov. 12, 2019, 2:38 p.m. UTC | #2
On 09-11-2019 01:36, Andrew Burgess (Code Review) wrote:
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/601
> ......................................................................
> 
> gdb_vecs.h: Avoid self move assign
> 
> While working on another patch I ran into an issue with
> unordered_remove (in gdb_vecs.h), where removing the last item of the
> vector can cause a self move assign.
> 
> When compiling the C++ standard library in debug mode (with
> -D_GLIBCXX_DEBUG=1) this causes an error to trigger.
> 
> I've fixed the issue in this patch and provided a unit test.
> 
> The provided unit test includes an assignment operator which checks
> for self move assign, this removes the need to compile with
> -D_GLIBCXX_DEBUG=1 in order to spot the bug.  If you're keen to see
> the error reported from the C++ standard library then remove operator=
> from the unit test and recompile GDB with -D_GLIBCXX_DEBUG=1.
> 
> gdb/ChangeLog:
> 
> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add new file to the list.
> 	* unittests/vec-utils-selftests.c: New file.
> 	* gdbsupport/gdb_vecs.h (unordered_remove): Avoid self move assign.
> 
> Change-Id: I80247b20cd5212038117db7412865f5e6a9257cd
> ---
> M gdb/ChangeLog
> M gdb/Makefile.in
> M gdb/gdbsupport/gdb_vecs.h
> A gdb/unittests/vec-utils-selftests.c
> 4 files changed, 75 insertions(+), 1 deletion(-)

Hi,

this breaks my build with gcc-9:
...
In file included from
/data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:23:
/data/gdb_versions/devel/src/gdb/gdbsupport/gdb_vecs.h: In instantiation
of 'T unordered_remove(std::vector<T>&, \
typename std::vector<T>::iterator) [with T =
selftests::vector_utils_tests::unordered_remove_tests()::obj; typena\
me std::vector<T>::iterator =
__gnu_cxx::__normal_iterator<selftests::vector_utils_tests::unordered_remove_tests(\
)::obj*,
std::vector<selftests::vector_utils_tests::unordered_remove_tests()::obj> >]':
/data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:53:26:
  required from here
/data/gdb_versions/devel/src/gdb/gdbsupport/gdb_vecs.h:53:5: error:
implicitly-declared 'selftests::vector_utils_\
tests::unordered_remove_tests()::obj::obj(const
selftests::vector_utils_tests::unordered_remove_tests()::obj&)' i\
s deprecated [-Werror=deprecated-copy]
   53 |   T removed = std::move (*it);
      |     ^~~~~~~
/data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:41:10:
note: because 'selftests::vector_utils_te\
sts::unordered_remove_tests()::obj' has user-provided
'selftests::vector_utils_tests::unordered_remove_tests()::o\
bj&
selftests::vector_utils_tests::unordered_remove_tests()::obj::operator=(const
selftests::vector_utils_tests::\
unordered_remove_tests()::obj&)'
   41 |     obj &operator= (const obj &other)
      |          ^~~~~~~~
In file included from
/data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:23:
/data/gdb_versions/devel/src/gdb/gdbsupport/gdb_vecs.h:58:10: error:
implicitly-declared 'selftests::vector_utils\
_tests::unordered_remove_tests()::obj::obj(const
selftests::vector_utils_tests::unordered_remove_tests()::obj&)' \
is deprecated [-Werror=deprecated-copy]
   58 |   return removed;
      |          ^~~~~~~
/data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:41:10:
note: because 'selftests::vector_utils_te\
sts::unordered_remove_tests()::obj' has user-provided
'selftests::vector_utils_tests::unordered_remove_tests()::o\
bj&
selftests::vector_utils_tests::unordered_remove_tests()::obj::operator=(const
selftests::vector_utils_tests::\
unordered_remove_tests()::obj&)'
   41 |     obj &operator= (const obj &other)
      |          ^~~~~~~~
cc1plus: all warnings being treated as errors
make[1]: *** [Makefile:1647: unittests/vec-utils-selftests.o] Error 1
...
  
Simon Marchi Nov. 14, 2019, 2:57 a.m. UTC | #3
On 2019-11-12 9:38 a.m., Tom de Vries wrote:
> On 09-11-2019 01:36, Andrew Burgess (Code Review) wrote:
>> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/601
>> ......................................................................
>>
>> gdb_vecs.h: Avoid self move assign
>>
>> While working on another patch I ran into an issue with
>> unordered_remove (in gdb_vecs.h), where removing the last item of the
>> vector can cause a self move assign.
>>
>> When compiling the C++ standard library in debug mode (with
>> -D_GLIBCXX_DEBUG=1) this causes an error to trigger.
>>
>> I've fixed the issue in this patch and provided a unit test.
>>
>> The provided unit test includes an assignment operator which checks
>> for self move assign, this removes the need to compile with
>> -D_GLIBCXX_DEBUG=1 in order to spot the bug.  If you're keen to see
>> the error reported from the C++ standard library then remove operator=
>> from the unit test and recompile GDB with -D_GLIBCXX_DEBUG=1.
>>
>> gdb/ChangeLog:
>>
>> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add new file to the list.
>> 	* unittests/vec-utils-selftests.c: New file.
>> 	* gdbsupport/gdb_vecs.h (unordered_remove): Avoid self move assign.
>>
>> Change-Id: I80247b20cd5212038117db7412865f5e6a9257cd
>> ---
>> M gdb/ChangeLog
>> M gdb/Makefile.in
>> M gdb/gdbsupport/gdb_vecs.h
>> A gdb/unittests/vec-utils-selftests.c
>> 4 files changed, 75 insertions(+), 1 deletion(-)
> 
> Hi,
> 
> this breaks my build with gcc-9:
> ...
> In file included from
> /data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:23:
> /data/gdb_versions/devel/src/gdb/gdbsupport/gdb_vecs.h: In instantiation
> of 'T unordered_remove(std::vector<T>&, \
> typename std::vector<T>::iterator) [with T =
> selftests::vector_utils_tests::unordered_remove_tests()::obj; typena\
> me std::vector<T>::iterator =
> __gnu_cxx::__normal_iterator<selftests::vector_utils_tests::unordered_remove_tests(\
> )::obj*,
> std::vector<selftests::vector_utils_tests::unordered_remove_tests()::obj> >]':
> /data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:53:26:
>   required from here
> /data/gdb_versions/devel/src/gdb/gdbsupport/gdb_vecs.h:53:5: error:
> implicitly-declared 'selftests::vector_utils_\
> tests::unordered_remove_tests()::obj::obj(const
> selftests::vector_utils_tests::unordered_remove_tests()::obj&)' i\
> s deprecated [-Werror=deprecated-copy]
>    53 |   T removed = std::move (*it);
>       |     ^~~~~~~
> /data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:41:10:
> note: because 'selftests::vector_utils_te\
> sts::unordered_remove_tests()::obj' has user-provided
> 'selftests::vector_utils_tests::unordered_remove_tests()::o\
> bj&
> selftests::vector_utils_tests::unordered_remove_tests()::obj::operator=(const
> selftests::vector_utils_tests::\
> unordered_remove_tests()::obj&)'
>    41 |     obj &operator= (const obj &other)
>       |          ^~~~~~~~
> In file included from
> /data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:23:
> /data/gdb_versions/devel/src/gdb/gdbsupport/gdb_vecs.h:58:10: error:
> implicitly-declared 'selftests::vector_utils\
> _tests::unordered_remove_tests()::obj::obj(const
> selftests::vector_utils_tests::unordered_remove_tests()::obj&)' \
> is deprecated [-Werror=deprecated-copy]
>    58 |   return removed;
>       |          ^~~~~~~
> /data/gdb_versions/devel/src/gdb/unittests/vec-utils-selftests.c:41:10:
> note: because 'selftests::vector_utils_te\
> sts::unordered_remove_tests()::obj' has user-provided
> 'selftests::vector_utils_tests::unordered_remove_tests()::o\
> bj&
> selftests::vector_utils_tests::unordered_remove_tests()::obj::operator=(const
> selftests::vector_utils_tests::\
> unordered_remove_tests()::obj&)'
>    41 |     obj &operator= (const obj &other)
>       |          ^~~~~~~~
> cc1plus: all warnings being treated as errors
> make[1]: *** [Makefile:1647: unittests/vec-utils-selftests.o] Error 1
> ...
> 

I got that too with my recent-ish gcc.  Proposed patch here:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/626

Simon
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f5c8a76..7c5634c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2019-11-09  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add new file to the list.
+	* unittests/vec-utils-selftests.c: New file.
+	* gdbsupport/gdb_vecs.h (unordered_remove): Avoid self move assign.
+
 2019-11-08  Tom Tromey  <tromey@adacore.com>
 
 	* top.c (read_command_file): Update.
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 4f431c3..9ca77f6 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -440,6 +440,7 @@ 
 	unittests/tracepoint-selftests.c \
 	unittests/unpack-selftests.c \
 	unittests/utils-selftests.c \
+	unittests/vec-utils-selftests.c \
 	unittests/xml-utils-selftests.c
 
 SUBDIR_UNITTESTS_OBS = $(patsubst %.c,%.o,$(SUBDIR_UNITTESTS_SRCS))
diff --git a/gdb/gdbsupport/gdb_vecs.h b/gdb/gdbsupport/gdb_vecs.h
index e87ebe2..e8af624 100644
--- a/gdb/gdbsupport/gdb_vecs.h
+++ b/gdb/gdbsupport/gdb_vecs.h
@@ -51,7 +51,8 @@ 
   gdb_assert (it >= vec.begin () && it < vec.end ());
 
   T removed = std::move (*it);
-  *it = std::move (vec.back ());
+  if (it != vec.end () - 1)
+    *it = std::move (vec.back ());
   vec.pop_back ();
 
   return removed;
diff --git a/gdb/unittests/vec-utils-selftests.c b/gdb/unittests/vec-utils-selftests.c
new file mode 100644
index 0000000..823bbb6
--- /dev/null
+++ b/gdb/unittests/vec-utils-selftests.c
@@ -0,0 +1,66 @@ 
+/* Self tests for vector utility routines for GDB, the GNU debugger.
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program 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 of the License, or
+   (at your option) any later version.
+
+   This program 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 program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "gdbsupport/common-defs.h"
+#include "gdbsupport/selftest.h"
+
+#include "gdbsupport/gdb_vecs.h"
+
+namespace selftests {
+namespace vector_utils_tests {
+
+static void
+unordered_remove_tests ()
+{
+  /* Create vector with a single object in, and then remove that object.
+     This test was added after a bug was discovered where unordered_remove
+     would cause a self move assign.  If the C++ standard library is
+     compiled in debug mode (by passing -D_GLIBCXX_DEBUG=1) and the
+     operator= is removed from struct obj this test used to fail with an
+     error from the C++ standard library.  */
+  struct obj
+  {
+    std::vector<void *> var;
+
+    obj &operator= (const obj &other)
+    {
+      if (this == &other)
+	error (_("detected self move assign"));
+      this->var = other.var;
+      return *this;
+    }
+  };
+
+  std::vector<obj> v;
+  v.emplace_back ();
+  auto it = v.end () - 1;
+  unordered_remove (v, it);
+  SELF_CHECK (v.empty ());
+}
+
+} /* namespace vector_utils_tests */
+} /* amespace selftests */
+
+void
+_initialize_vec_utils_selftests ()
+{
+  selftests::register_test
+    ("unordered_remove",
+     selftests::vector_utils_tests::unordered_remove_tests);
+}