Building trunk for nvptx on ubuntu 18.04.5 LTS with g++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0, I run into: ... src/gcc/config/nvptx/nvptx.c: In member function ‘void bb_sese: :append(bb_sese*)’: src/gcc/config/nvptx/nvptx.c:3539:38: error: no matching functi on for call to ‘swap(bracket_vec_t&, bracket_vec_t&)’ std::swap (brackets, child->brackets); ^ In file included from /usr/include/c++/7/bits/nested_exception.h:40:0, from /usr/include/c++/7/exception:143, from /usr/include/c++/7/ios:39, from /usr/include/c++/7/istream:38, from /usr/include/c++/7/sstream:38, from src/gcc/config/nvptx/nvptx.c:24: /usr/include/c++/7/bits/move.h:187:5: note: candidate: template<class _Tp> typename std::enabl e_if<std::__and_<std::__not_<std::__is_tuple_like<_Tp> >, std::is_move_constructible<_Tp>, std ::is_move_assignable<_Tp> >::value>::type std::swap(_Tp&, _Tp&) swap(_Tp& __a, _Tp& __b) ^~~~ /usr/include/c++/7/bits/move.h:187:5: note: template argument deduction/substitution failed: /usr/include/c++/7/bits/move.h: In substitution of ‘template<class _Tp> typename std::enable_i f<std::__and_<std::__not_<std::__is_tuple_like<_Tp> >, std::is_move_constructible<_Tp>, std::i s_move_assignable<_Tp> >::value>::type std::swap(_Tp&, _Tp&) [with _Tp = auto_vec<bracket>]’: ...
Regression, started at: ... commit 4b9d61f79c0c0185a33048ae6cc72269cf7efa31 Author: Richard Biener <rguenther@suse.de> Date: Thu Aug 6 14:50:56 2020 +0200 add move CTOR to auto_vec, use auto_vec for get_loop_exit_edges This adds a move CTOR to auto_vec<T, 0> and makes use of a auto_vec<edge> return value for get_loop_exit_edges denoting that lifetime management of the vector is handed to the caller. The move CTOR prompted the hash_table change because it appearantly makes the copy CTOR implicitely deleted (good) and hash-table expansion of the odr_enum_map which is hash_map <nofree_string_hash, odr_enum> where odr_enum has an auto_vec<odr_enum_val, 0> member triggers this. Not sure if there's a latent bug there before this (I think we're not invoking DTORs, but we're invoking copy-CTORs). ... The type bracket_vec_t is defined as: ... typedef auto_vec<bracket> bracket_vec_t; ... so that does look at least related.
Configure from build-gcc/config.log: ... $ /home/vries/nvptx/trunk/source-gcc/configure --target=nvptx-none --prefix= --enable-languages=c,c++,fortran --enable-werror --enable-checking=yes CC=gcc -m64 -Wl,-rpath,/i686-pc-linux-gnu/lib64 CXX=g++ -m64 -Wl,-rpath,/i686-pc-linux-gnu/lib64 --enable-linker-plugin-flags=CC=gcc\ -m32\ -Wl,-rpath,/i686-pc-linux-gnu/lib --enable-linker-plugin-configure-flags=--host=i686-pc-linux-gnu --with-sysroot=/nvptx-none --with-build-sysroot=/home/vries/nvptx/trunk/install/nvptx-none --with-build-time-tools=/home/vries/nvptx/trunk/install/nvptx-none/bin --disable-sjlj-exceptions --enable-newlib-io-long-long CFLAGS=-O0 -g CXXFLAGS=-O0 -g ...
Created attachment 49271 [details] gzipped preprocessed source Reproduce: .... $ g++ -m64 -fno-PIE -c -O0 -g -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -fpreprocessed nvptx.c -Wno-implicit-fallthrough nvptx.c: In member function ‘void bb_sese::append(bb_sese*)’: /home/vries/nvptx/trunk/source-gcc/gcc/config/nvptx/nvptx.c:3539:38: error: no matching function for call to ‘swap(bracket_vec_t&, bracket_vec_t&)’ std::swap (brackets, child->brackets); ^ ...
That swap couldn't have worked before because auto_vec eventually contains a pointer into itself. So the patch has improved things from broken to rejected. Rejected because while there's a move CTOR now, std::swap isn't happy with just that. No idea why - Jonathan?
Maybe try implementing the deleted void operator= (auto_vec&&) = delete; but the diagnostic isn't really pointing to that ...
OK, so diff --git a/gcc/vec.c b/gcc/vec.c index a28899170ed..0cda3b96beb 100644 --- a/gcc/vec.c +++ b/gcc/vec.c @@ -560,6 +560,9 @@ vec_c_tests () test_qsort (); test_reverse (); test_auto_delete_vec (); + auto_vec<int> a; + auto_vec<int> b; + std::swap (a, b); } } // namespace selftest produces a similar error: /home/rguenther/src/gcc3/gcc/vec.c: In function 'void selftest::vec_c_tests()': /home/rguenther/src/gcc3/gcc/vec.c:565:18: error: no matching function for call to 'swap(auto_vec<int>&, auto_vec<int>&)' std::swap (a, b); ^ In file included from /usr/include/c++/7/bits/nested_exception.h:40:0, from /usr/include/c++/7/exception:143, from /usr/include/c++/7/new:40, from /home/rguenther/src/gcc3/gcc/system.h:236, from /home/rguenther/src/gcc3/gcc/vec.c:30: /usr/include/c++/7/bits/move.h:187:5: note: candidate: template<class _Tp> typename std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<_Tp> >, std::is_move_constructible<_Tp>, std::is_move_assignable<_Tp> >::value>::type std::swap(_Tp&, _Tp&) swap(_Tp& __a, _Tp& __b) ^~~~ /usr/include/c++/7/bits/move.h:187:5: note: template argument deduction/substitution failed: /usr/include/c++/7/bits/move.h: In substitution of 'template<class _Tp> typename std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<_Tp> >, std::is_move_constructible<_Tp>, std::is_move_assignable<_Tp> >::value>::type std::swap(_Tp&, _Tp&) [with _Tp = auto_vec<int>]': /home/rguenther/src/gcc3/gcc/vec.c:565:18: required from here /usr/include/c++/7/bits/move.h:187:5: error: no type named 'type' in 'struct std::enable_if<false, void>' /usr/include/c++/7/bits/move.h:210:5: note: candidate: template<class _Tp, long unsigned int _Nm> typename std::enable_if<std::__is_swappable<_Tp>::value>::type std::swap(_Tp (&)[_Nm], _Tp (&)[_Nm]) swap(_Tp (&__a)[_Nm], _Tp (&__b)[_Nm]) ^~~~ /usr/include/c++/7/bits/move.h:210:5: note: template argument deduction/substitution failed: /home/rguenther/src/gcc3/gcc/vec.c:565:18: note: mismatched types '_Tp [_Nm]' and 'auto_vec<int>' std::swap (a, b); ^ In file included from /usr/include/c++/7/utility:70:0, from /home/rguenther/src/gcc3/gcc/system.h:237, from /home/rguenther/src/gcc3/gcc/vec.c:30: /usr/include/c++/7/bits/stl_pair.h:495:5: note: candidate: template<class _T1, class _T2> typename std::enable_if<std::__and_<std::__is_swappable<_T1>, std::__is_swappable<_T2> >::value>::type std::swap(std::pair<_T1, _T2>&, std::pair<_T1, _T2>&) swap(pair<_T1, _T2>& __x, pair<_T1, _T2>& __y) ^~~~ /usr/include/c++/7/bits/stl_pair.h:495:5: note: template argument deduction/substitution failed: /home/rguenther/src/gcc3/gcc/vec.c:565:18: note: 'auto_vec<int>' is not derived from 'std::pair<_T1, _T2>' std::swap (a, b); ^ In file included from /usr/include/c++/7/utility:70:0, from /home/rguenther/src/gcc3/gcc/system.h:237, from /home/rguenther/src/gcc3/gcc/vec.c:30: /usr/include/c++/7/bits/stl_pair.h:503:5: note: candidate: template<class _T1, class _T2> typename std::enable_if<(! std::__and_<std::__is_swappable<_T1>, std::__is_swappable<_T2> >::value)>::type std::swap(std::pair<_T1, _T2>&, std::pair<_T1, _T2>&) <deleted> swap(pair<_T1, _T2>&, pair<_T1, _T2>&) = delete; ^~~~ /usr/include/c++/7/bits/stl_pair.h:503:5: note: template argument deduction/substitution failed: /home/rguenther/src/gcc3/gcc/vec.c:565:18: note: 'auto_vec<int>' is not derived from 'std::pair<_T1, _T2>' std::swap (a, b); ^ make: *** [Makefile:1123: vec.o] Error 1 IMHO looks like a libstdc++ bug to me?
(In reply to Richard Biener from comment #4) > That swap couldn't have worked before because auto_vec eventually contains a > pointer into itself. So the patch has improved things from broken to > rejected. Rejected because while there's a move CTOR now, std::swap isn't > happy with just that. > > No idea why - Jonathan? Because swap requires assignment, not just construction. I don't see a libstdc++ bug here.
On Fri, 25 Sep 2020, redi at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97207 > > --- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> --- > (In reply to Richard Biener from comment #4) > > That swap couldn't have worked before because auto_vec eventually contains a > > pointer into itself. So the patch has improved things from broken to > > rejected. Rejected because while there's a move CTOR now, std::swap isn't > > happy with just that. > > > > No idea why - Jonathan? > > Because swap requires assignment, not just construction. > > I don't see a libstdc++ bug here. But there is assignment? Just no move assignment? I guess swap is really special because clearly formerly { auto_vec<int> a, b; a.safe_push (1); b = a; } would have caused a double-free. std::swap fixes this because, well, it swaps. Going to try implement move assignment then. Just wonder why when move assingment is not there it doesn't fall back to non-move assignment semantics (aka previous behavior)
diff --git a/gcc/vec.h b/gcc/vec.h index d73d865cff2..c0e577893a3 100644 --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1546,7 +1546,12 @@ public: this->m_vec = r.m_vec; r.m_vec = NULL; } - void operator= (auto_vec&&) = delete; + void operator= (auto_vec&& r) + { + this->release (); + this->m_vec = r.m_vec; + r.m_vec = NULL; + } }; works for the vec.c test, Tom - can you check if it works for nvptx?
If you don't want to support assignment you can still support swapping: --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1546,9 +1546,21 @@ public: this->m_vec = r.m_vec; r.m_vec = NULL; } + void operator= (auto_vec&&) = delete; + + void swap(auto_vec&& r) + { + std::swap(this->m_vec = r.m_vec); + } }; +template<typename T> +void swap(auto_vec<T>& l, auto_vec<T>& r) +{ + l.swap(r); +} + /* Allocate heap memory for pointer V and create the internal vector with space for NELEMS elements. If NELEMS is 0, the internal Then use swap(v1, v2) not std::swap(v1, v2)
(In reply to rguenther@suse.de from comment #8) > On Fri, 25 Sep 2020, redi at gcc dot gnu.org wrote: > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97207 > > > > --- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> --- > > (In reply to Richard Biener from comment #4) > > > That swap couldn't have worked before because auto_vec eventually contains a > > > pointer into itself. So the patch has improved things from broken to > > > rejected. Rejected because while there's a move CTOR now, std::swap isn't > > > happy with just that. > > > > > > No idea why - Jonathan? > > > > Because swap requires assignment, not just construction. > > > > I don't see a libstdc++ bug here. > > But there is assignment? Just no move assignment? No there isn't. You deleted the move assignment operator, so there's no assignment at all. Swap uses move assignment anyway. > I guess swap > is really special because clearly formerly > > { > auto_vec<int> a, b; > a.safe_push (1); > b = a; > } > > would have caused a double-free. std::swap fixes this because, well, > it swaps. Going to try implement move assignment then. > > Just wonder why when move assingment is not there it doesn't > fall back to non-move assignment semantics (aka previous behavior) Because you deleted it, that says you don't want assignment.
(In reply to Richard Biener from comment #9) > diff --git a/gcc/vec.h b/gcc/vec.h > index d73d865cff2..c0e577893a3 100644 > --- a/gcc/vec.h > +++ b/gcc/vec.h > @@ -1546,7 +1546,12 @@ public: > this->m_vec = r.m_vec; > r.m_vec = NULL; > } > - void operator= (auto_vec&&) = delete; > + void operator= (auto_vec&& r) > + { > + this->release (); > + this->m_vec = r.m_vec; > + r.m_vec = NULL; > + } > }; > > > > works for the vec.c test, Tom - can you check if it works for nvptx? Assignment operators should return a reference to *this, not void. Don't break conventions without good reason. I suggest adding move assignment *and* swap. Then swap(v1, v2) can be more efficient than the generic std::swap(v1, v2) by swapping directly, instead of move-constructing a temporary and then assigning twice.
(In reply to Jonathan Wakely from comment #10) > If you don't want to support assignment you can still support swapping: > > --- a/gcc/vec.h > +++ b/gcc/vec.h > @@ -1546,9 +1546,21 @@ public: > this->m_vec = r.m_vec; > r.m_vec = NULL; > } > + > void operator= (auto_vec&&) = delete; > + > + void swap(auto_vec&& r) > + { > + std::swap(this->m_vec = r.m_vec); Oops, copy&paste error, that should be: std::swap(this->m_vec, r.m_vec); To add move assignment and swap: --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1546,9 +1546,26 @@ public: this->m_vec = r.m_vec; r.m_vec = NULL; } - void operator= (auto_vec&&) = delete; + + auto_vec& operator= (auto_vec&& r) + { + this->release(); + this->swap(r); + return *this; + } + + void swap(auto_vec&& r) + { + std::swap(this->m_vec, r.m_vec); + } }; +template<typename T> +void swap(auto_vec<T>& l, auto_vec<T>& r) +{ + l.swap(r); +} + /* Allocate heap memory for pointer V and create the internal vector with space for NELEMS elements. If NELEMS is 0, the internal
OK, adding explicit swap doesn't look worth the trouble (the object is just a single pointer). Might be useful if we ever support std::move for auto_vec<T, N> with N != 0. Which reminds me that still uses broken default implementations. I am testing the following, sorry for my limited C++ knowledge... (but seriously I didn't expect a std::swap(auto_vec) user...) diff --git a/gcc/vec.h b/gcc/vec.h index d73d865cff2..d8c7cdac073 100644 --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1546,7 +1546,13 @@ public: this->m_vec = r.m_vec; r.m_vec = NULL; } - void operator= (auto_vec&&) = delete; + auto_vec& operator= (auto_vec&& r) + { + this->release (); + this->m_vec = r.m_vec; + r.m_vec = NULL; + return *this; + } };
(In reply to Richard Biener from comment #9) > diff --git a/gcc/vec.h b/gcc/vec.h > index d73d865cff2..c0e577893a3 100644 > --- a/gcc/vec.h > +++ b/gcc/vec.h > @@ -1546,7 +1546,12 @@ public: > this->m_vec = r.m_vec; > r.m_vec = NULL; > } > - void operator= (auto_vec&&) = delete; > + void operator= (auto_vec&& r) > + { > + this->release (); > + this->m_vec = r.m_vec; > + r.m_vec = NULL; > + } > }; > > > > works for the vec.c test, Tom - can you check if it works for nvptx? It does.
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>: https://gcc.gnu.org/g:7bfc4cd2c812a3197c09797796828459714f8849 commit r11-3459-g7bfc4cd2c812a3197c09797796828459714f8849 Author: Richard Biener <rguenther@suse.de> Date: Fri Sep 25 13:59:15 2020 +0200 middle-end/97207 - implement move assign for auto_vec<> This implements the missing move assignment to make std::swap work on auto_vec<> 2020-09-25 Richard Biener <rguenther@suse.de> PR middle-end/97207 * vec.h (auto_vec<T>::operator=(auto_vec<T>&&)): Implement.
Fixed then.