Re: [PATCH] PR libstdc++/81751 don't call fflush(NULL)

[Jonathan Wakely]

On 09/08/17 18:39 +0200, Paolo Carlini wrote:

Hi,


On 9 Aug 2017, at 17:56, Jonathan Wakely  wrote:

This fixes a couple of problems in __gnu_cxx::stdio_filebuf,
specifically in the __basic_file::sys_open(FILE*, openmode) function
it uses when constructed from an existing FILE stream.

Firstly, r86756 changed __basic_file::sys_open(FILE*, openmode) to put
the call to sync() before the assignment that sets _M_cfile. This
means the fflush(_M_cfile) call has a null argument, and flushes all
open streams. I don't think that was intentional, and we only want to
flush the FILE we're constructing the streambuf with. (I think this is
a regression from 3.4.3, which just flushed the one stream).

Secondly, we zero errno so that we can tell if fflush sets it. We need
to restore the original value to meet the promise we make at
https://gcc.gnu.org/onlinedocs/libstdc++/manual/errno.html

Paolo, does the this->sync() to fflush(__file) change look right?


Yes, I went through the audit trail of libstdc++/81751 and the change looks 
good. Certainly we didn't intentionally want the behavior of fflush(0)!


OK, thanks - I've committed the change to trunk now.

Re: [PATCH] PR libstdc++/81751 don't call fflush(NULL)

[Paolo Carlini]

Hi,

> On 9 Aug 2017, at 17:56, Jonathan Wakely  wrote:
> 
> This fixes a couple of problems in __gnu_cxx::stdio_filebuf,
> specifically in the __basic_file::sys_open(FILE*, openmode) function
> it uses when constructed from an existing FILE stream.
> 
> Firstly, r86756 changed __basic_file::sys_open(FILE*, openmode) to put
> the call to sync() before the assignment that sets _M_cfile. This
> means the fflush(_M_cfile) call has a null argument, and flushes all
> open streams. I don't think that was intentional, and we only want to
> flush the FILE we're constructing the streambuf with. (I think this is
> a regression from 3.4.3, which just flushed the one stream).
> 
> Secondly, we zero errno so that we can tell if fflush sets it. We need
> to restore the original value to meet the promise we make at
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/errno.html
> 
> Paolo, does the this->sync() to fflush(__file) change look right?

Yes, I went through the audit trail of libstdc++/81751 and the change looks 
good. Certainly we didn't intentionally want the behavior of fflush(0)!

Thanks!
Paolo

[PATCH] PR libstdc++/81751 don't call fflush(NULL)

[Jonathan Wakely]

This fixes a couple of problems in __gnu_cxx::stdio_filebuf,
specifically in the __basic_file::sys_open(FILE*, openmode) function
it uses when constructed from an existing FILE stream.

Firstly, r86756 changed __basic_file::sys_open(FILE*, openmode) to put
the call to sync() before the assignment that sets _M_cfile. This
means the fflush(_M_cfile) call has a null argument, and flushes all
open streams. I don't think that was intentional, and we only want to
flush the FILE we're constructing the streambuf with. (I think this is
a regression from 3.4.3, which just flushed the one stream).

Secondly, we zero errno so that we can tell if fflush sets it. We need
to restore the original value to meet the promise we make at
https://gcc.gnu.org/onlinedocs/libstdc++/manual/errno.html

Paolo, does the this->sync() to fflush(__file) change look right?

PR libstdc++/79820
PR libstdc++/81751
* config/io/basic_file_stdio.cc (sys_open(FILE*, ios_base::openmode)):
Call fflush on the stream instead of calling sync() while _M_cfile is
null. Restore original value of errno.
* testsuite/ext/stdio_filebuf/char/79820.cc: New.
* testsuite/ext/stdio_filebuf/char/81751.cc: New.

Tested powerpc64le-linux, not yet committed.

commit 72565d197cc91a04882a398d9d242a0581d6cc09
Author: Jonathan Wakely 
Date:   Tue Aug 8 23:20:40 2017 +0100

PR libstdc++/81751 don't call fflush(NULL)

PR libstdc++/79820
PR libstdc++/81751
* config/io/basic_file_stdio.cc (sys_open(FILE*, ios_base::openmode)):
Call fflush on the stream instead of calling sync() while _M_cfile is
null. Restore original value of errno.
* testsuite/ext/stdio_filebuf/char/79820.cc: New.
* testsuite/ext/stdio_filebuf/char/81751.cc: New.

diff --git a/libstdc++-v3/config/io/basic_file_stdio.cc 
b/libstdc++-v3/config/io/basic_file_stdio.cc
index e736701..eeb1e5e 100644
--- a/libstdc++-v3/config/io/basic_file_stdio.cc
+++ b/libstdc++-v3/config/io/basic_file_stdio.cc
@@ -195,11 +195,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 __basic_file* __ret = NULL;
 if (!this->is_open() && __file)
   {
-   int __err;
+   int __err, __save_errno = errno;
+   // POSIX guarantees that fflush sets errno on error, but C doesn't.
errno = 0;
do
- __err = this->sync();
+ __err = fflush(__file);
while (__err && errno == EINTR);
+   errno = __save_errno;
if (!__err)
  {
_M_cfile = __file;
diff --git a/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc 
b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
new file mode 100644
index 000..ba566f8
--- /dev/null
+++ b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
@@ -0,0 +1,39 @@
+// 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
+// .
+
+// { dg-require-fileio "" }
+
+#include 
+#include 
+#include 
+#include 
+
+void
+test01()
+{
+  FILE* f = std::fopen("79820.txt", "w");
+  std::fclose(f);
+  errno = 127;
+  __gnu_cxx::stdio_filebuf b(f, std::ios::out, BUFSIZ);
+  VERIFY(errno == 127); // PR libstdc++/79820
+}
+
+int
+main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/ext/stdio_filebuf/char/81751.cc 
b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/81751.cc
new file mode 100644
index 000..22191dcb
--- /dev/null
+++ b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/81751.cc
@@ -0,0 +1,53 @@
+// 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
+// .
+
+// { dg-require-fileio "" }
+
+#include