Bug 97207 - [nvptx, build] nvptx.c:3539:38: error: no matching function for call to ‘swap(bracket_vec_t&, bracket_vec_t&)’
Summary: [nvptx, build] nvptx.c:3539:38: error: no matching function for call to ‘swap...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-25 11:22 UTC by Tom de Vries
Modified: 2020-09-25 12:52 UTC (History)
2 users (show)

See Also:
Host:
Target: nvptx
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
gzipped preprocessed source (426.40 KB, application/gzip)
2020-09-25 11:38 UTC, Tom de Vries
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2020-09-25 11:22:56 UTC
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>]’:
...
Comment 1 Tom de Vries 2020-09-25 11:25:46 UTC
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.
Comment 2 Tom de Vries 2020-09-25 11:29:35 UTC
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
...
Comment 3 Tom de Vries 2020-09-25 11:38:20 UTC
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);
                                      ^
...
Comment 4 Richard Biener 2020-09-25 11:40:44 UTC
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?
Comment 5 Richard Biener 2020-09-25 11:42:55 UTC
Maybe try implementing the deleted

  void operator= (auto_vec&&) = delete;

but the diagnostic isn't really pointing to that ...
Comment 6 Richard Biener 2020-09-25 11:47:30 UTC
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?
Comment 7 Jonathan Wakely 2020-09-25 11:52:07 UTC
(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.
Comment 8 rguenther@suse.de 2020-09-25 11:55:50 UTC
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)
Comment 9 Richard Biener 2020-09-25 11:57:08 UTC
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?
Comment 10 Jonathan Wakely 2020-09-25 11:57:40 UTC
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)
Comment 11 Jonathan Wakely 2020-09-25 11:58:37 UTC
(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.
Comment 12 Jonathan Wakely 2020-09-25 12:00:03 UTC
(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.
Comment 13 Jonathan Wakely 2020-09-25 12:03:31 UTC
(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
Comment 14 Richard Biener 2020-09-25 12:08:04 UTC
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;
+    }
 };
Comment 15 Tom de Vries 2020-09-25 12:09:22 UTC
(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.
Comment 16 GCC Commits 2020-09-25 12:52:11 UTC
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.
Comment 17 Richard Biener 2020-09-25 12:52:36 UTC
Fixed then.