[PATCH] libstdc++: Don't initialize from *this inside some views [PR97600]

[PATCH] libstdc++: Don't initialize from *this inside some views [PR97600]

[Patrick Palka]

This works around a subtle issue where instantiating the begin()/end()
member of some views (as part of return type deduction) inadvertently
requires computing the satisfaction value of range<foo_view>.

This is problematic because the constraint range<foo_view> requires the
begin()/end() member to be callable.  But it's not callable until we've
deduced its return type, so evaluation of range<foo_view> yields false
at this point.  And if at any point after both members are instantiated
(and their return types deduced) we evaluate range<foo_view> again, this
time it will yield true since the begin()/end() members are now both
callable.  This makes the program ill-formed according to
[temp.constr.atomic]/3:

  If, at different points in the program, the satisfaction result is
  different for identical atomic constraints and template arguments, the
  program is ill-formed, no diagnostic required.

The views affected by this issue are those whose begin()/end() member
has a placeholder return type and that member initializes an _Iterator
or _Sentinel object from a reference to *this.  The second condition is
relevant because it means explicit conversion functions are considered
during overload resolution (as per [over.match.copy], I think), and
therefore it causes g++ to check the constraints of the conversion
function view_interface<foo_view>::operator bool().  And this conversion
function's constraints indirectly require range<foo_view>.

This issue is observable on trunk only with basic_istream_view (as in
the testcase in the PR).  But a pending patch that makes g++ memoize
constraint satisaction values indefinitely (it currently invalidates
the satisfaction cache on various events) causes many existing tests for
the other affected views to fail, because range<foo_view> then remains
false for the whole compilation.

This patch works around this issue by adjusting the constructors of the
_Iterator and _Sentinel types of the affected views to take their
foo_view argument by pointer instead of by reference, so that g++ no
longer considers explicit conversion functions when resolving the
direct-initialization inside these views' begin()/end() members.

Tested on x86_64-pc-linux-gnu, and also verified that this fixes the
testsuite failures when combined with the mentioned frontend patch
(https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557237.html).
Does this look OK for trunk?

libstdc++-v3/ChangeLog:

	PR libstdc++/97600
	* include/std/ranges (basic_istream_view::begin): Initialize
	_Iterator from 'this' instead of '*this'.
	(basic_istream_view::_Iterator::_Iterator): Adjust constructor
	accordingly.
	(filter_view::_Iterator::_Iterator): Take a filter_view*
	argument instead of a filter_view& argument.
	(filter_view::_Sentinel::_Sentinel): Likewise.
	(filter_view::begin): Initialize _Iterator from 'this' instead
	of '*this'.
	(filter_view::end): Likewise.
	(transform_view::_Iterator::_Iterator): Take a _Parent* instead
	of a _Parent&.
	(filter_view::_Iterator::operator+): Adjust accordingly.
	(filter_view::_Iterator::operator-): Likewise.
	(filter_view::begin): Initialize _Iterator from 'this' instead
	of '*this'.
	(filter_view::end): Likewise.
	(join_view::_Iterator): Take a _Parent* instead of a _Parent&.
	(join_view::_Sentinel): Likewise.
	(join_view::begin): Initialize _Iterator from 'this' instead of
	'*this'.
	(join_view::end): Initialize _Sentinel from 'this' instead of
	'*this'.
	(split_view::_OuterIter): Take a _Parent& instead of a _Parent*.
	(split_view::begin): Initialize _OuterIter from 'this' instead
	of '*this'.
	(split_view::end): Likewise.
	* testsuite/std/ranges/97600.cc: New test.
---
 libstdc++-v3/include/std/ranges            | 78 +++++++++++-----------
 libstdc++-v3/testsuite/std/ranges/97600.cc | 32 +++++++++
 2 files changed, 71 insertions(+), 39 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/std/ranges/97600.cc

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index a3e5354848a..28787cc5cc3 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -636,7 +636,7 @@ namespace views
       {
 	if (_M_stream != nullptr)
 	  *_M_stream >> _M_object;
-	return _Iterator{*this};
+	return _Iterator{this};
       }
 
       constexpr default_sentinel_t
@@ -657,8 +657,8 @@ namespace views
 	_Iterator() = default;
 
 	constexpr explicit
-	_Iterator(basic_istream_view& __parent) noexcept
-	  : _M_parent(std::__addressof(__parent))
+	_Iterator(basic_istream_view* __parent) noexcept
+	  : _M_parent(__parent)
 	{ }
 
 	_Iterator(const _Iterator&) = delete;
@@ -1147,9 +1147,9 @@ namespace views
 	_Iterator() = default;
 
 	constexpr
-	_Iterator(filter_view& __parent, _Vp_iter __current)
+	_Iterator(filter_view* __parent, _Vp_iter __current)
 	  : _M_current(std::move(__current)),
-	    _M_parent(std::__addressof(__parent))
+	    _M_parent(__parent)
 	{ }
 
 	constexpr _Vp_iter
@@ -1239,8 +1239,8 @@ namespace views
 	_Sentinel() = default;
 
 	constexpr explicit
-	_Sentinel(filter_view& __parent)
-	  : _M_end(ranges::end(__parent._M_base))
+	_Sentinel(filter_view* __parent)
+	  : _M_end(ranges::end(__parent->_M_base))
 	{ }
 
 	constexpr sentinel_t<_Vp>
@@ -1280,23 +1280,23 @@ namespace views
       begin()
       {
 	if (_M_cached_begin._M_has_value())
-	  return {*this, _M_cached_begin._M_get(_M_base)};
+	  return {this, _M_cached_begin._M_get(_M_base)};
 
 	__glibcxx_assert(_M_pred.has_value());
 	auto __it = __detail::find_if(ranges::begin(_M_base),
 				      ranges::end(_M_base),
 				      std::ref(*_M_pred));
 	_M_cached_begin._M_set(_M_base, __it);
-	return {*this, std::move(__it)};
+	return {this, std::move(__it)};
       }
 
       constexpr auto
       end()
       {
 	if constexpr (common_range<_Vp>)
-	  return _Iterator{*this, ranges::end(_M_base)};
+	  return _Iterator{this, ranges::end(_M_base)};
 	else
-	  return _Sentinel{*this};
+	  return _Sentinel{this};
       }
     };
 
@@ -1375,9 +1375,9 @@ namespace views
 	  _Iterator() = default;
 
 	  constexpr
-	  _Iterator(_Parent& __parent, _Base_iter __current)
+	  _Iterator(_Parent* __parent, _Base_iter __current)
 	    : _M_current(std::move(__current)),
-	      _M_parent(std::__addressof(__parent))
+	      _M_parent(__parent)
 	  { }
 
 	  constexpr
@@ -1490,17 +1490,17 @@ namespace views
 	  friend constexpr _Iterator
 	  operator+(_Iterator __i, difference_type __n)
 	    requires random_access_range<_Base>
-	  { return {*__i._M_parent, __i._M_current + __n}; }
+	  { return {__i._M_parent, __i._M_current + __n}; }
 
 	  friend constexpr _Iterator
 	  operator+(difference_type __n, _Iterator __i)
 	    requires random_access_range<_Base>
-	  { return {*__i._M_parent, __i._M_current + __n}; }
+	  { return {__i._M_parent, __i._M_current + __n}; }
 
 	  friend constexpr _Iterator
 	  operator-(_Iterator __i, difference_type __n)
 	    requires random_access_range<_Base>
-	  { return {*__i._M_parent, __i._M_current - __n}; }
+	  { return {__i._M_parent, __i._M_current - __n}; }
 
 	  // _GLIBCXX_RESOLVE_LIB_DEFECTS
 	  // 3483. transform_view::iterator's difference is overconstrained
@@ -1611,13 +1611,13 @@ namespace views
 
       constexpr _Iterator<false>
       begin()
-      { return _Iterator<false>{*this, ranges::begin(_M_base)}; }
+      { return _Iterator<false>{this, ranges::begin(_M_base)}; }
 
       constexpr _Iterator<true>
       begin() const
 	requires range<const _Vp>
 	  && regular_invocable<const _Fp&, range_reference_t<const _Vp>>
-      { return _Iterator<true>{*this, ranges::begin(_M_base)}; }
+      { return _Iterator<true>{this, ranges::begin(_M_base)}; }
 
       constexpr _Sentinel<false>
       end()
@@ -1625,7 +1625,7 @@ namespace views
 
       constexpr _Iterator<false>
       end() requires common_range<_Vp>
-      { return _Iterator<false>{*this, ranges::end(_M_base)}; }
+      { return _Iterator<false>{this, ranges::end(_M_base)}; }
 
       constexpr _Sentinel<true>
       end() const
@@ -1637,7 +1637,7 @@ namespace views
       end() const
 	requires common_range<const _Vp>
 	  && regular_invocable<const _Fp&, range_reference_t<const _Vp>>
-      { return _Iterator<true>{*this, ranges::end(_M_base)}; }
+      { return _Iterator<true>{this, ranges::end(_M_base)}; }
 
       constexpr auto
       size() requires sized_range<_Vp>
@@ -2181,9 +2181,9 @@ namespace views
 	  _Iterator() = default;
 
 	  constexpr
-	  _Iterator(_Parent& __parent, _Outer_iter __outer)
+	  _Iterator(_Parent* __parent, _Outer_iter __outer)
 	    : _M_outer(std::move(__outer)),
-	      _M_parent(std::__addressof(__parent))
+	      _M_parent(__parent)
 	  { _M_satisfy(); }
 
 	  constexpr
@@ -2303,8 +2303,8 @@ namespace views
 	  _Sentinel() = default;
 
 	  constexpr explicit
-	  _Sentinel(_Parent& __parent)
-	    : _M_end(ranges::end(__parent._M_base))
+	  _Sentinel(_Parent* __parent)
+	    : _M_end(ranges::end(__parent->_M_base))
 	  { }
 
 	  constexpr
@@ -2351,7 +2351,7 @@ namespace views
 	constexpr bool __use_const
 	  = (__detail::__simple_view<_Vp>
 	     && is_reference_v<range_reference_t<_Vp>>);
-	return _Iterator<__use_const>{*this, ranges::begin(_M_base)};
+	return _Iterator<__use_const>{this, ranges::begin(_M_base)};
       }
 
       constexpr auto
@@ -2359,7 +2359,7 @@ namespace views
 	requires input_range<const _Vp>
 	  && is_reference_v<range_reference_t<const _Vp>>
       {
-	return _Iterator<true>{*this, ranges::begin(_M_base)};
+	return _Iterator<true>{this, ranges::begin(_M_base)};
       }
 
       constexpr auto
@@ -2368,10 +2368,10 @@ namespace views
 	if constexpr (forward_range<_Vp> && is_reference_v<_InnerRange>
 		      && forward_range<_InnerRange>
 		      && common_range<_Vp> && common_range<_InnerRange>)
-	  return _Iterator<__detail::__simple_view<_Vp>>{*this,
+	  return _Iterator<__detail::__simple_view<_Vp>>{this,
 							 ranges::end(_M_base)};
 	else
-	  return _Sentinel<__detail::__simple_view<_Vp>>{*this};
+	  return _Sentinel<__detail::__simple_view<_Vp>>{this};
       }
 
       constexpr auto
@@ -2384,9 +2384,9 @@ namespace views
 		      && forward_range<range_reference_t<const _Vp>>
 		      && common_range<const _Vp>
 		      && common_range<range_reference_t<const _Vp>>)
-	  return _Iterator<true>{*this, ranges::end(_M_base)};
+	  return _Iterator<true>{this, ranges::end(_M_base)};
 	else
-	  return _Sentinel<true>{*this};
+	  return _Sentinel<true>{this};
       }
     };
 
@@ -2505,14 +2505,14 @@ namespace views
 	  _OuterIter() = default;
 
 	  constexpr explicit
-	  _OuterIter(_Parent& __parent) requires (!forward_range<_Base>)
-	    : _M_parent(std::__addressof(__parent))
+	  _OuterIter(_Parent* __parent) requires (!forward_range<_Base>)
+	    : _M_parent(__parent)
 	  { }
 
 	  constexpr
-	  _OuterIter(_Parent& __parent, iterator_t<_Base> __current)
+	  _OuterIter(_Parent* __parent, iterator_t<_Base> __current)
 	    requires forward_range<_Base>
-	    : _M_parent(std::__addressof(__parent)),
+	    : _M_parent(__parent),
 	      _M_current(std::move(__current))
 	  { }
 
@@ -2737,25 +2737,25 @@ namespace views
       {
 	if constexpr (forward_range<_Vp>)
 	  return _OuterIter<__detail::__simple_view<_Vp>>{
-	      *this, ranges::begin(_M_base)};
+	      this, ranges::begin(_M_base)};
 	else
 	  {
 	    _M_current = ranges::begin(_M_base);
-	    return _OuterIter<false>{*this};
+	    return _OuterIter<false>{this};
 	  }
       }
 
       constexpr auto
       begin() const requires forward_range<_Vp> && forward_range<const _Vp>
       {
-	return _OuterIter<true>{*this, ranges::begin(_M_base)};
+	return _OuterIter<true>{this, ranges::begin(_M_base)};
       }
 
       constexpr auto
       end() requires forward_range<_Vp> && common_range<_Vp>
       {
 	return _OuterIter<__detail::__simple_view<_Vp>>{
-	    *this, ranges::end(_M_base)};
+	    this, ranges::end(_M_base)};
       }
 
       constexpr auto
@@ -2764,7 +2764,7 @@ namespace views
 	if constexpr (forward_range<_Vp>
 		      && forward_range<const _Vp>
 		      && common_range<const _Vp>)
-	  return _OuterIter<true>{*this, ranges::end(_M_base)};
+	  return _OuterIter<true>{this, ranges::end(_M_base)};
 	else
 	  return default_sentinel;
       }
diff --git a/libstdc++-v3/testsuite/std/ranges/97600.cc b/libstdc++-v3/testsuite/std/ranges/97600.cc
new file mode 100644
index 00000000000..d992318259d
--- /dev/null
+++ b/libstdc++-v3/testsuite/std/ranges/97600.cc
@@ -0,0 +1,32 @@
+// Copyright (C) 2020 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library 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, or (at your option)
+// any later version.
+
+// This library 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 library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++2a" }
+// { dg-do compile { target c++2a } }
+
+// PR libstdc++/97600
+
+#include <sstream>
+#include <ranges>
+
+void
+test01()
+{
+  std::ranges::basic_istream_view<int, char, std::char_traits<char>> v;
+  v.begin();
+  static_assert(std::ranges::range<decltype(v)>);
+}
-- 
2.29.0.rc0

Re: [PATCH] libstdc++: Don't initialize from *this inside some views [PR97600]

[Jonathan Wakely]

On 30/10/20 11:11 -0400, Patrick Palka via Libstdc++ wrote:
>This works around a subtle issue where instantiating the begin()/end()
>member of some views (as part of return type deduction) inadvertently
>requires computing the satisfaction value of range<foo_view>.
>
>This is problematic because the constraint range<foo_view> requires the
>begin()/end() member to be callable.  But it's not callable until we've
>deduced its return type, so evaluation of range<foo_view> yields false
>at this point.  And if at any point after both members are instantiated
>(and their return types deduced) we evaluate range<foo_view> again, this
>time it will yield true since the begin()/end() members are now both
>callable.  This makes the program ill-formed according to
>[temp.constr.atomic]/3:
>
>  If, at different points in the program, the satisfaction result is
>  different for identical atomic constraints and template arguments, the
>  program is ill-formed, no diagnostic required.
>
>The views affected by this issue are those whose begin()/end() member
>has a placeholder return type and that member initializes an _Iterator
>or _Sentinel object from a reference to *this.  The second condition is
>relevant because it means explicit conversion functions are considered
>during overload resolution (as per [over.match.copy], I think), and
>therefore it causes g++ to check the constraints of the conversion
>function view_interface<foo_view>::operator bool().  And this conversion
>function's constraints indirectly require range<foo_view>.
>
>This issue is observable on trunk only with basic_istream_view (as in
>the testcase in the PR).  But a pending patch that makes g++ memoize
>constraint satisaction values indefinitely (it currently invalidates
>the satisfaction cache on various events) causes many existing tests for
>the other affected views to fail, because range<foo_view> then remains
>false for the whole compilation.
>
>This patch works around this issue by adjusting the constructors of the
>_Iterator and _Sentinel types of the affected views to take their
>foo_view argument by pointer instead of by reference, so that g++ no
>longer considers explicit conversion functions when resolving the
>direct-initialization inside these views' begin()/end() members.

Nice solution.

>Tested on x86_64-pc-linux-gnu, and also verified that this fixes the
>testsuite failures when combined with the mentioned frontend patch
>(https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557237.html).
>Does this look OK for trunk?

Yes, thanks.