[v3] Fix second half of libstdc++/6745

[v3] Fix second half of libstdc++/6745

[Paolo Carlini]

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

Hi,

the below, tested x86-linux, approved by Benjamin, fixes the
interactive part of the PR.

Ciao, Paolo.

///////////

2002-11-18  Paolo Carlini  <pcarlini@unitus.it>

        PR libstdc++/6745 (continued)
        * include/bits/streambuf.tcc (__copy_streambufs):
        Deal with interactive input by using isatty as in the
        fix for libstdc++/8399.

[-- Attachment #2: patch_6745_int --]
[-- Type: text/plain, Size: 973 bytes --]

diff -urN libstdc++-v3-orig/include/bits/streambuf.tcc libstdc++-v3/include/bits/streambuf.tcc
--- libstdc++-v3-orig/include/bits/streambuf.tcc	2002-11-01 18:30:35.000000000 +0100
+++ libstdc++-v3/include/bits/streambuf.tcc	2002-11-17 20:51:40.000000000 +0100
@@ -37,6 +37,10 @@
 
 #pragma GCC system_header
 
+#ifdef _GLIBCPP_HAVE_UNISTD_H
+#include <unistd.h>
+#endif
+
 namespace std 
 {
   template<typename _CharT, typename _Traits>
@@ -219,8 +223,14 @@
 		}
  	      else 
 		{
-		  _CharT __buf[256];
-		  streamsize __charsread = __sbin->sgetn(__buf, sizeof(__buf));
+#ifdef _GLIBCPP_HAVE_ISATTY		  
+		  size_t __size = isatty(0) ? 1 : static_cast<size_t>(BUFSIZ);
+#else
+		  size_t __size = 1;
+#endif
+		  _CharT* __buf =
+		    static_cast<_CharT*>(__builtin_alloca(sizeof(_CharT) * __size));
+		  streamsize __charsread = __sbin->sgetn(__buf, __size);
 		  __xtrct = __sbout->sputn(__buf, __charsread);
 		  __ret += __xtrct;
 		  if (__xtrct != __charsread)

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

Paolo Carlini writes:
> the below, tested x86-linux, approved by Benjamin, fixes the
> interactive part of the PR.

> +#ifdef _GLIBCPP_HAVE_ISATTY		  
> +		  size_t __size = isatty(0) ? 1 : static_cast<size_t>(BUFSIZ);
> +#else
> +		  size_t __size = 1;
> +#endif

One problem with this.  __copy_streambufs is a general function -- it's not
just used to copy from stdin.  However, this patch conditionalizes the
function's buffer size on isatty(0), even though in many cases when it's
used, (i.e., copying from a readable buffer other than cin->rdbuf()), the
nature of stdin is irrelevant.

Is dynamic_cast<> allowed in libstdc++?  One possibility that might work for
the _GLIBCPP_HAVE_ISATTY block is something like this:
   size_t __size;
   basic_filebuf<_CharT, _Traits>* __fbin =
      dynamic_cast<basic_filebuf<_CharT, _Traits>*>(__sbin);
   if (__fbin != NULL && isatty(__fbin->_M_file.fd())) {
     __size = 1;
   }
   else {
     __size = static_cast<size_t>(BUFSIZ);
   }

This is just a sketch -- basic_filebuf::_M_file is protected, so a friend
declaration to __copy_streambufs would be needed.  Alternately,
basic_filebuf could acquire a public _M_is_tty() method.  (Would there be
binary compatibility issues with this?)

-- 
Jonathan Lennox
lennox at cs dot columbia dot edu

Re: [v3] Fix second half of libstdc++/6745

[Paolo Carlini]

Jonathan Lennox wrote:

>Paolo Carlini writes:
>  
>
>>+#ifdef _GLIBCPP_HAVE_ISATTY		  
>>+		  size_t __size = isatty(0) ? 1 : static_cast<size_t>(BUFSIZ);
>>+#else
>>+		  size_t __size = 1;
>>+#endif
>>    
>>
>One problem with this.  __copy_streambufs is a general function -- it's not
>just used to copy from stdin.  However, this patch conditionalizes the
>function's buffer size on isatty(0), even though in many cases when it's
>used, (i.e., copying from a readable buffer other than cin->rdbuf()), the
>nature of stdin is irrelevant.
>
Hummm...
Could you possibly post an example accompanying your argument (which is 
probably correct!)? I'd like to see some code which incorrectly leads to 
1 whereas BUFSIZ would be perfectly ok (and faster!).

Thanks, Paolo.

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

On Monday, November 18 2002, "Paolo Carlini" wrote to "Jonathan Lennox, gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org" saying:

> Jonathan Lennox wrote:

> >One problem with this.  __copy_streambufs is a general function -- it's not
> >just used to copy from stdin.  However, this patch conditionalizes the
> >function's buffer size on isatty(0), even though in many cases when it's
> >used, (i.e., copying from a readable buffer other than cin->rdbuf()), the
> >nature of stdin is irrelevant.
> >
> Hummm...
> Could you possibly post an example accompanying your argument (which is 
> probably correct!)? I'd like to see some code which incorrectly leads to 
> 1 whereas BUFSIZ would be perfectly ok (and faster!).

I'm pretty sure it can't happen with any of the standard streambuf classes
defined by the C++ standard, but it can easily happen for user-defined
streambuf classes.

As an example, consider test_buffer_1 from
libstdc++/testsuite/27_io/ostream_inserter_other.cc:
  class test_buffer_1 : public std::streambuf 
  {
  public:
    test_buffer_1(const std::string& s) : str(s), it(str.begin()) { }
  
  protected:
    virtual int underflow() { return (it != str.end() ? *it : EOF); }
    virtual int uflow() { return (it != str.end() ? *it++ : EOF); }

  private:
    const std::string str;
    std::string::const_iterator it;
  };

__copy_streambufs will loop once without your patch, but s.length() times
with it, if stdin is a tty.

An actual practical example is something like Matt Austern's syncstream,
which is attached to PR 8071.

-- 
Jonathan Lennox
lennox at cs dot columbia dot edu

Re: [v3] Fix second half of libstdc++/6745

[Paolo Carlini]

Jonathan Lennox wrote:

>I'm pretty sure it can't happen with any of the standard streambuf classes
>defined by the C++ standard, but it can easily happen for user-defined
>streambuf classes.
>
>As an example, consider test_buffer_1 from
>libstdc++/testsuite/27_io/ostream_inserter_other.cc:
>  class test_buffer_1 : public std::streambuf 
>  {
>  public:
>    test_buffer_1(const std::string& s) : str(s), it(str.begin()) { }
>  
>  protected:
>    virtual int underflow() { return (it != str.end() ? *it : EOF); }
>    virtual int uflow() { return (it != str.end() ? *it++ : EOF); }
>
>  private:
>    const std::string str;
>    std::string::const_iterator it;
>  };
>
>__copy_streambufs will loop once without your patch, but s.length() times
>with it, if stdin is a tty.
>
>An actual practical example is something like Matt Austern's syncstream,
>which is attached to PR 8071.
>
I want to study this in more detail but basically I believe you are 
right: we have a performance
issue in such cases. Could you possibly work out a patch along the lines 
which you suggested?
Not adding new public members to basic_filebuf, however...

Paolo

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

On Monday, November 18 2002, "Paolo Carlini" wrote to "Jonathan Lennox, gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org" saying:

> I want to study this in more detail but basically I believe you are 
> right: we have a performance issue in such cases. Could you possibly work
> out a patch along the lines which you suggested?
> Not adding new public members to basic_filebuf, however...

Is making __copy_streambufs a friend of basic_filebuf acceptable?

Also, I'm still not clear on whether it's acceptable to use dynamic_cast<>
in libstdc++.

-- 
Jonathan Lennox
lennox at cs dot columbia dot edu

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

On Monday, November 18 2002, "Paolo Carlini" wrote to "Jonathan Lennox, gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org" saying:

> I want to study this in more detail but basically I believe you are 
> right: we have a performance issue in such cases. Could you possibly work
> out a patch along the lines which you suggested?
> Not adding new public members to basic_filebuf, however...

Also note that because of the structure of this code, any cases which have a
performance issue here are cases which were segfaulting or going into an
infinite loop in any 3.x GCC prior to 3.2.1.  So it's not a crucial issue.

-- 
Jonathan Lennox
lennox at cs dot columbia dot edu

Re: [v3] Fix second half of libstdc++/6745

[Paolo Carlini]

Jonathan Lennox wrote:

>>I want to study this in more detail but basically I believe you are 
>>right: we have a performance issue in such cases. Could you possibly work
>>out a patch along the lines which you suggested?
>>Not adding new public members to basic_filebuf, however...
>>    
>>
>Is making __copy_streambufs a friend of basic_filebuf acceptable?
>
Probably...

>Also, I'm still not clear on whether it's acceptable to use dynamic_cast<>
>in libstdc++.
>
Nice question... In fact, in the current codebase there aren't, I'm not 
sure why, however.

Benjamin?

Ciao, Paolo.

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

On Tuesday, November 19 2002, "Paolo Carlini" wrote to "Jonathan Lennox, libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org, bkoz" saying:

> Jonathan Lennox wrote:
> 
> >>I want to study this in more detail but basically I believe you are 
> >>right: we have a performance issue in such cases. Could you possibly work
> >>out a patch along the lines which you suggested?
> >>Not adding new public members to basic_filebuf, however...
> >>    
> >>
> >Is making __copy_streambufs a friend of basic_filebuf acceptable?
> >
> Probably...
> 
> >Also, I'm still not clear on whether it's acceptable to use dynamic_cast<>
> >in libstdc++.
> >
> Nice question... In fact, in the current codebase there aren't, I'm not 
> sure why, however.

Is libstdc++ supposed to work in the presence of -fno-rtti on the command
line?

-- 
Jonathan Lennox
lennox at cs dot columbia edu

Re: [v3] Fix second half of libstdc++/6745

[Gabriel Dos Reis]

Jonathan Lennox <lennox@cs.columbia.edu> writes:

| > Nice question... In fact, in the current codebase there aren't, I'm not 
| > sure why, however.
| 
| Is libstdc++ supposed to work in the presence of -fno-rtti on the command
| line?

Right now, we've attempted to honor that flag (in conjunction with
-fno-exceptions).  I suppose we should continue to do so.  I'm a
little bit worried about the fragility of __copy_streambufs().

-- Gaby

Re: [v3] Fix second half of libstdc++/6745

[Paolo Carlini]

Gabriel Dos Reis wrote:

>| Is libstdc++ supposed to work in the presence of -fno-rtti on the command
>| line?
>
>Right now, we've attempted to honor that flag (in conjunction with
>-fno-exceptions).  I suppose we should continue to do so.
>
I see, thanks.

>I'm a
>little bit worried about the fragility of __copy_streambufs().
>
Me too :-(
OTOH, it seems to me that we should find a workable way to improve the 
performance
for the testcases proposed by Jonathan involving user-defined streambuf 
classes.

I'd like to work on this in the next week but I cannot anticipate how 
much progress I will
be able to make...

Ciao, Paolo.

Re: [v3] Fix second half of libstdc++/6745

[Gabriel Dos Reis]

Paolo Carlini <pcarlini@unitus.it> writes:

| OTOH, it seems to me that we should find a workable way to improve the
| performance
| for the testcases proposed by Jonathan involving user-defined
| streambuf classes.

Agreed.

-- Gaby

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

On Tuesday, November 19 2002, "Paolo Carlini" wrote to "Gabriel Dos Reis, Jonathan Lennox, libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org, bkoz" saying:

> Gabriel Dos Reis wrote:
> >I'm a little bit worried about the fragility of __copy_streambufs().
>
> Me too :-(
> OTOH, it seems to me that we should find a workable way to improve the 
> performance for the testcases proposed by Jonathan involving user-defined 
> streambuf classes.

A potentially much simpler solution occured to me.  Could we declare that
Paolo's original patch (the one that started this thread) only applies to
cin?  I.e., make the condition this:
    if (__sbin == cin->rdbuf() && isatty(0))

Would this work?  I'm not sure of all the potential details of reopening cin
-- is it possible for it to get disassociated from fd 0?

-- 
Jonathan Lennox
lennox at cs dot columbia dot edu

Re: [v3] Fix second half of libstdc++/6745

[Paolo Carlini]

Jonathan Lennox wrote:

>A potentially much simpler solution occured to me.  Could we declare that
>Paolo's original patch (the one that started this thread) only applies to
>cin?  I.e., make the condition this:
>    if (__sbin == cin->rdbuf() && isatty(0))
>
>Would this work?  I'm not sure of all the potential details of reopening cin
>-- is it possible for it to get disassociated from fd 0?
>
Hey Jonathan, during lunch time here in Italy the very *same* idea 
occurred to me!

Let's investigate it (and its variants) further!

Ciao for now, Paolo.

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

On Tuesday, November 19 2002, "Paolo Carlini" wrote to "Jonathan Lennox, libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org" saying:

> Jonathan Lennox wrote:
> 
> >A potentially much simpler solution occured to me.  Could we declare that
> >Paolo's original patch (the one that started this thread) only applies to
> >cin?  I.e., make the condition this:
> >    if (__sbin == cin->rdbuf() && isatty(0))
> >
> >Would this work?  I'm not sure of all the potential details of reopening cin
> >-- is it possible for it to get disassociated from fd 0?
> >
> Hey Jonathan, during lunch time here in Italy the very *same* idea 
> occurred to me!
> 
> Let's investigate it (and its variants) further!

The trick is going to be to get it to work properly for both cin and wcin,
while not breaking things for any other type that a user decides to use to
instantiate a basic_streambuf.

Here's a suggestion.  Have a template function

  template<typename _CharT, typename _Traits>
  bool __streambuf_is_stdin_and_tty(const basic_streambuf<_CharT, _Traits>*)
  {
    return false;
  }

And then declare the specializations

  template<> bool __streambuf_is_stdin_and_tty<char, traits<char> >(const streambuf*);
  template<> bool __streambuf_is_stdin_and_tty<wchar_t, traits<wchar_t> >(const wstreambuf*);

These specializations can do the actual appropriate checks, as we've
described previously.  They can live in ios.cc, so they can safely compare
the passed-in buffer with [w]cin_buf and then access [w]cin_buf.fd() to call
isatty() on it; this is significantly cleaner than the other variants of
these tests, imo.

This would be an ABI change, though -- it would add two new exported symbols
to libstdc++.  What's the policy on this?  It wouldn't hurt old binaries
dynamically linking with new libstdc++, but the opposite wouldn't work.

I'm not crazy about the name of the function, but I can't think of anything
better off-hand.  Maybe a name which asked "is this a streambuf which should
be read a character at a time when copying it?" would be better, for future
extensibility if we find other streambufs for which that's true.

Note: I don't have a copyright assignment on file, so this is probably
getting involved enough that I shouldn't be the one to write the code.

-- 
Jonathan Lennox
lennox at cs dot columbia dot edu

Re: [v3] Fix second half of libstdc++/6745

[Phil Edwards]

On Tue, Nov 19, 2002 at 03:44:41PM -0500, Jonathan Lennox wrote:
> This would be an ABI change, though -- it would add two new exported symbols
> to libstdc++.  What's the policy on this?  It wouldn't hurt old binaries
> dynamically linking with new libstdc++, but the opposite wouldn't work.

That's not a breaking change, though:  the symbol wouldn't be exported,
so new binaries wouldn't be looking for it in first place.

-- 
I would therefore like to posit that computing's central challenge, viz. "How
not to make a mess of it," has /not/ been met.
                                                 - Edsger Dijkstra, 1930-2002

Re: [v3] Fix second half of libstdc++/6745

[Jonathan Lennox]

On Tuesday, November 19 2002, "Phil Edwards" wrote to "Jonathan Lennox, Paolo Carlini, libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org" saying:

> On Tue, Nov 19, 2002 at 03:44:41PM -0500, Jonathan Lennox wrote:
> > This would be an ABI change, though -- it would add two new exported symbols
> > to libstdc++.  What's the policy on this?  It wouldn't hurt old binaries
> > dynamically linking with new libstdc++, but the opposite wouldn't work.
> 
> That's not a breaking change, though:  the symbol wouldn't be exported,
> so new binaries wouldn't be looking for it in first place.

Just to clarify why this is so:

The only users of the template specializations of my hypothetical
__streambuf_is_stdin_and_tty function would be the explicit instantiations
of __copy_streambufs<char> and __copy_streambufs<wchar_t> in
libstdc++-v3/src/streambuf-inst.cc.  Since these types are declared "extern
template", they won't be emitted in user code, and any instantiations of
__copy_streambufs for user types would pick up the generic (trivial) version
of __streambuf_is_stdin_and_tty from the header file.  Thus, the symbols for
the explicit specializations don't need to be exported.

Is that right?  It feels somewhat fragile to me.  In particular, it's a more
intimate dependency on the GCC-specific "extern template" extension than I
believe libstdc++ currently requires -- right now, it's only an optimization,
not something necessary for correctness.  (For example, currently, you can
copy libstdc++ header files to a local include directory and comment out the
"extern template" declarations, and get versions of the libstdc++ classes
with debugging information.)

-- 
Jonathan Lennox
lennox at cs dot columbia dot edu

Re: [v3] Fix second half of libstdc++/6745

[Benjamin Kosnik]

>A potentially much simpler solution occured to me.  Could we declare that
>Paolo's original patch (the one that started this thread) only applies to
>cin?  I.e., make the condition this:
>    if (__sbin == cin->rdbuf() && isatty(0))

Hey y'all. I think a better way to think about this is that the
streambuf is unbuffered. That way you don't have to deal with the isatty
issue. (For unbuffered streams, _M_buf_size_opt == 0)

If you do the checks based on that I think it will be cleaner. 

-benjamin

Re: [v3] Fix second half of libstdc++/6745

[Paolo Carlini]

Hi Benjamin and Jonathan and everyone!

>>A potentially much simpler solution occured to me.  Could we declare that
>>Paolo's original patch (the one that started this thread) only applies to
>>cin?  I.e., make the condition this:
>>   if (__sbin == cin->rdbuf() && isatty(0))
>>    
>>
>Hey y'all. I think a better way to think about this is that the
>streambuf is unbuffered. That way you don't have to deal with the isatty
>issue. (For unbuffered streams, _M_buf_size_opt == 0)
>
>If you do the checks based on that I think it will be cleaner.
>
Interesting, thanks.
I'm planning to work harder on this, following Jonathan ideas, in the 
next few days.

However I can already confirm that the idea basically *works*! I mean, a 
gross implementation
using reinterpret_cast<basic_streambuf<char/wchar_t>* > to compare 
__sbin to cin and wcin
(then or-ing the two) leads to to the efficient use of BUFSIZ in the 
safe situations: for instance,
Matt Austern testcase (that attached to 8071) runs about 3 time faster 
than with current mainline!

In my first quick attempts at coding Jonathan's nice ideas about 
__streambuf_is_stdin_and_tty
I had some troubles with redefinition errors at link time. I tried to 
follow closely the strategy
currently used for __copy_streambufs but without success, dunno why...
(I must admit that I don't know well the "extern template" extension...)

Ciao for now, Paolo.

Re: [v3] Fix second half of libstdc++/6745

[Gabriel Dos Reis]

Jonathan Lennox <lennox@cs.columbia.edu> writes:

| Here's a suggestion.  Have a template function
| 
|   template<typename _CharT, typename _Traits>
|   bool __streambuf_is_stdin_and_tty(const basic_streambuf<_CharT, _Traits>*)
|   {
|     return false;
|   }
| 
| And then declare the specializations
| 
|   template<> bool __streambuf_is_stdin_and_tty<char, traits<char> >(const streambuf*);
|   template<> bool __streambuf_is_stdin_and_tty<wchar_t, traits<wchar_t> >(const wstreambuf*);

This cannot work because being attached with a TTY is dyanmic property
of streambuf and not a static property -- that is one can rebind the
rdbuf() to another stream buffer.

-- Gaby

Re: [v3] Fix second half of libstdc++/6745

[Gabriel Dos Reis]

Benjamin Kosnik <bkoz@redhat.com> writes:

| >A potentially much simpler solution occured to me.  Could we declare that
| >Paolo's original patch (the one that started this thread) only applies to
| >cin?  I.e., make the condition this:
| >    if (__sbin == cin->rdbuf() && isatty(0))
| 
| Hey y'all. I think a better way to think about this is that the
| streambuf is unbuffered. That way you don't have to deal with the isatty
| issue. (For unbuffered streams, _M_buf_size_opt == 0)
| 
| If you do the checks based on that I think it will be cleaner. 

I would favor that approach, though I would have to rethink about it.

-- Gaby

Re: [v3] Fix second half of libstdc++/6745

[Gabriel Dos Reis]

Paolo Carlini <pcarlini@unitus.it> writes:

| Hi Benjamin and Jonathan and everyone!
| 
| >>A potentially much simpler solution occured to me.  Could we declare that
| >>Paolo's original patch (the one that started this thread) only applies to
| >>cin?  I.e., make the condition this:
| >>   if (__sbin == cin->rdbuf() && isatty(0))
| >>
| >Hey y'all. I think a better way to think about this is that the
| >streambuf is unbuffered. That way you don't have to deal with the isatty
| >issue. (For unbuffered streams, _M_buf_size_opt == 0)
| >
| >If you do the checks based on that I think it will be cleaner.
| >
| Interesting, thanks.
| I'm planning to work harder on this, following Jonathan ideas, in the
| next few days.
| 
| However I can already confirm that the idea basically *works*! I mean,
| a gross implementation
| using reinterpret_cast<basic_streambuf<char/wchar_t>* > to compare
| __sbin to cin and wcin

Please don't do that because that won't solve the problem, I think.

Basically, the thing is that being attached with a TTY is a *dynamic*
property that can potientially happen at any time to thingies that are
not just cin ou variants.

-- Gaby