[Patch] Fix variant::operator= on references

[Patch] Fix variant::operator= on references

[Tim Shen]

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

Hi, this patch fixes the following compilation failure:

#include <variant>

int main()
{
   float f1 = 1.0f, f2 = 2.0f;

   std::variant<float&> v1(f1);

   v1 = f2; // #1
}

The bug is caused by a misuse of __storage. I also examined other
__storage usage, they all seem appropriate.

Tested on x86_64-linux-gnu.

Thanks!

-- 
Regards,
Tim Shen

[-- Attachment #2: a.diff --]
[-- Type: application/octet-stream, Size: 1444 bytes --]

commit aa8e2a5a1bb9e615e14fdc8417a60fda5de222a3
Author: Tim Shen <timshen@google.com>
Date:   Thu Sep 22 00:35:13 2016 -0700

    2016-09-22  Tim Shen  <timshen@google.com>
    
    	* libstdc++-v3/include/std/variant (variant::operator=): Fix assignment
    	on references.
    	* libstdc++-v3/testsuite/20_util/variant/compile.cc: Add test.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 013884b..911dab3 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -1147,8 +1147,7 @@ namespace __variant
 	{
 	  constexpr auto __index = __accepted_index<_Tp&&>;
 	  if (index() == __index)
-	    *static_cast<__storage<__to_type<__index>>*>(this->_M_storage())
-	      = forward<_Tp>(__rhs);
+	    std::get<__index>(*this) = forward<_Tp>(__rhs);
 	  else
 	    this->emplace<__index>(forward<_Tp>(__rhs));
 	  __glibcxx_assert(holds_alternative<__accepted_type<_Tp&&>>(*this));
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index 99f980a..a0a8d70 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -169,6 +169,12 @@ void copy_assign()
     variant<DefaultNoexcept> a;
     static_assert(!noexcept(a = a), "");
   }
+
+  {
+    float f1 = 1.0f, f2 = 2.0f;
+    std::variant<float&> v1(f1);
+    v1 = f2;
+  }
 }
 
 void move_assign()

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 12:43 AM, Tim Shen <timshen@google.com> wrote:
> Hi, this patch fixes the following compilation failure:

For the record, the bug is found by Ville. Thank you Ville! :)


-- 
Regards,
Tim Shen

Re: [Patch] Fix variant::operator= on references

[Jonathan Wakely]

On 22/09/16 00:43 -0700, Tim Shen wrote:
>Hi, this patch fixes the following compilation failure:
>
>#include <variant>
>
>int main()
>{
>   float f1 = 1.0f, f2 = 2.0f;
>
>   std::variant<float&> v1(f1);
>
>   v1 = f2; // #1
>}
>
>The bug is caused by a misuse of __storage. I also examined other
>__storage usage, they all seem appropriate.

@@ -1147,8 +1147,7 @@ namespace __variant
        {
          constexpr auto __index = __accepted_index<_Tp&&>;
          if (index() == __index)
-           *static_cast<__storage<__to_type<__index>>*>(this->_M_storage())
-             = forward<_Tp>(__rhs);
+           std::get<__index>(*this) = forward<_Tp>(__rhs);

Please qualify std::forward here.

OK for trunk with that change, thanks for the quick fix.

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 1:39 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> Please qualify std::forward here.

Done. When writing the initial version, I was trying to save as much
qualifications as possible (as long as the semantic doesn't change)
for readability, but that might not be a good idea.

>
> OK for trunk with that change, thanks for the quick fix.
>
>

Committed. Thanks!


-- 
Regards,
Tim Shen

Re: [Patch] Fix variant::operator= on references

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On 22/09/16 01:49 -0700, Tim Shen wrote:
>Done. When writing the initial version, I was trying to save as much
>qualifications as possible (as long as the semantic doesn't change)
>for readability, but that might not be a good idea.

It does change the semantics, as forward<_Tp>(__tp) can find another
function via ADL (see the new test in this patch).

Tested powerpc64le-linux, committed to trunk.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 12543 bytes --]

commit fe8a92068c783bd2a911c1864c62ffd8c3f31ea1
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Sep 22 10:45:50 2016 +0100

    Always qualify std::forward in <variant>
    
    	* include/bits/uses_allocator.h (__uses_allocator_construct): Qualify
    	std::forward and ::new. Cast pointer to void*.
    	* include/std/variant (_Variant_storage, _Union, _Variant_base)
    	(__access, __visit_invoke, variant, visit): Qualify std::forward.
    	* testsuite/20_util/variant/compile.cc: Test for ADL problems.

diff --git a/libstdc++-v3/include/bits/uses_allocator.h b/libstdc++-v3/include/bits/uses_allocator.h
index 500bd90..c7d14f3 100644
--- a/libstdc++-v3/include/bits/uses_allocator.h
+++ b/libstdc++-v3/include/bits/uses_allocator.h
@@ -144,24 +144,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Tp, typename... _Args>
     void __uses_allocator_construct_impl(__uses_alloc0 __a, _Tp* __ptr,
 					 _Args&&... __args)
-    { new (__ptr) _Tp(forward<_Args>(__args)...); }
+    { ::new ((void*)__ptr) _Tp(std::forward<_Args>(__args)...); }
 
   template<typename _Tp, typename _Alloc, typename... _Args>
     void __uses_allocator_construct_impl(__uses_alloc1<_Alloc> __a, _Tp* __ptr,
 					 _Args&&... __args)
-    { new (__ptr) _Tp(allocator_arg, *__a._M_a, forward<_Args>(__args)...); }
+    {
+      ::new ((void*)__ptr) _Tp(allocator_arg, *__a._M_a,
+			       std::forward<_Args>(__args)...);
+    }
 
   template<typename _Tp, typename _Alloc, typename... _Args>
     void __uses_allocator_construct_impl(__uses_alloc2<_Alloc> __a, _Tp* __ptr,
 					 _Args&&... __args)
-    { new (__ptr) _Tp(forward<_Args>(__args)..., *__a._M_a); }
+    { ::new ((void*)__ptr) _Tp(std::forward<_Args>(__args)..., *__a._M_a); }
 
   template<typename _Tp, typename _Alloc, typename... _Args>
     void __uses_allocator_construct(const _Alloc& __a, _Tp* __ptr,
 				    _Args&&... __args)
     {
       __uses_allocator_construct_impl(__use_alloc<_Tp, _Alloc, _Args...>(__a),
-				      __ptr, forward<_Args>(__args)...);
+				      __ptr, std::forward<_Args>(__args)...);
     }
 
 _GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 1ad33fc..ac483f3 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -298,7 +298,7 @@ namespace __variant
 
       template<size_t _Np, typename... _Args>
 	constexpr _Variant_storage(in_place_index_t<_Np>, _Args&&... __args)
-	: _M_union(in_place<_Np>, forward<_Args>(__args)...)
+	: _M_union(in_place<_Np>, std::forward<_Args>(__args)...)
 	{ }
 
       ~_Variant_storage() = default;
@@ -316,13 +316,13 @@ namespace __variant
 
 	template<typename... _Args>
 	  constexpr _Union(in_place_index_t<0>, _Args&&... __args)
-	  : _M_first(in_place<0>, forward<_Args>(__args)...)
+	  : _M_first(in_place<0>, std::forward<_Args>(__args)...)
 	  { }
 
 	template<size_t _Np, typename... _Args,
 		 typename = enable_if_t<0 < _Np && _Np < sizeof...(_Rest) + 1>>
 	  constexpr _Union(in_place_index_t<_Np>, _Args&&... __args)
-	  : _M_rest(in_place<_Np - 1>, forward<_Args>(__args)...)
+	  : _M_rest(in_place<_Np - 1>, std::forward<_Args>(__args)...)
 	  { }
 
 	_Uninitialized<__storage<_First>> _M_first;
@@ -386,7 +386,7 @@ namespace __variant
       template<size_t _Np, typename... _Args>
 	constexpr explicit
 	_Variant_base(in_place_index_t<_Np> __i, _Args&&... __args)
-	: _Storage(__i, forward<_Args>(__args)...), _M_index(_Np)
+	: _Storage(__i, std::forward<_Args>(__args)...), _M_index(_Np)
 	{ }
 
       template<typename _Alloc>
@@ -426,7 +426,7 @@ namespace __variant
 	  using _Storage =
 	    __storage<variant_alternative_t<_Np, variant<_Types...>>>;
 	  __uses_allocator_construct(__a, static_cast<_Storage*>(_M_storage()),
-				     forward<_Args>(__args)...);
+				     std::forward<_Args>(__args)...);
 	  __glibcxx_assert(_M_index == _Np);
 	}
 
@@ -581,7 +581,7 @@ namespace __variant
     decltype(auto) __access(_Variant&& __v)
     {
       return __get_alternative<__reserved_type_map<_Variant&&, __storage<_Tp>>>(
-	__get_storage(forward<_Variant>(__v)));
+	__get_storage(std::forward<_Variant>(__v)));
     }
 
   // A helper used to create variadic number of _To types.
@@ -591,10 +591,11 @@ namespace __variant
   // Call the actual visitor.
   // _Args are qualified storage types.
   template<typename _Visitor, typename... _Args>
-    decltype(auto) __visit_invoke(_Visitor&& __visitor,
-				  _To_type<_Args, void*>... __ptrs)
+    decltype(auto)
+    __visit_invoke(_Visitor&& __visitor, _To_type<_Args, void*>... __ptrs)
     {
-      return forward<_Visitor>(__visitor)(__get_alternative<_Args>(__ptrs)...);
+      return std::forward<_Visitor>(__visitor)(
+	  __get_alternative<_Args>(__ptrs)...);
     }
 
   // Used for storing multi-dimensional vtable.
@@ -1010,7 +1011,7 @@ namespace __variant
 	constexpr
 	variant(_Tp&& __t)
 	noexcept(is_nothrow_constructible_v<__accepted_type<_Tp&&>, _Tp&&>)
-	: variant(in_place<__accepted_index<_Tp&&>>, forward<_Tp>(__t))
+	: variant(in_place<__accepted_index<_Tp&&>>, std::forward<_Tp>(__t))
 	{ __glibcxx_assert(holds_alternative<__accepted_type<_Tp&&>>(*this)); }
 
       template<typename _Tp, typename... _Args,
@@ -1018,7 +1019,7 @@ namespace __variant
 			  && is_constructible_v<_Tp, _Args&&...>>>
 	constexpr explicit
 	variant(in_place_type_t<_Tp>, _Args&&... __args)
-	: variant(in_place<__index_of<_Tp>>, forward<_Args>(__args)...)
+	: variant(in_place<__index_of<_Tp>>, std::forward<_Args>(__args)...)
 	{ __glibcxx_assert(holds_alternative<_Tp>(*this)); }
 
       template<typename _Tp, typename _Up, typename... _Args,
@@ -1029,7 +1030,7 @@ namespace __variant
 	variant(in_place_type_t<_Tp>, initializer_list<_Up> __il,
 		_Args&&... __args)
 	: variant(in_place<__index_of<_Tp>>, __il,
-		  forward<_Args>(__args)...)
+		  std::forward<_Args>(__args)...)
 	{ __glibcxx_assert(holds_alternative<_Tp>(*this)); }
 
       template<size_t _Np, typename... _Args,
@@ -1037,7 +1038,7 @@ namespace __variant
 		 is_constructible_v<__to_type<_Np>, _Args&&...>>>
 	constexpr explicit
 	variant(in_place_index_t<_Np>, _Args&&... __args)
-	: _Base(in_place<_Np>, forward<_Args>(__args)...),
+	: _Base(in_place<_Np>, std::forward<_Args>(__args)...),
 	_Default_ctor_enabler(_Enable_default_constructor_tag{})
 	{ __glibcxx_assert(index() == _Np); }
 
@@ -1047,7 +1048,7 @@ namespace __variant
 	constexpr explicit
 	variant(in_place_index_t<_Np>, initializer_list<_Up> __il,
 		_Args&&... __args)
-	: _Base(in_place<_Np>, __il, forward<_Args>(__args)...),
+	: _Base(in_place<_Np>, __il, std::forward<_Args>(__args)...),
 	_Default_ctor_enabler(_Enable_default_constructor_tag{})
 	{ __glibcxx_assert(index() == _Np); }
 
@@ -1084,7 +1085,7 @@ namespace __variant
 		 && !is_same_v<decay_t<_Tp>, variant>, variant&>>
 	variant(allocator_arg_t, const _Alloc& __a, _Tp&& __t)
 	: variant(allocator_arg, __a, in_place<__accepted_index<_Tp&&>>,
-		  forward<_Tp>(__t))
+		  std::forward<_Tp>(__t))
 	{ __glibcxx_assert(holds_alternative<__accepted_type<_Tp&&>>(*this)); }
 
       template<typename _Alloc, typename _Tp, typename... _Args,
@@ -1095,7 +1096,7 @@ namespace __variant
 	variant(allocator_arg_t, const _Alloc& __a, in_place_type_t<_Tp>,
 		_Args&&... __args)
 	: variant(allocator_arg, __a, in_place<__index_of<_Tp>>,
-		  forward<_Args>(__args)...)
+		  std::forward<_Args>(__args)...)
 	{ __glibcxx_assert(holds_alternative<_Tp>(*this)); }
 
       template<typename _Alloc, typename _Tp, typename _Up, typename... _Args,
@@ -1106,7 +1107,7 @@ namespace __variant
 	variant(allocator_arg_t, const _Alloc& __a, in_place_type_t<_Tp>,
 		initializer_list<_Up> __il, _Args&&... __args)
 	: variant(allocator_arg, __a, in_place<__index_of<_Tp>>, __il,
-		  forward<_Args>(__args)...)
+		  std::forward<_Args>(__args)...)
 	{ __glibcxx_assert(holds_alternative<_Tp>(*this)); }
 
       template<typename _Alloc, size_t _Np, typename... _Args,
@@ -1115,7 +1116,7 @@ namespace __variant
 		   __to_type<_Np>, _Alloc, _Args&&...>>>
 	variant(allocator_arg_t, const _Alloc& __a, in_place_index_t<_Np>,
 		_Args&&... __args)
-	: _Base(__a, in_place<_Np>, forward<_Args>(__args)...),
+	: _Base(__a, in_place<_Np>, std::forward<_Args>(__args)...),
 	_Default_ctor_enabler(_Enable_default_constructor_tag{})
 	{ __glibcxx_assert(index() == _Np); }
 
@@ -1125,7 +1126,7 @@ namespace __variant
 		   __to_type<_Np>, _Alloc, initializer_list<_Up>&, _Args&&...>>>
 	variant(allocator_arg_t, const _Alloc& __a, in_place_index_t<_Np>,
 		initializer_list<_Up> __il, _Args&&... __args)
-	: _Base(__a, in_place<_Np>, __il, forward<_Args>(__args)...),
+	: _Base(__a, in_place<_Np>, __il, std::forward<_Args>(__args)...),
 	_Default_ctor_enabler(_Enable_default_constructor_tag{})
 	{ __glibcxx_assert(index() == _Np); }
 
@@ -1149,7 +1150,7 @@ namespace __variant
 	  if (index() == __index)
 	    std::get<__index>(*this) = std::forward<_Tp>(__rhs);
 	  else
-	    this->emplace<__index>(forward<_Tp>(__rhs));
+	    this->emplace<__index>(std::forward<_Tp>(__rhs));
 	  __glibcxx_assert(holds_alternative<__accepted_type<_Tp&&>>(*this));
 	  return *this;
 	}
@@ -1159,7 +1160,7 @@ namespace __variant
 	{
 	  static_assert(__exactly_once<_Tp>,
 			"T should occur for exactly once in alternatives");
-	  this->emplace<__index_of<_Tp>>(forward<_Args>(__args)...);
+	  this->emplace<__index_of<_Tp>>(std::forward<_Args>(__args)...);
 	  __glibcxx_assert(holds_alternative<_Tp>(*this));
 	}
 
@@ -1168,7 +1169,7 @@ namespace __variant
 	{
 	  static_assert(__exactly_once<_Tp>,
 			"T should occur for exactly once in alternatives");
-	  this->emplace<__index_of<_Tp>>(__il, forward<_Args>(__args)...);
+	  this->emplace<__index_of<_Tp>>(__il, std::forward<_Args>(__args)...);
 	  __glibcxx_assert(holds_alternative<_Tp>(*this));
 	}
 
@@ -1181,7 +1182,7 @@ namespace __variant
 	  __try
 	    {
 	      ::new (this) variant(in_place<_Np>,
-				   forward<_Args>(__args)...);
+				   std::forward<_Args>(__args)...);
 	    }
 	  __catch (...)
 	    {
@@ -1200,7 +1201,7 @@ namespace __variant
 	  __try
 	    {
 	      ::new (this) variant(in_place<_Np>, __il,
-				   forward<_Args>(__args)...);
+				   std::forward<_Args>(__args)...);
 	    }
 	  __catch (...)
 	    {
@@ -1310,12 +1311,12 @@ namespace __variant
     visit(_Visitor&& __visitor, _Variants&&... __variants)
     {
       using _Result_type =
-	decltype(forward<_Visitor>(__visitor)(get<0>(__variants)...));
+	decltype(std::forward<_Visitor>(__visitor)(get<0>(__variants)...));
       static constexpr auto _S_vtable =
 	__detail::__variant::__gen_vtable<
 	  _Result_type, _Visitor&&, _Variants&&...>::_S_apply();
       auto __func_ptr = _S_vtable._M_access(__variants.index()...);
-      return (*__func_ptr)(forward<_Visitor>(__visitor),
+      return (*__func_ptr)(std::forward<_Visitor>(__visitor),
 			   __detail::__variant::__get_storage(__variants)...);
     }
 
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index a0a8d70..85a697f 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -418,3 +418,50 @@ void test_pr77641()
 
   constexpr std::variant<X> v1 = X{};
 }
+
+namespace adl_trap
+{
+  struct X {
+    X() = default;
+    X(int) { }
+    X(std::initializer_list<int>, const X&) { }
+  };
+  template<typename T> void move(T&) { }
+  template<typename T> void forward(T&) { }
+
+  struct Visitor {
+    template<typename T> void operator()(T&&) { }
+  };
+}
+
+void test_adl()
+{
+   using adl_trap::X;
+   using std::allocator_arg;
+   X x;
+   std::allocator<int> a;
+   std::initializer_list<int> il;
+   adl_trap::Visitor vis;
+
+   std::variant<X> v0(x);
+   v0 = x;
+   v0.emplace<0>(x);
+   v0.emplace<0>(il, x);
+   visit(vis, v0);
+   variant<X> v1{in_place<0>, x};
+   variant<X> v2{in_place<X>, x};
+   variant<X> v3{in_place<0>, il, x};
+   variant<X> v4{in_place<X>, il, x};
+   variant<X> v5{allocator_arg, a, in_place<0>, x};
+   variant<X> v6{allocator_arg, a, in_place<X>, x};
+   variant<X> v7{allocator_arg, a, in_place<0>, il, x};
+   variant<X> v8{allocator_arg, a, in_place<X>, il, x};
+   variant<X> v9{allocator_arg, a, in_place<X>, 1};
+
+   std::variant<X&> vr0(x);
+   vr0 = x;
+   variant<X&> vr1{in_place<0>, x};
+   variant<X&> vr2{in_place<X&>, x};
+   variant<X&> vr3{allocator_arg, a, in_place<0>, x};
+   variant<X&> vr4{allocator_arg, a, in_place<X&>, x};
+}

Re: [Patch] Fix variant::operator= on references

[Ville Voutilainen]

On 22 September 2016 at 13:03, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 22/09/16 01:49 -0700, Tim Shen wrote:
>>
>> Done. When writing the initial version, I was trying to save as much
>> qualifications as possible (as long as the semantic doesn't change)
>> for readability, but that might not be a good idea.
>
>
> It does change the semantics, as forward<_Tp>(__tp) can find another
> function via ADL (see the new test in this patch).


Yeah, it's not a question about readability or style, unqualified
function calls attract
the ADL demons like fairies attract vampires.

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 3:03 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 22/09/16 01:49 -0700, Tim Shen wrote:
>>
>> Done. When writing the initial version, I was trying to save as much
>> qualifications as possible (as long as the semantic doesn't change)
>> for readability, but that might not be a good idea.
>
>
> It does change the semantics, as forward<_Tp>(__tp) can find another
> function via ADL (see the new test in this patch).

Then my question is, what about type traits uses like
is_copy_constructible? I have seen non-qualified uses in std::any and
std::optional and other places. Should all of them be qualified?

>
> Tested powerpc64le-linux, committed to trunk.
>
>



-- 
Regards,
Tim Shen

Re: [Patch] Fix variant::operator= on references

[Tim Shen]

On Thu, Sep 22, 2016 at 3:36 AM, Tim Shen <timshen@google.com> wrote:
> Then my question is, what about type traits uses like
> is_copy_constructible? I have seen non-qualified uses in std::any and
> std::optional and other places. Should all of them be qualified?

Ah never mind, I realized that *usually* a type trait use is not part
of a function call, so ADL is not triggered.

-- 
Regards,
Tim Shen

Re: [Patch] Fix variant::operator= on references

[Jonathan Wakely]

On 22/09/16 03:40 -0700, Tim Shen wrote:
>On Thu, Sep 22, 2016 at 3:36 AM, Tim Shen <timshen@google.com> wrote:
>> Then my question is, what about type traits uses like
>> is_copy_constructible? I have seen non-qualified uses in std::any and
>> std::optional and other places. Should all of them be qualified?
>
>Ah never mind, I realized that *usually* a type trait use is not part
>of a function call, so ADL is not triggered.

ADL is only used to do name lookup for unqualified functions, so it is
never necessary to qualify those types inside namespace std. Name
lookup will always find the right type.