Re: [PATCH] libstdc++/77641 fix std::variant for literal types

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Ville Voutilainen]

This problem is not introduced by the latest patch, but it's something that
we should look at anyway. There's been recent discussion about what
assignments do with variants that hold references. Consider this:

#include <variant>

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

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

   v1 = f2; // #1
}

The line marked with #1 fails to compile before and after the latest
variant patch:

In file included from tony-variant1.cpp:1:0:
/usr/local/include/c++/7.0.0/variant: In instantiation of
‘std::enable_if_t<(((__exactly_once<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type> &&
is_constructible_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type, _Tp&&>) &&
is_assignable_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type&, _Tp&&>) && (!
is_same_v<typename std::decay<_Tp>::type, std::variant<_Types> >)),
std::variant<_Types>&> std::variant<_Types>::operator=(_Tp&&) [with
_Tp = float&; _Types = {float&};
std::enable_if_t<(((__exactly_once<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type> &&
is_constructible_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type, _Tp&&>) &&
is_assignable_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type&, _Tp&&>) && (!
is_same_v<typename std::decay<_Tp>::type, std::variant<_Types> >)),
std::variant<_Types>&> = std::variant<float&>&]’:
tony-variant1.cpp:9:9:   required from here
/usr/local/include/c++/7.0.0/variant:1151:8: error: use of deleted
function ‘std::__detail::__variant::_Reference_storage<float&>&
std::__detail::__variant::_Reference_storage<float&>::operator=(std::__detail::__variant::_Reference_storage<float&>&&)’
      *static_cast<__storage<__to_type<__index>>*>(this->_M_storage())
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        = forward<_Tp>(__rhs);
        ^~~~~~~~~~~~~~~~~~~~~
In file included from tony-variant1.cpp:1:0:
/usr/local/include/c++/7.0.0/variant:137:12: note:
‘std::__detail::__variant::_Reference_storage<float&>&
std::__detail::__variant::_Reference_storage<float&>::operator=(std::__detail::__variant::_Reference_storage<float&>&&)’
is implicitly deleted because the default definition would be
ill-formed:
     struct _Reference_storage
            ^~~~~~~~~~~~~~~~~~
/usr/local/include/c++/7.0.0/variant:137:12: error: non-static
reference member ‘float&
std::__detail::__variant::_Reference_storage<float&>::_M_storage’,
can’t use default assignment operator

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Jonathan Wakely]

On 22/09/16 09:45 +0300, Ville Voutilainen wrote:
>This problem is not introduced by the latest patch, but it's something that
>we should look at anyway. There's been recent discussion about what
>assignments do with variants that hold references. Consider this:
>
>#include <variant>
>
>int main()
>{
>   float f1 = 1.0f, f2 = 2.0f;
>
>   std::variant<float&> v1(f1);
>
>   v1 = f2; // #1
>}

It works if we add this to _Reference_storage:

      template<typename _Tp>
	_Reference_storage&
	operator=(_Tp&& __t)
	{
	  _M_storage = std::forward<_Tp>(__t);
	  return *this;
	}

I'm not sure if that's the right fix.

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Jonathan Wakely]

On 22/09/16 09:37 +0100, Jonathan Wakely wrote:
>On 22/09/16 09:45 +0300, Ville Voutilainen wrote:
>>This problem is not introduced by the latest patch, but it's something that
>>we should look at anyway. There's been recent discussion about what
>>assignments do with variants that hold references. Consider this:
>>
>>#include <variant>
>>
>>int main()
>>{
>>  float f1 = 1.0f, f2 = 2.0f;
>>
>>  std::variant<float&> v1(f1);
>>
>>  v1 = f2; // #1
>>}
>
>It works if we add this to _Reference_storage:
>
>     template<typename _Tp>
>	_Reference_storage&
>	operator=(_Tp&& __t)
>	{
>	  _M_storage = std::forward<_Tp>(__t);
>	  return *this;
>	}
>
>I'm not sure if that's the right fix.

Oh I missed Tim's patch for this that he already posted.

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Jonathan Wakely]

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

On 21/09/16 20:23 -0700, Tim Shen wrote:
>On Wed, Sep 21, 2016 at 1:52 AM, Jonathan Wakely wrote:
>> THanks, OK for trunk.
>
>Committed.

This fixes the pretty printer.

Committed to trunk.


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

commit 6b869af56fa80da5b746390ce5616ebebcc0bd5d
Author: redi <redi@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Sep 22 10:06:41 2016 +0000

    Update pretty printer for std::variant
    
    	* python/libstdcxx/v6/printers.py (StdVariantPrinter): Adjust for
    	recent change to _Variant_storage.
    	* testsuite/libstdc++-prettyprinters/cxx17.cc: Test variant with
    	reference type.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@240345 138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index ac529dd..18c6f6b 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -994,7 +994,7 @@ class StdVariantPrinter(SingleObjContainerPrinter):
             visualizer = None
         else:
             self.contained_type = alternatives[int(self.index)]
-            addr = val['_M_first']['_M_storage'].address
+            addr = val['_M_union']['_M_first']['_M_storage'].address
             contained_value = addr.cast(self.contained_type.pointer()).dereference()
             visualizer = gdb.default_visualizer(contained_value)
         super (StdVariantPrinter, self).__init__(contained_value, visualizer, 'array')
diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc
index 42d79c0..09d79e9 100644
--- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc
+++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc
@@ -84,6 +84,9 @@ main()
   variant<float, int, string_view> v4{ str };
 // { dg-final { note-test v4 {std::variant<float, int, std::string_view> [index 2] = {"string"}} } }
 
+  variant<string_view&> vref{str};
+// { dg-final { note-test vref {std::variant<std::basic_string_view<char, std::char_traits<char> > &> [index 0] = {"string"}} } }
+
   std::cout << "\n";
   return 0;			// Mark SPOT
 }

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Tim Shen]

On Wed, Sep 21, 2016 at 1:52 AM, Jonathan Wakely wrote:
> THanks, OK for trunk.

Committed.

Thanks!


-- 
Regards,
Tim Shen

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Jonathan Wakely]

On 20/09/16 11:03 -0700, Tim Shen wrote:
>On Mon, Sep 19, 2016 at 1:06 PM, Tim Shen <timshen@google.com> wrote:
>> I believe that it's a "typo" from me - it should be
>> is_trivially_destructible, not is_default_constructible (so that we
>> can avoid using aligned_storage in the corresponding specialization).
>> is_literal_type works, since literal types are trivially destructible.
>
>Sorry I misunderstood your patch.
>
>The underlying problem is that _Variant_storage wasn't always default
>constructible, but it should be.
>
>Jon, your fix doesn't fix the following constexpr variation of your test case:
>  struct X {
>    constexpr X() { }
>  };
>
>  constexpr std::variant<X> v1 = X{};
>
>So I have a patch here to make _Variant_storage's internal union
>default constructible.
>
>Tested on x86_64-linux-gnu.

THanks, OK for trunk.

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Tim Shen]

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

On Mon, Sep 19, 2016 at 1:06 PM, Tim Shen <timshen@google.com> wrote:
> I believe that it's a "typo" from me - it should be
> is_trivially_destructible, not is_default_constructible (so that we
> can avoid using aligned_storage in the corresponding specialization).
> is_literal_type works, since literal types are trivially destructible.

Sorry I misunderstood your patch.

The underlying problem is that _Variant_storage wasn't always default
constructible, but it should be.

Jon, your fix doesn't fix the following constexpr variation of your test case:
  struct X {
    constexpr X() { }
  };

  constexpr std::variant<X> v1 = X{};

So I have a patch here to make _Variant_storage's internal union
default constructible.

Tested on x86_64-linux-gnu.


-- 
Regards,
Tim Shen

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

commit 9f29dcfbb779ccbafc5b7b36192ef6b0d4826ec9
Author: Tim Shen <timshen@google.com>
Date:   Tue Sep 20 10:50:10 2016 -0700

    2016-09-20  Tim Shen  <timshen@google.com>
    
    	PR libstdc++/77641
    	* include/std/variant (_Variant_storage::_Variant_storage):
    	Change _Variant_storage's union to be default constructible.
    	* testsuite/20_util/variant/compile.cc: New test.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 7dbb533..013884b 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -296,15 +296,9 @@ namespace __variant
     {
       constexpr _Variant_storage() = default;
 
-      template<typename... _Args>
-	constexpr _Variant_storage(in_place_index_t<0>, _Args&&... __args)
-	: _M_first(in_place<0>, forward<_Args>(__args)...)
-	{ }
-
-      template<size_t _Np, typename... _Args,
-	       typename = enable_if_t<0 < _Np && _Np < sizeof...(_Rest) + 1>>
+      template<size_t _Np, typename... _Args>
 	constexpr _Variant_storage(in_place_index_t<_Np>, _Args&&... __args)
-	: _M_rest(in_place<_Np - 1>, forward<_Args>(__args)...)
+	: _M_union(in_place<_Np>, forward<_Args>(__args)...)
 	{ }
 
       ~_Variant_storage() = default;
@@ -313,14 +307,27 @@ namespace __variant
       _M_storage() const
       {
 	return const_cast<void*>(
-	  static_cast<const void*>(std::addressof(_M_first._M_storage)));
+	  static_cast<const void*>(std::addressof(_M_union._M_first._M_storage)));
       }
 
-      union
+      union _Union
       {
+	constexpr _Union() {};
+
+	template<typename... _Args>
+	  constexpr _Union(in_place_index_t<0>, _Args&&... __args)
+	  : _M_first(in_place<0>, 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)...)
+	  { }
+
 	_Uninitialized<__storage<_First>> _M_first;
 	_Variant_storage<_Rest...> _M_rest;
-      };
+      } _M_union;
     };
 
   template<typename _Derived, bool __is_trivially_destructible>
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index b57d356..99f980a 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -403,3 +403,12 @@ void test_void()
   v = 3;
   v = "asdf";
 }
+
+void test_pr77641()
+{
+  struct X {
+    constexpr X() { }
+  };
+
+  constexpr std::variant<X> v1 = X{};
+}

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Tim Shen]

On Mon, Sep 19, 2016 at 5:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> As noted in bugzilla PR 77641 I don't think is_literal_type is the
> right condition for _Uninitialized, because a type can have a
> non-trivial default constructor but still be literal, but that means
> it can't be used in the union in _Variant_storage.
>
> I'm not sure if the right condition is is_literal &&
> is_trivially_default_constructible, or if this is enough:

I believe that it's a "typo" from me - it should be
is_trivially_destructible, not is_default_constructible (so that we
can avoid using aligned_storage in the corresponding specialization).
is_literal_type works, since literal types are trivially destructible.

>
>         PR libstdc++/77641
>         * include/std/variant (__detail::__variant::_Uninitialized): Check
>         is_trivially_default_constructible_v instead of is_literal_type_v.
>         * testsuite/20_util/variant/compile.cc: Test literal type with
>         non-trivial default constructor.
>
> Tim, are there case that this doesn't handle, where we need is_literal
> as well? (bear in mind that is_trivially_default_constructible also
> depends on trivially destructible).

I checked for is_default_constructible, and all other sites are appropriate.


-- 
Regards,
Tim Shen

[PATCH] libstdc++/77641 fix std::variant for literal types

[Jonathan Wakely]

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

As noted in bugzilla PR 77641 I don't think is_literal_type is the
right condition for _Uninitialized, because a type can have a
non-trivial default constructor but still be literal, but that means
it can't be used in the union in _Variant_storage.

I'm not sure if the right condition is is_literal &&
is_trivially_default_constructible, or if this is enough:

	PR libstdc++/77641
	* include/std/variant (__detail::__variant::_Uninitialized): Check
	is_trivially_default_constructible_v instead of is_literal_type_v.
	* testsuite/20_util/variant/compile.cc: Test literal type with
	non-trivial default constructor.

Tim, are there case that this doesn't handle, where we need is_literal
as well? (bear in mind that is_trivially_default_constructible also
depends on trivially destructible).

Tested powerpc64le-linux with no failures.


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

commit 0d5f60751145f81d914d7411e0f4d79546e06fed
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Sep 19 13:18:07 2016 +0100

    libstdc++/77641 fix std::variant for literal types
    
    	PR libstdc++/77641
    	* include/std/variant (__detail::__variant::_Uninitialized): Check
    	is_trivially_default_constructible_v instead of is_literal_type_v.
    	* testsuite/20_util/variant/compile.cc: Test literal type with
    	non-trivial default constructor.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 7dbb533..6c090f6 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -168,7 +168,7 @@ namespace __variant
   template<typename _Type>
     using __storage = typename __storage_type<_Type>::type;
 
-  template<typename _Type, bool __is_literal = std::is_literal_type_v<_Type>>
+  template<typename _Type, bool = is_trivially_default_constructible_v<_Type>>
     struct _Uninitialized;
 
   template<typename _Type>
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index b57d356..3042a2e 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -403,3 +403,12 @@ void test_void()
   v = 3;
   v = "asdf";
 }
+
+void test_pr77641()
+{
+  struct X {
+    constexpr X() { }
+  };
+
+  std::variant<X> v1 = X{};
+}