This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf


On 28/09/17 15:06 +0300, Petr Ovtchenkov wrote:
On Thu, 28 Sep 2017 11:34:25 +0100
Jonathan Wakely <jwakely@redhat.com> wrote:

On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
>istreambuf_iterator should not forget about attached
>streambuf when it reach EOF.
>
>Checks in debug mode has no infuence more on character
>extraction in istreambuf_iterator increment operators.
>In this aspect behaviour in debug and non-debug mode
>is similar now.
>
>Test for detached srteambuf in istreambuf_iterator:
>When istreambuf_iterator reach EOF of istream, it should not
>forget about attached streambuf.
From fact "EOF in stream reached" not follow that
>stream reach end of life and input operation impossible
>more.
>---
> libstdc++-v3/include/bits/streambuf_iterator.h     | 41 +++++++--------
> .../24_iterators/istreambuf_iterator/3.cc          | 61 ++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 22 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>
>diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
>b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d89 100644
>--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       istreambuf_iterator&
>       operator++()
>       {
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+	__glibcxx_requires_cond(_M_sbuf,
> 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> 				._M_iterator(*this));
> 	if (_M_sbuf)
> 	  {
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+	    int_type _tmp =
>+#endif
> 	    _M_sbuf->sbumpc();
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+	    __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
>+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
>+				    ._M_iterator(*this));
>+#endif
> 	    _M_c = traits_type::eof();
> 	  }
> 	return *this;
>@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       istreambuf_iterator
>       operator++(int)
>       {
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+        _M_get();
>+	__glibcxx_requires_cond(_M_sbuf
>+				&& !traits_type::eq_int_type(_M_c,traits_type::eof()),
> 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> 				._M_iterator(*this));
>
> 	istreambuf_iterator __old = *this;
> 	if (_M_sbuf)
> 	  {
>-	    __old._M_c = _M_sbuf->sbumpc();
>+	    _M_sbuf->sbumpc();
> 	    _M_c = traits_type::eof();
> 	  }
> 	return __old;
>@@ -177,18 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       _M_get() const
>       {
> 	const int_type __eof = traits_type::eof();
>-	int_type __ret = __eof;
>-	if (_M_sbuf)
>-	  {
>-	    if (!traits_type::eq_int_type(_M_c, __eof))
>-	      __ret = _M_c;
>-	    else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
>-					       __eof))
>-	      _M_c = __ret;
>-	    else
>-	      _M_sbuf = 0;
>-	  }
>-	return __ret;
>+	if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
>+          _M_c = _M_sbuf->sgetc();
>+	return _M_c;
>       }
>
>       bool
>@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
>       typedef typename traits_type::int_type               int_type;
>
>-      if (__first._M_sbuf && !__last._M_sbuf)
>+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> 	{
> 	  streambuf_type* __sb = __first._M_sbuf;
> 	  int_type __c = __sb->sgetc();
>@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
>       typedef typename traits_type::int_type               int_type;
>
>-      if (__first._M_sbuf && !__last._M_sbuf)
>+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> 	{
> 	  const int_type __ival = traits_type::to_int_type(__val);
> 	  streambuf_type* __sb = __first._M_sbuf;
>@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	      else
> 		__c = __sb->snextc();
> 	    }
>-
>-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
>-	    __first._M_c = __c;
>-	  else
>-	    __first._M_sbuf = 0;
>+	  __first._M_c = __c;
> 	}
>       return __first;
>     }
>diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new file mode 100644
>index 0000000..803ede4
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>@@ -0,0 +1,61 @@
>+// { dg-options "-std=gnu++17" }
>+
>+// Copyright (C) 2017 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/>.
>+
>+#include <algorithm>
>+#include <sstream>
>+#include <iterator>
>+#include <cstring>
>+#include <testsuite_hooks.h>
>+
>+void test03()
>+{
>+  using namespace std;
>+  bool test __attribute__((unused)) = true;

This variable should be removed, we don't need these variables any
more.

>+  std::stringstream s;
>+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
>+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
>+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
>+  //          012345678901234567890123456789012345
>+  //          0         1         2         3
>+  s << b;
>+  VERIFY( !s.fail() );
>+
>+  istreambuf_iterator<char> i(s);
>+  copy_n(i, 36, r);
>+  ++i; // EOF reached
>+  VERIFY(i == std::istreambuf_iterator<char>());
>+
>+  VERIFY(memcmp(b, r, 36) == 0);
>+
>+  s << q;
>+  VERIFY(!s.fail());
>+
>+  copy_n(i, 36, r);

This is undefined behaviour. The end-of-stream iterator value cannot
be dereferenced.

Within this test istreambuf_iterator in eof state never dereferenced.

That is quite implementation dependent.

The libc++ and VC++ implementations fail this test, because once an
istreambuf_iterator has been detected to reach end-of-stream it
doesn't get "reset" by changes to the streambuf.

The libc++ implementation crashes, because operator== on an
end-of-stream iterator sets its streambuf* to null, and any further
increment or dereference will segfault.

So this is testing something that other implementations don't support,
and isn't justifiable from the standard.

The test itself simulate "stop and go" istream usage.
stringstream is convenient for behaviuor illustration, but in "real life"
I can assume socket or tty on this place.

At the very minimum we should have a comment in the test explaining
how it relies on non-standard, non-portable behaviour.

But I'd prefer to avoid introducing more divergence from other
implementations.

debugmode-dependent behaviour is also issue in this patch;
I don't suggest it as a separate patch because solutions are intersect.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]