From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46306 invoked by alias); 28 Sep 2017 12:38:11 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 46283 invoked by uid 89); 28 Sep 2017 12:38:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 28 Sep 2017 12:38:09 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AD5E7C0CB559; Thu, 28 Sep 2017 12:38:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AD5E7C0CB559 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jwakely@redhat.com Received: from localhost (unknown [10.33.36.48]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3CCA21718A; Thu, 28 Sep 2017 12:38:07 +0000 (UTC) Date: Thu, 28 Sep 2017 12:38:00 -0000 From: Jonathan Wakely To: Petr Ovtchenkov Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf Message-ID: <20170928123806.GN4582@redhat.com> References: <20170928103425.GG4582@redhat.com> <20170928150643.2f667ec9@void-ptr.info> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20170928150643.2f667ec9@void-ptr.info> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.8.3 (2017-05-23) X-SW-Source: 2017-09/txt/msg01874.txt.bz2 On 28/09/17 15:06 +0300, Petr Ovtchenkov wrote: >On Thu, 28 Sep 2017 11:34:25 +0100 >Jonathan Wakely 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 >> >+// . >> >+ >> >+#include >> >+#include >> >+#include >> >+#include >> >+#include >> >+ >> >+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 i(s); >> >+ copy_n(i, 36, r); >> >+ ++i; // EOF reached >> >+ VERIFY(i == std::istreambuf_iterator()); >> >+ >> >+ 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.