[review] gdb_vecs.h: Avoid self move assign
Commit Message
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
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.
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
...
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
@@ -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.
@@ -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))
@@ -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;
new file mode 100644
@@ -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);
+}