[Bug libstdc++/45628] New: std::fstream::tellg invalidates I/O buffer

[Bug libstdc++/45628] New: std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

Querying the position in a file uses filebuf::seekoff( 0, ios::cur ). Although
the Standard specifies that this combination of parameters does not flush
putback or the output sequence, it does anyway, which results in a performance
hit. (§27.8.1.4/11)

Regression:
http://gcc.gnu.org/viewcvs/trunk/libstdc%2B%2B-v3/include/bits/fstream.tcc?r1=68163&r2=68420
(@@ -473,41 +486,26 @@)

This has been broken a long time!


-- 
           Summary: std::fstream::tellg invalidates I/O buffer
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: potswa at mac dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #1 from potswa at mac dot com  2010-09-10 07:09 -------
Created an attachment (id=21762)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21762&action=view)
simple testcase

This little program attempts to read three characters from its own source,
checking the position each time. Reading three bytes from buffered file should
only underflow once, right?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #2 from paolo dot carlini at oracle dot com  2010-09-10 10:55 -------
_M_terminate_output, correctly, does nothing in this case, cannot be the
problem, and there is nothing wrong wrt the standard mandated behavior. The
"problem" is that in our implementation, similarly to traditional C stdio
impls, reading and writing are completely separate operations, and the user
switches between the two with seeks, essentially. Any seek puts back the
internal status to "uncommitted" (_M_reading = false, _M_writing = false) and
afterwards the user can start *either* reading or writing, irrespective of the
previous history, and the seek logic doesn't know what will actually happen in
the future, of course. The user should not perform redundant seeks, because
they have a cost, they do something more than just "seeking". On the other
hand, a series of read or write operations has maximum performance, we don't
think we could possibly do better. Thus, I'm open to ideas, but I don't think
that within the current design one can change / improve much. Note that the
patch you linked is exactly the one implementing the above semantics.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #3 from paolo dot carlini at oracle dot com  2010-09-10 12:09 -------
I *think* it can work to minimally change what we have now to not reset the get
area buffers when (0, ios::cur) and we have been reading: as far as I can see,
if in that specific case we get back to reading again, the get area remains
completely valid indeed.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |paolo dot carlini at oracle
                   |dot org                     |dot com
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2010-09-10 12:11:41
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #4 from paolo dot carlini at oracle dot com  2010-09-10 12:20 -------
Does not work: when we reach the end of the buffer and we access again the file
to refill it, we start reading from the wrong position, the position we seeked
to.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #5 from paolo dot carlini at oracle dot com  2010-09-10 12:36 -------
To clarify: when we start reading in a buffered mode, the first underflow reads
the buffer and leaves the physical file at the first char beyond the buffer. If
we do afterwards a seek to the current reading position, belonging to the
buffer, the physical position along the file also changes of course, an
underlying fseek is performed. Then, if we don't refresh the buffer with a new
underflow, an inconsistency is born: the physical position along the file
doesn't correspond to the first char after the buffer and the next underflow
will read from the wrong position.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[don dot wakefield at gmail dot com]

------- Comment #6 from don dot wakefield at gmail dot com  2010-09-10 14:06 -------
Re: comment 5 - what is needed is for filebuf::seekoff(0,ios::cur) to:

  1) *not* invalidate the buffer
  2) *not* move the file pointer

since all that special case asks is "where am I in the 'logical' file?"

This can be accomplished by having filebuf::seekoff() recognize an off_type of
0, and a seekdir of cur, and special-case the code to call
_M_file.seekoff(0,cur) (not moving the file pointer, right?) and then adjusting
the resulting pos_type to reflect the true, earlier position held by gptr().

Am I missing something?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #7 from paolo dot carlini at oracle dot com  2010-09-10 14:39 -------
Then, seekoff would also return a position beyond the buffer, right? Or you
want it to return 1 anyway? Actually, I think the standard want us to use width
* off for the underlying fseek anyway, not only for off == 0, and this is not
what we have been doing. I think there is something seriously different here,
beyond the performance issue, which we should ponder much more, after so many
years.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[don dot wakefield at gmail dot com]

------- Comment #8 from don dot wakefield at gmail dot com  2010-09-10 14:54 -------
Paolo, yes, _M_file.seekoff(0,cur) would return the current physical file
position, and then filebuf::seekoff would adjust the returned pos_type to
reflect the position within the *logical* file, framed by the buffer and
pointed to by gptr().

As for the mechanics of width*off, I confess that locale issues leave me
completely befuddled, so I won't try to address that.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #9 from paolo dot carlini at oracle dot com  2010-09-10 15:00 -------
Ok. I don't think we should change the code to deal such specially with off ==
0, if we are going to change it we should decouple the return value from what
the underlying seek returns, and always call fseek(..., width * off, ...) as
the standard mandates. Then dealing with off == 0 becomes simple.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[don dot wakefield at gmail dot com]

------- Comment #10 from don dot wakefield at gmail dot com  2010-09-10 15:15 -------
(In reply to comment #9)
> Ok. I don't think we should change the code to deal such specially with off ==
> 0, if we are going to change it we should decouple the return value from what
> the underlying seek returns, and always call fseek(..., width * off, ...) as
> the standard mandates. Then dealing with off == 0 becomes simple.

I'm not sure I understand what you are saying. My concern is that calls to
filebuf::seekoff(0,ios::cur) should not invalidate the buffer just to return a
correct answer to "where am I?". Whether this is an accident of history or not,
I've seen this usage enshrined in code as a respected idiom.

Does your text above specify this?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #11 from paolo dot carlini at oracle dot com  2010-09-10 15:19 -------
Sure. What I meant - contrary to wait you said, I think - is that an elegant
and complete solution to this issue involves changing much more generally our
code to *always* behave as if fseek(off * width) were called, not just fseek(0)
in the special case you care about.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[don dot wakefield at gmail dot com]

------- Comment #12 from don dot wakefield at gmail dot com  2010-09-10 15:24 -------
(In reply to comment #11)
> Sure. What I meant - contrary to wait you said, I think - is that an elegant
> and complete solution to this issue involves changing much more generally our
> code to *always* behave as if fseek(off * width) were called, not just fseek(0)
> in the special case you care about.

Okay, I understand better now. My apologies, and thanks for your comments on
this issue.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #13 from paolo dot carlini at oracle dot com  2010-09-10 15:45 -------
Good, I think we are close to a fix, I'm already testing something. So, do we
have a symmetric issue with the put area or not? I'm not sure.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #14 from potswa at mac dot com  2010-09-10 15:59 -------
(In reply to comment #13)
> Good, I think we are close to a fix, I'm already testing something. So, do we
> have a symmetric issue with the put area or not? I'm not sure.

I believe so. tellg and tellp are both handled by seekoff( 0, ios::cur ), and
the required behavior doesn't differentiate between them, partly because for an
fstream the get and put pointers are the same. (The result doesn't depend on
the pointers, it comes from fseek.)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #15 from potswa at mac dot com  2010-09-10 16:15 -------
(In reply to comment #14)

> (The result doesn't depend on
> the pointers, it comes from fseek.)

I re-read Comment 5 and understand it this time ;v) . Well, any solution should
fix both tellg and tellp, since the pointers are the same upon synchronization.

Then, yes, we need to check which type of operation occurred last, and either
update whichever pointer is stale, or selectively use the correct one.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #16 from paolo dot carlini at oracle dot com  2010-09-10 17:11 -------
Actually, however, I don't think we can really always call fseek(off * width)
as the Standard want us to do. In a sense I'm happy because the change is gonna
be less invasive, on the other hand I'm a bit puzzled.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #17 from potswa at mac dot com  2010-09-10 17:25 -------
(In reply to comment #16)
> Actually, however, I don't think we can really always call fseek(off * width)
> as the Standard want us to do. In a sense I'm happy because the change is gonna
> be less invasive, on the other hand I'm a bit puzzled.

Could you post a patch? I'm not sure what you mean by generalizing (off *
width), if off == 0 then width is irrelevant. If width < 0 then the only valid
value for off is 0. If width > 0, off != 0 then repositioning is the primary
effect.

The task is to call fseek(0,cur), and then subtract the number of bytes in the
put area plus the "external characters," right?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #18 from paolo dot carlini at oracle dot com  2010-09-10 17:29 -------
I'm almost ready for the patch, please be patient ;) If look at the standard,
it says that the last step of seekoff is *always* as if calling fseek(..., off
* width, ...). If look at the current code, we have the concept of
__computed_off and, in many cases we end up calling the equivalent of fseek
with something != off * width. I'm changing that to (0, cur) for the case you
care about, but not changing anything else otherwise.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #19 from paolo dot carlini at oracle dot com  2010-09-10 17:30 -------
Of course here I'm always under the assumption width > 0.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]



------- Comment #20 from potswa at mac dot com  2010-09-10 17:35 -------
(In reply to comment #17)

> The task is to call fseek(0,cur), and then subtract the number of bytes in the
> put area plus the "external characters," right?

Er, I don't mean "bytes in the put area" exactly, but you know what I mean…
what I'm asking is, how does your simplification relate to the task of figuring
out how many file bytes the buffers hold, without flushing them, which the code
does not currently seem designed to do.

For regression, note that the code previous to the linked patch was

// NB: Need to do this in case _M_file in indeterminate
// state, ie _M_file._offset == -1
pos_type __tmp = _M_file.seekoff(__off, ios_base::cur, __mode);
if (__tmp >= 0)
  {
    // Seek successful.
    __ret = __tmp;
    __ret += std::max(this->_M_out_cur, this->_M_in_cur) 
             - _M_filepos;
  }

which does not appear to do multibyte compensation correctly. Was _M_filepos
the number of file bytes in whichever area was currently being used?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 933 bytes --]



------- Comment #21 from potswa at mac dot com  2010-09-10 17:40 -------
(In reply to comment #18)
> I'm almost ready for the patch, please be patient ;) If look at the standard,
> it says that the last step of seekoff is *always* as if calling fseek(..., off
> * width, ...). If look at the current code, we have the concept of
> __computed_off and, in many cases we end up calling the equivalent of fseek
> with something != off * width. I'm changing that to (0, cur) for the case you
> care about, but not changing anything else otherwise.

The standard says always to use (off * width, whence) but that is just the
external effect if buffering is transparent. __computed_off compensates for the
file pointer being necessarily different from gptr(), pptr(). (You can't seek
for every putc!)

Don't mean to be impatient, just trying to follow along the discussion…


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #22 from paolo dot carlini at oracle dot com  2010-09-10 17:42 -------
Good. Then I have a draft almost ready ;)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #23 from potswa at mac dot com  2010-09-10 18:53 -------
(In reply to comment #22)
> Good. Then I have a draft almost ready ;)

I have a very straightforward, low-impact solution, but I haven't tested it.
(My tree is pretty out of date.) Would you like to try it, if you're having
trouble?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #24 from paolo dot carlini at oracle dot com  2010-09-10 19:01 -------
Created an attachment (id=21768)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21768&action=view)
Draft

This is what I have so far, unfortunately I cannot work only on this today.
Anyway, it passes testing and this specific testcase, but is incomplete vs
wchar_t. If you have something which you are confident works fine for wchar_t
too, I can give it a try later today or over the next days, thanks.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #25 from potswa at mac dot com  2010-09-10 19:26 -------
Created an attachment (id=21769)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21769&action=view)
alternative approach. untested

I hope this compiles ;v) . But it seems to "color within the lines."

Why does your patch call setp/setg to (re?)invalidate the opposite area? And
then declares it is neither reading nor writing? Also, -1 return from _M_seek
is not handled in seekoff.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #26 from potswa at mac dot com  2010-09-10 19:30 -------
(In reply to comment #25)
> Created an attachment (id=21769)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21769&action=view) [edit]
> alternative approach. untested
> 
> I hope this compiles ;v) . But it seems to "color within the lines."

Bah, it doesn't. I missed an underscore at 

    vv
    __is_tell? __state_type(_M_state_last) : move(_M_state_last);
    ^^


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #27 from paolo dot carlini at oracle dot com  2010-09-10 19:31 -------
Note that certainly we don't want to use C++0x stuff here. Also, one thing at a
time of course, thus if we have been missing some error checking, etc, it's for
another time.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #28 from paolo dot carlini at oracle dot com  2010-09-10 19:34 -------
PS: you are right that we have to check that _M_seek succeeds before adding
back __computed_off.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #29 from paolo dot carlini at oracle dot com  2010-09-10 19:51 -------
And, please, if you want to help, manage to run the testsuite, we have got some
pretty nasty testcases ;)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]



------- Comment #30 from potswa at mac dot com  2010-09-10 20:33 -------
(In reply to comment #29)
> And, please, if you want to help, manage to run the testsuite, we have got some
> pretty nasty testcases ;)

I'll see if I can compile the latest… guess it's more useless to have an old
tree than one that merely doesn't compile.

Also, I failed to account for overflow() called from _M_terminate_output
flushing the put sequence and setting the file position to the actual location.
So my patch is buggy with tell after write. It appears that your patch will
still flush upon tell after write.

C++0x, easy enough to eliminate. I'm not aware of my patch fixing anything
besides the problem at hand…


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #31 from paolo dot carlini at oracle dot com  2010-09-11 04:27 -------
I'm afraid that the situation I outlined in Comment #5 is just the simple one.
The real problem with the new scheme - which tries to deal specially with (0,
cur) by not moving the file pointer - is when *writes* follow the seek. After a
while the buffer becomes full and must be flushed to the file starting at the
logical position corresponding to the previous seek. Thus - it seems to me -
the file pointer must be finally adjusted. How to do that without saving
anything in the filebuf? (note that within the current ABI we cannot add data
members)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #32 from potswa at mac dot com  2010-09-11 04:49 -------
(In reply to comment #31)
> I'm afraid that the situation I outlined in Comment #5 is just the simple one.
> The real problem with the new scheme - which tries to deal specially with (0,
> cur) by not moving the file pointer - is when *writes* follow the seek. After a
> while the buffer becomes full and must be flushed to the file starting at the
> logical position corresponding to the previous seek. Thus - it seems to me -
> the file pointer must be finally adjusted. How to do that without saving
> anything in the filebuf? (note that within the current ABI we cannot add data
> members)

I don't see how this is particularly difficult. A seekoff(0,ios_cur) operation
should only ever call lseek(0,SEEK_CUR). It does not point the file position
inside the buffer.

For simple byte-oriented case:

read, tell, write: egptr corresponds to to file marker. Tell finds logical
position using gptr-egptr. Write invalidates get area and starts fresh.

write, tell, write: pbase corresponds to file marker. Tell finds logical
position using pptr-pbase. Pointers and marker are still valid for next write.

To handle writes, we simply have to avoid calling _M_seek and overflow.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #33 from potswa at mac dot com  2010-09-11 04:56 -------
Note, I am not attempting to tell after write with a nontrivial codecvt
installed. Maybe the issue of Comment #5 is only in the general case?

I suppose it leaves UTF-8 files still a bit slow, but I still think that's
pretty well justified.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #34 from paolo dot carlini at oracle dot com  2010-09-11 09:21 -------
Run the full testsuite, and you will see. In general, if you simply do fseek(0,
cur) and then start writing, when eventually you have to flush you need the
actual logical position in the file - the last fseek(0, cur) - 'something' -
which is not available anywhere. I'm not saying it cannot be implemented, I'm
very dubious it can without breaking the ABI by adding an additional data
member, which we cannot do at the moment. To be honest I also don't think the
issue is very serious if only because nobody complained in 7 years, and we have
a lot to do for C++0x. Thus, if you can help with something concrete minimally
passing the testsuite and clearly addressing the concerns above, excellent,
otherwise, I cannot anticipate now when we are going to do something here. Just
to be honest.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #35 from potswa at mac dot com  2010-09-11 09:43 -------
(In reply to comment #34)
> Run the full testsuite, and you will see.

Lol, you're still looking at this too? I *just* got those pesky four testcases
done. I wasn't manually putting the codecvt state into the fpos in the special
case. I'll rerun the entire suite and post the patch here.

> In general, if you simply do fseek(0,
> cur) and then start writing, when eventually you have to flush you need the
> actual logical position in the file - the last fseek(0, cur) - 'something' -
> which is not available anywhere.

No, if fseek(0,cur) is implemented to have no side effects, it has NO side
effects. Nothing is lost. When the flush happens, it uses the logical position
obtained at the last flush, just as if fseek(0,cur) never occurred.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #36 from paolo dot carlini at oracle dot com  2010-09-11 10:03 -------
I'm traveling. Note, I don't understand how you are addressing my concerns,
thus whatever results you get from the testsuite, make sure we are not
regressing on the situation I outlined, thus write a new testcase reading in in
the buffer, say, 0123456789, then seeking to 0, reading consecutive positions
up to 5 via simple get, calling seekoff(0, cur), put x in the place of 5. Then
close, reopen, and check that you have 01234x6789.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #37 from paolo dot carlini at oracle dot com  2010-09-17 12:42 -------
Created an attachment (id=21819)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21819&action=view)
Tested x86_64-linux, mainline

This is a carefully tested patch (tested in mainline, per the normal policy,
where I also added two additional seekoff correctness testcases), which works
in limited circumstances (enough to fix the testcase, anyway) when I can
convince myself it's fully correct and consistent with our general framework.
My plan is committing it first and then possibly generalizing it, always
together with additional accompanying testcases, anyway.


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #21769|0                           |1
        is obsolete|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #38 from potswa at mac dot com  2010-09-17 17:51 -------
Created an attachment (id=21822)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21822&action=view)
Works with codecvt. Tested Tested x86_64-darwin, mainline

Ah, now I see the trick:

        if (__off == 0 && !(_M_mode & ios_base::out))

So if the file is open for writing, disable the optimization.

I had a problem with this condition for these testcases:

27_io/basic_filebuf/sputbackc/char/1-io.cc
27_io/basic_filebuf/sputbackc/char/2-io.cc

which contain code such as

    strmsz_2 = fb_01.sputn(", i wanna reach out and", 10);
    fb_01.pubseekoff(0, std::ios_base::cur); // if this doesn't flush
    c1 = fb_01.sgetc(); // this underflow is ignored
    c2 = fb_01.sputbackc('z'); // as well as this putback

Essentially, pubseekoff(0,cur) is being used as a sync(). I see nothing in the
Standard to support that, and indeed the sync() shouldn't be needed either, so
I was planning to open a new bug.

Anyway, if I apply your limitation to my patch, it passes the unmodified
testsuite, so here it is.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #39 from potswa at mac dot com  2010-09-17 18:04 -------
Oops, no, I'm on the 4.5.2 series, not mainline.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #40 from paolo dot carlini at oracle dot com  2010-09-17 18:53 -------
In general, our users know that seeking allows to switch from reading to
writing, and viceversa (when the stream has been appropriately opened of
course). This assumption remained true for years and years. Thus, for now at
least, I would rather not change it, whether the Standard is completely clear
in this area or not.

Also, I don't think the name __is_tell is appropriate, because of course this
kinf of situation in principle can occur also when tell is not involved (like
in your testcase ;)

Modulo the above comments, I think we can enable the optimization for codecvt
too, yes, let me reformat your other tweaks and more cleanly incorporate the
!(_M_mode & ios_base::out) thing. 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #41 from potswa at mac dot com  2010-09-17 19:04 -------
Well, I'm happy if you'd like to merge the diffs. My code was written with the
intent of optimizing the output case, too, but I guess it's not too inefficient
or awkward from the perspective of input only.

I just filed a bug http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45708 about the
requirement to separate reads and writes. I suppose it will be a duplicate, but
maybe it is worded a little better than existing bugs. Although your current
users might know the restriction, I don't think it's really well-documented. I
personally observed this behavior when getting started with iostreams, and it
was quite discouraging. GNU having a large part of the iostreams marketshare, I
wouldn't be surprised if this were a minor stumbling block to iostreams
adoption overall.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #42 from paolo dot carlini at oracle dot com  2010-09-17 19:10 -------
Before any other bug or analysis, I would recommend going back to the ton of
discussions in 2002 / 2003 when the design of basic_filebuf has been changed to
use _M_reading and _M_writing, **on purpose**. Didn't happen by chance, was a
deliberate redesign of the previous design which allowed major performance
improvements. And, to be clear, nobody complained anymore, *ever* all these
years. After you have analyzed those discussions (look in particular for Nathan
Myers and me), we can consider, for the future, alternate designs.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #43 from potswa at mac dot com  2010-09-17 19:11 -------
Thanks for the pointer, I'll read that.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #44 from paolo dot carlini at oracle dot com  2010-09-17 19:17 -------
By the way, if, for the future, you mean to contribute in these areas, if you
are really interested in these topics, I would recommend starting immediately
the Copyright assignment paperwork http://gcc.gnu.org/contribute.html send an
email to assignments@ 



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #45 from potswa at mac dot com  2010-09-17 19:21 -------
Already did copyright assignment :vP


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #46 from paolo dot carlini at oracle dot com  2010-09-17 19:26 -------
Ok, thanks.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #47 from paolo dot carlini at oracle dot com  2010-09-17 19:38 -------
To further clarify: what you have in mind isn't something which can belong to a
casual PR, is a major redesign of basic_filebuf, according to a different basic
philosophy, which at the time, Nathan called unified vs non-unified, if I
remember correctly. At the time we moved *away* from what you essentially want,
because we believed the new design to be superior in terms of performance.
Anyway, if you want to propose something different, or a variant of the old
design, please post messages to the libstdc++ mailing list, not here, remember
to involve Nathan in the discussions, benchmark in various circumstances the
various options, in particular, if you want something similar to the old scheme
make sure you are *improving* on it. Remember, in practice, that in the 7 years
since we moved to new scheme, **nobody ever** asked for the old behavior,
nobody complained about the performance of basic_filebuf, thus, if, in the
future, we are going to change it again, we really want to be sure to do it
after a **very** serious and public analysis.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #48 from potswa at mac dot com  2010-09-17 19:45 -------
I'm having trouble finding the discussion that precedes the June 24, 2003
redesign, but I'll add "unified" to the search terms.

Actually, last week I started some changes with the aim of fixing the bug, not
changing the philosophy. Rather than eliminate the state variables, just call
overflow and underflow in the cases that otherwise fail. So if I finish that,
and it performs well, I suppose I'll start an RFC.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #49 from paolo dot carlini at oracle dot com  2010-09-17 19:50 -------
It was **a ton** of work and discussions in public and among the maintainers,
in private. Anyway, if you have something which doesn't touch basic_streambuf,
keeps the get and put areas of basic_filebuf completely separate, with seeks
switching between reading and writing via state variables, then it's fine, in
principle.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

------- Comment #50 from potswa at mac dot com  2010-09-17 19:56 -------
(In reply to comment #49)
> with seeks
> switching between reading and writing via state variables, then it's fine, in
> principle.

Why require seeks? The whole point is that they are extraneous. I'm not
proposing to break code that calls seekoff, just to make it properly redundant.

I'm experimenting with switching states on any operation that requires it.
Overlapping get and put areas are obviously suicide; if that is what you
attempted before, then I'm encouraged about remaining alternatives. Outside the
context of the closed bug, I'm not proposing anything yet.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #51 from paolo dot carlini at oracle dot com  2010-09-17 20:07 -------
If you can allow writes after reads and viceversa *also* without seeks in the
middle, and without affecting performance and without adding data members,
that's fine. Let's see what you come up with it. By the way, get and put areas
never overlapped in the past, just look at the code in SVN, the movement of the
pointers was synced, true, exactly because one wanted each area to somehow
"know" what the other area was doing in order to "more easily" switch between
reads and writes without seeks. And nobody liked that scheme, was slow and
buggy. That happened way before I started contributing, for the record. Then
the new design came, outlined by Nathan, and simply inspired by C stdio,
nothing strange. As a matter of fact, many users found it also quite easy to
use, because - in case wasn't clear already from my previous comments -
**nobody ever** complained.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[don dot wakefield at gmail dot com]

------- Comment #52 from don dot wakefield at gmail dot com  2010-09-17 21:11 -------
(In reply to comment #51)
>  ...As a matter of fact, many users found it also quite easy to
> use, because - in case wasn't clear already from my previous comments -
> **nobody ever** complained.

I just have to butt in here. I don't expect any sympathy, but the only reason
my company never complained about the performance issue with
filebuf::seekoff(0,cur) is that we only just started using compilers which
included that change. Our company can't be the only one beholden to
conservative customers, so I won't be surprised if you get other questions
similar to ours, years after the change went in.

In any case, I appreciate all the effort you have put into this.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #53 from paolo dot carlini at oracle dot com  2010-09-17 21:22 -------
What can I say, I don't know anybody still using GCCs dating back to 2003. In
any case, my point wasn't really about seek(0, cur) and its optimization, etc,
my point was about the general design, where you use seeks to switch, you have
get and put areas completely independent, etc. As far as I know, **nobody**
asked to have the old behavior back, not in Bugzilla, not in the mailing lists,
nowhere in public discussions. Nobody **ever** commented **anywhere** that
using seeks to switch was unusual, entire Linux distros have been quickly
recompiled to use the new filebuf, we are now able to perform series of
consecutive get or put almost as fast as "C" getc_unlocked and putc_unlocked
(see the performance testsuite), etc. To summarize, I have nothing in principle
against speeding up this and that (of course) but I do not accept comments
implying that the current design is just wrong and should be changed to
something else without a careful analysis, benchmarks, a discussion on the
mailing list with all the experts involved, etc.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[potswa at mac dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]



------- Comment #54 from potswa at mac dot com  2010-09-17 21:51 -------
Well, for my part, a few years ago I played around with fstream a little,
noticed the tellg requirement was weird (I only discovered it by throwing the
kitchen sink at the problem), wasn't able to interpret the standard well enough
to confirm that GCC was wrong, and decided never to use that part of fstream.
It's not really a common use case anyway.

I think that a minority of people actually consider submitting bugs.

Companies are often very conservative with compilers and qualification, and are
less motivated to report something the further behind they fall. Under what
conditions does a manager actually decide to upgrade? If you're still using
2.95 and considering upgrading to 3.4, why bother reporting a bug against a
compiler which is already obsolete?

> but I do not accept comments
> implying that the current design is just wrong and should be changed to
> something else without a careful analysis, benchmarks, a discussion on the
> mailing list with all the experts involved, etc.

Changes should never occur without analysis one way or another. It's easy to
see that there is a bug… just look at the Standard's requirements on underflow.
As for whether this bug is a symptom of a "wrong design," I don't think so. I'm
getting the impression that you guys got tired after a long redesign process
and oversimplified the state machine.

While fixing bugs in the overambitious design with simultaneous get and put
areas (they might as well be overlapping as synchronized, both are impractical
and not the intent of the Standard), the decision was made to cut back
features, and non-explicit mode switches got lost.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #55 from paolo dot carlini at oracle dot com  2010-09-17 21:59 -------
>                                                                          I'm
> getting the impression that you guys got tired after a long redesign process
> and oversimplified the state machine.

Not me. What I remember is that Nathan Myers explained that C stdio, at least
traditionally worked exactly like that, and since Nathan *designed* parts of
the first C++ Standard itself, actively participated to all the meetings which
led to C++98, I trusted him by and large and found the new design
straightforward and well performing in most of out benchmarks. I still believe
he was quite right. Anyway, when you post something to the mailing list,
remember to add him in CC.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo dot carlini at oracle dot com]

------- Comment #56 from paolo dot carlini at oracle dot com  2010-09-20 21:32 -------
David himself is on it.


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|paolo dot carlini at oracle |unassigned at gcc dot gnu
                   |dot com                     |dot org
             Status|ASSIGNED                    |NEW


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo at gcc dot gnu dot org]

------- Comment #57 from paolo at gcc dot gnu dot org  2010-09-22 19:41 -------
Subject: Bug 45628

Author: paolo
Date: Wed Sep 22 19:40:43 2010
New Revision: 164529

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=164529
Log:
2010-09-22  David Krauss  <potswa@mac.com>

        PR libstdc++/45628
        * include/bits/fstream.tcc (basic_filebuf::underflow): Add state
        transition to avoid modality requiring seekoff(0,ios::cur).
        (basic_filebuf::pbackfail): Likewise.
        (basic_filebuf::overflow): Likewise.
        (basic_filebuf::_M_seek): Avoid minor unnecessary conversion.
        (basic_filebuf::seekoff): Remove code to _M_get_ext_pos; make
        (0, ios::cur) a special case preserving buffer contents.
        (basic_filebuf::_M_get_ext_pos): New function to obtain status
        about codecvt extern_t buffer for overflow and seekoff.
        * include/std/fstream (basic_filebuf::_M_get_ext_pos): Likewise.
        * config/abi/pre/gnu.ver: Export new symbols.
        * testsuite/27_io/basic_filebuf/seekoff/char/45628-1.cc: New,
        verifies that seekoff(0, ios::cur) preserves buffers.
        * testsuite/27_io/basic_filebuf/seekoff/char/45628-2.cc: Likewise.
        for codecvt case. More lenient as it may still flush put area.
        * testsuite/27_io/basic_filebuf/seekoff/char/4.cc: Modify to
        check that seekoff is not required between read and write.
        * testsuite/27_io/basic_filebuf/seekoff/wchar_t/4.cc: Likewise.
        * testsuite/27_io/basic_filebuf/sync/wchar_t/1.cc: Remove.
        * testsuite/27_io/basic_filebuf/sync/wchar_t/1.cc: Likewise.
        * testsuite/util/testsuite_character.h (codecvt::do_length): Comply
        with 22.2.1.5.2/10 "Returns ... the LARGEST value in the range..."

Added:
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/seekoff/45628-2.cc
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/seekoff/char/45628-1.cc
Removed:
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/sync/char/1.cc
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/sync/wchar_t/
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/config/abi/pre/gnu.ver
    trunk/libstdc++-v3/include/bits/fstream.tcc
    trunk/libstdc++-v3/include/std/fstream
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/seekoff/char/4.cc
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/seekoff/wchar_t/4.cc
    trunk/libstdc++-v3/testsuite/util/testsuite_character.h


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

[Bug libstdc++/45628] std::fstream::tellg invalidates I/O buffer

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45628

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |paolo.carlini at oracle dot
                   |                            |com
         Resolution|                            |FIXED
   Target Milestone|---                         |4.6.0

--- Comment #58 from Paolo Carlini <paolo.carlini at oracle dot com> 2010-09-25 11:09:20 UTC ---
Fixed for 4.6.0.

-- 
Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.