[PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Jonathan Wakely]

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

The standard says that formatted/unformatted input operations should
catch any exceptions, set iostate bits in the istream, and rethrow if
the exception mask says to do so.

We're failing to do that when the exception happens in the
istream::sentry constructor, while it extracts characters to skip
whitespace. This means that input operations exit with an exception
even when the mask says not to. The fix is simply to add a try/catch
to the sentry constructor.

I've also clarified some comments related to these semantics.

	PR libstdc++/53984
	* include/bits/basic_ios.h (basic_ios::_M_setstate): Adjust comment.
	* include/bits/istream.tcc (basic_istream::sentry): Handle exceptions
	during construction.
	* include/std/istream: Adjust comments for formatted input functions
	and unformatted input functions.
	* testsuite/27_io/basic_fstream/53984.cc: New.
	* testsuite/27_io/basic_istream/sentry/char/53984.cc: New.

Tested powerpc64le-linux, committed to trunk.


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

commit a9a626df1531d0f06b147140b0f405f0847a0a42
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jul 25 19:12:22 2017 +0100

    PR libstdc++/53984 handle exceptions in basic_istream::sentry
    
    	PR libstdc++/53984
    	* include/bits/basic_ios.h (basic_ios::_M_setstate): Adjust comment.
    	* include/bits/istream.tcc (basic_istream::sentry): Handle exceptions
    	during construction.
    	* include/std/istream: Adjust comments for formatted input functions
    	and unformatted input functions.
    	* testsuite/27_io/basic_fstream/53984.cc: New.
    	* testsuite/27_io/basic_istream/sentry/char/53984.cc: New.

diff --git a/libstdc++-v3/include/bits/basic_ios.h b/libstdc++-v3/include/bits/basic_ios.h
index 318e41b..f0b8682 100644
--- a/libstdc++-v3/include/bits/basic_ios.h
+++ b/libstdc++-v3/include/bits/basic_ios.h
@@ -157,8 +157,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       setstate(iostate __state)
       { this->clear(this->rdstate() | __state); }
 
-      // Flip the internal state on for the proper state bits, then re
-      // throws the propagated exception if bit also set in
+      // Flip the internal state on for the proper state bits, then
+      // rethrows the propagated exception if bit also set in
       // exceptions().
       void
       _M_setstate(iostate __state)
diff --git a/libstdc++-v3/include/bits/istream.tcc b/libstdc++-v3/include/bits/istream.tcc
index b390f74..92df9d1 100644
--- a/libstdc++-v3/include/bits/istream.tcc
+++ b/libstdc++-v3/include/bits/istream.tcc
@@ -48,28 +48,36 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       ios_base::iostate __err = ios_base::goodbit;
       if (__in.good())
-	{
-	  if (__in.tie())
-	    __in.tie()->flush();
-	  if (!__noskip && bool(__in.flags() & ios_base::skipws))
-	    {
-	      const __int_type __eof = traits_type::eof();
-	      __streambuf_type* __sb = __in.rdbuf();
-	      __int_type __c = __sb->sgetc();
+	__try
+	  {
+	    if (__in.tie())
+	      __in.tie()->flush();
+	    if (!__noskip && bool(__in.flags() & ios_base::skipws))
+	      {
+		const __int_type __eof = traits_type::eof();
+		__streambuf_type* __sb = __in.rdbuf();
+		__int_type __c = __sb->sgetc();
 
-	      const __ctype_type& __ct = __check_facet(__in._M_ctype);
-	      while (!traits_type::eq_int_type(__c, __eof)
-		     && __ct.is(ctype_base::space, 
-				traits_type::to_char_type(__c)))
-		__c = __sb->snextc();
+		const __ctype_type& __ct = __check_facet(__in._M_ctype);
+		while (!traits_type::eq_int_type(__c, __eof)
+		       && __ct.is(ctype_base::space,
+				  traits_type::to_char_type(__c)))
+		  __c = __sb->snextc();
 
-	      // _GLIBCXX_RESOLVE_LIB_DEFECTS
-	      // 195. Should basic_istream::sentry's constructor ever
-	      // set eofbit?
-	      if (traits_type::eq_int_type(__c, __eof))
-		__err |= ios_base::eofbit;
-	    }
-	}
+		// _GLIBCXX_RESOLVE_LIB_DEFECTS
+		// 195. Should basic_istream::sentry's constructor ever
+		// set eofbit?
+		if (traits_type::eq_int_type(__c, __eof))
+		  __err |= ios_base::eofbit;
+	      }
+	  }
+	__catch(__cxxabiv1::__forced_unwind&)
+	  {
+	    __in._M_setstate(ios_base::badbit);
+	    __throw_exception_again;
+	  }
+	__catch(...)
+	  { __in._M_setstate(ios_base::badbit); }
 
       if (__in.good() && __err == ios_base::goodbit)
 	_M_ok = true;
diff --git a/libstdc++-v3/include/std/istream b/libstdc++-v3/include/std/istream
index 1fa2555..233cc6b 100644
--- a/libstdc++-v3/include/std/istream
+++ b/libstdc++-v3/include/std/istream
@@ -150,9 +150,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        *  whatever data is appropriate for the type of the argument.
        *
        *  If an exception is thrown during extraction, ios_base::badbit
-       *  will be turned on in the stream's error state without causing an
-       *  ios_base::failure to be thrown.  The original exception will then
-       *  be rethrown.
+       *  will be turned on in the stream's error state (without causing an
+       *  ios_base::failure to be thrown) and the original exception will
+       *  be rethrown if badbit is set in the exceptions mask.
       */
 
       //@{
@@ -286,9 +286,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        *  by gcount().
        *
        *  If an exception is thrown during extraction, ios_base::badbit
-       *  will be turned on in the stream's error state without causing an
-       *  ios_base::failure to be thrown.  The original exception will then
-       *  be rethrown.
+       *  will be turned on in the stream's error state (without causing an
+       *  ios_base::failure to be thrown) and the original exception will
+       *  be rethrown if badbit is set in the exceptions mask.
       */
 
       /**
diff --git a/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
new file mode 100644
index 0000000..e84072e
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
@@ -0,0 +1,36 @@
+// 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/>.
+
+// { dg-require-file-io "" }
+
+#include <fstream>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  std::fstream in(".");
+  int x;
+  in >> x;
+  VERIFY( in.bad() );
+}
+
+int
+main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/sentry/char/53984.cc b/libstdc++-v3/testsuite/27_io/basic_istream/sentry/char/53984.cc
new file mode 100644
index 0000000..9c08c72
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/sentry/char/53984.cc
@@ -0,0 +1,41 @@
+// 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 <streambuf>
+#include <istream>
+#include <testsuite_hooks.h>
+
+struct SB : std::streambuf
+{
+  virtual int_type underflow() { throw 1; }
+};
+
+void
+test01()
+{
+  SB sb;
+  std::istream is(&sb);
+  int i;
+  is >> i;
+  VERIFY( is.bad() );
+}
+
+int
+main()
+{
+  test01();
+}

Re: [PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Andreas Schwab]

On Jul 25 2017, Jonathan Wakely <jwakely@redhat.com> wrote:

> diff --git a/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
> new file mode 100644
> index 0000000..e84072e
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
> @@ -0,0 +1,36 @@
> +// 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/>.
> +
> +// { dg-require-file-io "" }

ERROR: 27_io/basic_fstream/53984.cc: unknown dg option: dg-require-file-io 18 {} for " dg-require-file-io 18 "" "

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Paolo Carlini]

Hi,

On 26/07/2017 16:21, Andreas Schwab wrote:
> ERROR: 27_io/basic_fstream/53984.cc: unknown dg option: 
> dg-require-file-io 18 {} for " dg-require-file-io 18 "" " 
Should be already fixed, a trivial typo.

Thanks,
Paolo.

Re: [PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Paolo Carlini]

Hi again,

On 26/07/2017 16:27, Paolo Carlini wrote:
> Hi,
>
> On 26/07/2017 16:21, Andreas Schwab wrote:
>> ERROR: 27_io/basic_fstream/53984.cc: unknown dg option: 
>> dg-require-file-io 18 {} for " dg-require-file-io 18 "" " 
> Should be already fixed, a trivial typo.
... but now the new test simply fails for me. If I don't spot something 
else trivial over the next few hours I guess better waiting for Jon to 
look into that.

Paolo.

Re: [PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Ville Voutilainen]

On 26 July 2017 at 21:14, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi again,
>
> On 26/07/2017 16:27, Paolo Carlini wrote:
>>
>> Hi,
>>
>> On 26/07/2017 16:21, Andreas Schwab wrote:
>>>
>>> ERROR: 27_io/basic_fstream/53984.cc: unknown dg option:
>>> dg-require-file-io 18 {} for " dg-require-file-io 18 "" "
>>
>> Should be already fixed, a trivial typo.
>
> ... but now the new test simply fails for me. If I don't spot something else
> trivial over the next few hours I guess better waiting for Jon to look into
> that.


Fails how?

Re: [PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Jonathan Wakely]

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

On 26/07/17 20:14 +0200, Paolo Carlini wrote:
>Hi again,
>
>On 26/07/2017 16:27, Paolo Carlini wrote:
>>Hi,
>>
>>On 26/07/2017 16:21, Andreas Schwab wrote:
>>>ERROR: 27_io/basic_fstream/53984.cc: unknown dg option: 
>>>dg-require-file-io 18 {} for " dg-require-file-io 18 "" "
>>Should be already fixed, a trivial typo.
>... but now the new test simply fails for me. If I don't spot 
>something else trivial over the next few hours I guess better waiting 
>for Jon to look into that.

Sorry about that, I must have only checked for FAILs and missed the
ERRORs.

It should have been an ifstream not fstream, otherwise the filebuf
can't even open the file. Fixed like so, committed to trunk.



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

commit 9c0a34c998843402049165f5e2bb01643da22fb7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Jul 26 23:01:08 2017 +0100

    PR libstdc++/53984 fix failing test
    
    	PR libstdc++/53984
    	* testsuite/27_io/basic_fstream/53984.cc: Fix test.

diff --git a/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
index 53c77c2..e49d2b1 100644
--- a/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
@@ -23,10 +23,13 @@
 void
 test01()
 {
-  std::fstream in(".");
-  int x;
-  in >> x;
-  VERIFY( in.bad() );
+  std::ifstream in(".");
+  if (in)
+  {
+    int x;
+    in >> x;
+    VERIFY( in.bad() );
+  }
 }
 
 int

Re: [PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Bin.Cheng]

On Wed, Jul 26, 2017 at 11:06 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 26/07/17 20:14 +0200, Paolo Carlini wrote:
>>
>> Hi again,
>>
>> On 26/07/2017 16:27, Paolo Carlini wrote:
>>>
>>> Hi,
>>>
>>> On 26/07/2017 16:21, Andreas Schwab wrote:
>>>>
>>>> ERROR: 27_io/basic_fstream/53984.cc: unknown dg option:
>>>> dg-require-file-io 18 {} for " dg-require-file-io 18 "" "
>>>
>>> Should be already fixed, a trivial typo.
>>
>> ... but now the new test simply fails for me. If I don't spot something
>> else trivial over the next few hours I guess better waiting for Jon to look
>> into that.
>
>
> Sorry about that, I must have only checked for FAILs and missed the
> ERRORs.
>
> It should have been an ifstream not fstream, otherwise the filebuf
> can't even open the file. Fixed like so, committed to trunk.
Hi, I have seen below failure on aarch64/arm linux/elf:
spawn [open ...]^M
/tmp/.../src/gcc/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc:29:
void test01(): Assertion 'in.bad()' failed.
FAIL: 27_io/basic_fstream/53984.cc execution test
extra_tool_flags are:
  -include bits/stdc++.h

Thanks,
bin
>
>

Re: [PATCH] PR libstdc++/53984 handle exceptions in basic_istream::sentry

[Jonathan Wakely]

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

On 26/07/17 23:06 +0100, Jonathan Wakely wrote:
>On 26/07/17 20:14 +0200, Paolo Carlini wrote:
>>Hi again,
>>
>>On 26/07/2017 16:27, Paolo Carlini wrote:
>>>Hi,
>>>
>>>On 26/07/2017 16:21, Andreas Schwab wrote:
>>>>ERROR: 27_io/basic_fstream/53984.cc: unknown dg option: 
>>>>dg-require-file-io 18 {} for " dg-require-file-io 18 "" "
>>>Should be already fixed, a trivial typo.
>>... but now the new test simply fails for me. If I don't spot 
>>something else trivial over the next few hours I guess better 
>>waiting for Jon to look into that.
>
>Sorry about that, I must have only checked for FAILs and missed the
>ERRORs.
>
>It should have been an ifstream not fstream, otherwise the filebuf
>can't even open the file. Fixed like so, committed to trunk.

This FAILs on AIX, because reading from a directory with fread doesn't
fail on AIX. This makes the test check whether reading fails, before
trying to check the behaviour of formatted input functions that fail.

Tested powerpc64le-linux and powerpc-aix, committed to trunk.


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

commit bb7f3e33c3442f2d93134555bb74cf3ea2991710
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Aug 10 22:02:17 2017 +0100

    PR libstdc++/81808 skip test if reading directory doesn't fail
    
    	PR libstdc++/81808
    	* testsuite/27_io/basic_fstream/53984.cc: Adjust test for targets
    	that allow opening a directory as a FILE and reading from it.

diff --git a/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
index e49d2b1..a319aff 100644
--- a/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_fstream/53984.cc
@@ -17,6 +17,8 @@
 
 // { dg-require-fileio "" }
 
+// PR libstdc++/53984
+
 #include <fstream>
 #include <testsuite_hooks.h>
 
@@ -26,9 +28,32 @@ test01()
   std::ifstream in(".");
   if (in)
   {
+    char c;
+    if (in.get(c))
+    {
+      // Reading a directory doesn't produce an error on this target
+      // so the formatted input functions below wouldn't fail anyway
+      // (see PR libstdc++/81808).
+      return;
+    }
     int x;
+    in.clear();
+    // Formatted input function should set badbit, but not throw:
     in >> x;
     VERIFY( in.bad() );
+
+    in.clear();
+    in.exceptions(std::ios::badbit);
+    try
+    {
+      // Formatted input function should set badbit, and throw:
+      in >> x;
+      VERIFY( false );
+    }
+    catch (const std::exception&)
+    {
+      VERIFY( in.bad() );
+    }
   }
 }