[review] gdb_vecs.h: Avoid self move assign

Message ID gerrit.1573259780000.I80247b20cd5212038117db7412865f5e6a9257cd@gnutoolchain-gerrit.osci.io
State Superseded
Headers show
Series
  • [review] gdb_vecs.h: Avoid self move assign
Related show

Commit Message

Sourceware to Gerrit sync (Code Review) Nov. 9, 2019, 12:36 a.m.
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(-)




-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I80247b20cd5212038117db7412865f5e6a9257cd
Gerrit-Change-Number: 601
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: newchange

Comments

Sourceware to Gerrit sync (Code Review) Nov. 9, 2019, 5:03 p.m. | #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.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I80247b20cd5212038117db7412865f5e6a9257cd
Gerrit-Change-Number: 601
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Sat, 09 Nov 2019 17:03:46 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Tom de Vries Nov. 12, 2019, 2:38 p.m. | #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. | #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);
+}