Bug 45841 - [4.6 Regression]: r164529 cris-elf libstdc++ 27_io/basic_filebuf/seekoff/char/2-io.cc
Summary: [4.6 Regression]: r164529 cris-elf libstdc++ 27_io/basic_filebuf/seekoff/char...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: 4.6.0
Assignee: Hans-Peter Nilsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-30 04:52 UTC by Hans-Peter Nilsson
Modified: 2010-10-07 22:14 UTC (History)
2 users (show)

See Also:
Host: x86_64-unknown-linux-gnu
Target: cris-axis-elf
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-10-04 23:59:13


Attachments
skip unnecessary filesystem seeks (547 bytes, patch)
2010-09-30 22:37 UTC, David Krauss
Details | Diff
patch for issue #2, lame check_v3_target_fileio (644 bytes, patch)
2010-10-05 01:44 UTC, Hans-Peter Nilsson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans-Peter Nilsson 2010-09-30 04:52:24 UTC
With revision r164528 this test passed.
From revision r164529 and on, this test has failed as follows:

Running /tmp/r164529/gcc/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp ...
... (non-regressions elided)
FAIL: 27_io/basic_filebuf/seekoff/char/2-io.cc execution test

With the message in the logfile being:

assertion "c1 == c3" failed: file "/tmp/hpautotest-gcc1/gcc/libstdc++-v3/testsuite/27_io/basic_filebuf/seekoff/char/2-io.cc", line 97, function: void test05()^M
program stopped with signal 6.^M
FAIL: 27_io/basic_filebuf/seekoff/char/2-io.cc execution test

Author of patch in suspect revision range CC:ed.
I can check a simulator trace if a code inspection doesn't ring you any bells.
Comment 1 David Krauss 2010-09-30 08:28:01 UTC
Well, that's almost certainly my fault, because that patch directly implements that testcase. However
Comment 2 David Krauss 2010-09-30 08:30:09 UTC
Well, that's almost certainly my fault, because that patch directly implements that testcase. However, I can't reproduce it, even after updating and rebuilding the compiler.

I'm new here, so… I suppose I need to figure out how to use that trace file next.
Comment 3 Paolo Carlini 2010-09-30 09:10:09 UTC
Please David and Hans-Peter, collaborate a bit on this, I can imagine David needs details because the problem can't be reproduced on many widespread targets, like Darwin or Linux. Indeed, the filebuf code has been changed, but only in the generic parts, thus I would really try to sort out first possible issues like code generation (what happens if the library is built -O0 instead of -O2?), undefined behavior (eg, uninitialized data) in the testcase itself, hidden dependency on target details in the testcase (eg, BUFSIZ), etc, because it seems highly unlikely to me that the generic code per se is incorrect, basing on this specific fail on this specific target only. By the way, is the first time one of the filebuf testcases fails unexpectedly on cris-axis-elf? I remember something puzzling in the past...
Comment 4 David Krauss 2010-09-30 09:52:48 UTC
Ah, I didn't even notice the target line before.

This is unbuffered I/O, which can be challenging to the codecvt. But this isn't a failure I would anticipate. If the codecvt fails, there should be an exception; if it succeeds, the result is cached so the second sgetc() returns the same result. (Thus, it's not *really* unbuffered.)

I don't see how the patch could change this particular behavior. Definitely need that trace. And I presume I'll need to upgrade gdb to use it. Which entails building GDB for that architecture from source, on my machine?

Or do you guys happen to have a setup I can ssh into? :v)
Comment 5 Paolo Carlini 2010-09-30 10:08:57 UTC
Challenging to the codecvt?!? Isn't this a *char* testcase?
Comment 6 David Krauss 2010-09-30 10:15:56 UTC
(In reply to comment #5)
> Challenging to the codecvt?!? Isn't this a *char* testcase?

Oh, my bad. I thought constraint_filebuf included that automatically. Guess not. I've got codecvts on the brain right now.

So yeah, there's really nothing remarkable at all about this testcase. Quite a random failure.

Are you still CC'd here Paolo?
Comment 7 Paolo Carlini 2010-09-30 10:24:49 UTC
After this message I will not be in CC anymore. I'm sorry, I'm getting too many distracting messages and cannot help much here anyway, at least not at this time.
Comment 8 Hans-Peter Nilsson 2010-09-30 11:46:47 UTC
(In reply to comment #4)
> Or do you guys happen to have a setup I can ssh into? :v)

Nominally, you *could* repeat my findings with a cross-compiler setup as described by <http://gcc.gnu.org/simtest-howto.html> but using that to investigate an execution failure might be a bit too challenging for a newcomer.

In my autotester using that kind of setup, this failure is a regression. There are certainly other failures (for different reasons; bugs/shortcomings in newlib, bugs in the test-cases etc.), but this one is a *regression*.  I guess from the earlier comments, your patch is rather exposing another issue, not the direct cause.  The trace I mentioned (I'll try to get to it this week) will show me at least the sequence of basic libc or system operations were used (seek, read, etc.) and I hope then you, by knowing the code around the patch, can help me by telling whether a specific call is sane or bogus, so we can pinpoint the failing code.  No, the trace can't be used with gdb, but only because that's a feature that's not yet been implemented. ;-)
Comment 9 David Krauss 2010-09-30 22:37:29 UTC
Created attachment 21926 [details]
skip unnecessary filesystem seeks

(In reply to comment #8)
> The trace I mentioned (I'll try to get to it this week) will
> show me at least the sequence of basic libc or system operations were used
> (seek, read, etc.) and I hope then you, by knowing the code around the patch,
> can help me by telling whether a specific call is sane or bogus

OK, the code path here is simple enough that just the I/O ops and results should provide everything.

The only difference I can find is that we now perform two consecutive seeks of offset zero, rather than one, when changing from reading to writing. I can fix that with a simple patch. Attached.

I somewhat doubt this will fix it, but if it's not more effort, then please use the patch for an additional test run. If it is a fix, then something is wrong with lseek().
Comment 10 Hans-Peter Nilsson 2010-09-30 22:55:20 UTC
(In reply to comment #9)
> I somewhat doubt this will fix it, but if it's not more effort, then please use
> the patch for an additional test run.

Thank you, testing in progress.  The least effort was to just throw it to the full build and test cycle, so it'll be a few hours, possibly interfered by some Z's.

> If it is a fix, then something is wrong
> with lseek().

That seems like an important clue, thanks.
Comment 11 Hans-Peter Nilsson 2010-10-01 03:15:40 UTC
(In reply to comment #9)
> I somewhat doubt this will fix it, but if it's not more effort, then please use
> the patch for an additional test run.

That patch made no difference to the results.
Comment 12 Paolo Carlini 2010-10-01 12:14:55 UTC
David, if you think your patchlet is an improvement anyway, please post it to the mailing list with a proper ChangeLog entry (per the aforementioned general rule of making progress in small steps, the most uncontroversial ones first)
Comment 13 David Krauss 2010-10-01 12:19:26 UTC
(In reply to comment #12)

I haven't decided on that. It's a performance improvement but rather ugly. I was thinking, if lseek(0,cur) can alter the behavior of a filesystem, it must not be that fast. But it didn't, so I'm not really inclined to pursue this further.
Comment 14 Paolo Carlini 2010-10-01 16:31:05 UTC
Ok..
Comment 15 Hans-Peter Nilsson 2010-10-04 23:51:54 UTC
(In reply to comment #9)
> ... something is wrong
> with lseek().

It certainly is!  There are at least three issues here.

1. The simulator has a bug: on a 64-bit host, the offset parameter to the lseek call is in-effect zero-extended from 32-bit-long to 64-bit-long.  This causes any change to the sequence of lseeks to show random regressions.

2. There is a test-suite hook, libstdc++-v3/testsuite/lib/libstdc++.exp:check_v3_target_fileio that should catch this.

3. Fixing #1 certainly shows improvement; 14 additional tests pass, but also 2 regressions, so there's still another issue...

Due to (#1 and) #2, I'm recategorizing this PR as belonging to the testsuite and I'm assigning it to myself, pending a patch for #2 and investigating #3.
Sorry for the noise, feel free to remove yourself from CC if this made you lose interest. :)

For the record, an strace excerpt of the simulator before and after r164529 shows the issue better than the actual simulator trace I had in mind.  (Though it also shows that the failing ltrace call is an addition, and I guess you would want to eliminate it, if possible.  Oops, four issues!  Maybe that was what the attached patch was about...)

r164528:
...
open("seekoff-2io.tst", O_RDWR)         = 4
fstat(4, {st_mode=S_IFREG|0644, st_size=116, ...}) = 0
lseek(4, 0, SEEK_CUR)                   = 0
lseek(4, 2, SEEK_SET)                   = 2
fstat(4, {st_mode=S_IFREG|0644, st_size=116, ...}) = 0
lseek(4, 0, SEEK_CUR)                   = 2
read(4, " ", 1)                         = 1
read(4, "9", 1)                         = 1
lseek(4, 3, SEEK_SET)                   = 3
write(4, "\n", 1)                       = 1
lseek(4, 4, SEEK_SET)                   = 4
read(4, "9", 1)                         = 1
lseek(4, 1, SEEK_CUR)                   = 6
read(4, "1", 1)                         = 1
read(4, "1", 1)                         = 1
lseek(4, 4294967295, SEEK_CUR)          = 4294967303
write(4, "x", 1)                        = 1
write(4, "\n", 1)                       = 1
lseek(4, 0, SEEK_CUR)                   = 4294967305
read(4, "", 1)                          = 0
read(4, "", 1)                          = 0
lseek(4, 0, SEEK_END)                   = 4294967305
write(4, "\n", 1)                       = 1
write(4, "because because because. . .", 28) = 28
lseek(4, 4294967295, SEEK_END)          = 8589934629
read(4, "", 1)                          = 0
lseek(4, 4294967295, SEEK_CUR)          = 12884901924
read(4, "", 1)                          = 0
read(4, "", 1)                          = 0
read(4, "", 1)                          = 0
close(4)                                = 0
...

r164529:
...
open("seekoff-2io.tst", O_RDWR)         = 4
fstat(4, {st_mode=S_IFREG|0644, st_size=116, ...}) = 0
lseek(4, 0, SEEK_CUR)                   = 0
lseek(4, 2, SEEK_SET)                   = 2
fstat(4, {st_mode=S_IFREG|0644, st_size=116, ...}) = 0
lseek(4, 0, SEEK_CUR)                   = 2
read(4, " ", 1)                         = 1
read(4, "9", 1)                         = 1
lseek(4, 3, SEEK_SET)                   = 3
write(4, "\n", 1)                       = 1
lseek(4, 4, SEEK_SET)                   = 4
read(4, "9", 1)                         = 1
lseek(4, 1, SEEK_CUR)                   = 6
read(4, "1", 1)                         = 1
read(4, "1", 1)                         = 1
lseek(4, 0, SEEK_CUR)                   = 8
lseek(4, 4294967295, SEEK_CUR)          = 4294967303
write(4, "x", 1)                        = 1
write(4, "\n", 1)                       = 1
lseek(4, 0, SEEK_CUR)                   = 4294967305
read(4, "", 1)                          = 0
read(4, "", 1)                          = 0
write(2, "assertion \"", 11assertion ")            = 11
write(2, "c1 == c3", 8c1 == c3)                 = 8
write(2, "\" failed: file \"", 16" failed: file ")      = 16
...
Comment 16 David Krauss 2010-10-05 00:24:35 UTC
(In reply to comment #15)
> r164528:
> ...
> read(4, "1", 1)                         = 1
> lseek(4, 4294967295, SEEK_CUR)          = 4294967303
> write(4, "x", 1)                        = 1
> write(4, "\n", 1)                       = 1
> lseek(4, 0, SEEK_CUR)                   = 4294967305
> read(4, "", 1)                          = 0
> read(4, "", 1)                          = 0
> lseek(4, 0, SEEK_END)                   = 4294967305
> ...
> 
> r164529:
> ...
> read(4, "1", 1)                         = 1
> lseek(4, 0, SEEK_CUR)                   = 8
> lseek(4, 4294967295, SEEK_CUR)          = 4294967303
> write(4, "x", 1)                        = 1
> write(4, "\n", 1)                       = 1
> lseek(4, 0, SEEK_CUR)                   = 4294967305
> read(4, "", 1)                          = 0
> read(4, "", 1)                          = 0
> write(2, "assertion \"", 11assertion ")            = 11

The failure had nothing to do with the additional seek. The excerpts above show that the added operation didn't move the file pointer… the failure occurred at the same position as success before the patch.

What the testcase ends up doing is extending the file and then reading past EOF. That's not illegal behavior and the case as written should still pass after comparing EOF == EOF, just as it did before my patch.

Hmm, yep, caught a bug in my patch! Thanks!
Comment 17 Hans-Peter Nilsson 2010-10-05 01:44:38 UTC
Created attachment 21962 [details]
patch for issue #2, lame check_v3_target_fileio

Patch-in-progress, pending investigation of issue #3, which might require further adjustments.

The check_v3_target_fileio gate now fails with unfixed simulator, passes with a fixed one.  Also returns 0 with a manual sanity-check just using g++ on host, on the generated source file.
Comment 18 Hans-Peter Nilsson 2010-10-05 01:58:45 UTC
(In reply to comment #16)
> What the testcase ends up doing is extending the file and then reading past
> EOF. That's not illegal behavior and the case as written should still pass
> after comparing EOF == EOF, just as it did before my patch.
> 
> Hmm, yep, caught a bug in my patch! Thanks!

Um... let me see if I got this right: the simulator with a buggy lseek caught a bug in your patch not seen in a correctly working environment?  (That could actually be issue #3.)

If so, I think that makes the top 5 weird bug behaviors in my experience!
(Ok, only some 22 years, nothing to speak of.)

Don't forget that for the new patch, I think we need a separate new regression test-case, one that "works" without relying on buggy simulators. :-D
Comment 19 David Krauss 2010-10-05 02:58:04 UTC
(In reply to comment #18)
> Um... let me see if I got this right: the simulator with a buggy lseek caught a
> bug in your patch not seen in a correctly working environment?  (That could
> actually be issue #3.)

Yep, accidentally seeking off the end causes read() to fail. Trying to read from the streambuf after a write causes a mode switch and virtual flush, which succeeds. I accidentally set the return error value variable to success based on flush success, causing getc() not to return EOF despite failure. The subsequent read did not have a flush and correctly returned EOF. Apparently reading after a write at EOF is not in the tests.

> If so, I think that makes the top 5 weird bug behaviors in my experience!
> (Ok, only some 22 years, nothing to speak of.)
> 
> Don't forget that for the new patch, I think we need a separate new regression
> test-case, one that "works" without relying on buggy simulators. :-D

Yeah lol, I don't know if the regression case is really necessary, but I suppose I should work it in somewhere. Should I reference this bug in regards to such a change, or does that make more work for you?

Do you mean 22 years of life or 22 years in the field ;v) … I think this is just serendipitous. Today is my 26th birthday, so you're either making me feel old or just experienced… I guess cutting my teeth on old Classic Mac OS and Apple Open Firmware (a fairly large Forth operating system, also deceased) gave me a fair share of weirdness, nothing has really surprised me for years… but I did quit software for a while in the meantime and this is my first return to operating systems…
Comment 20 Hans-Peter Nilsson 2010-10-05 04:02:00 UTC
(In reply to comment #19)
> Apparently
> reading after a write at EOF is not in the tests.

Good you noticed.

> Yeah lol, I don't know if the regression case is really necessary, but I
> suppose I should work it in somewhere.

It's a requirement for changes to the code-base.  The thinking goes, if you have a patch for a bug, you need to a) prove it, and b) make sure the fix doesn't get undone by later patches.

> Should I reference this bug in regards
> to such a change, or does that make more work for you?

Referencing would be slightly better than doing it from scratch, methinks.
I don't think I'm doing any extra work here.

> Do you mean 22 years of life or 22 years in the field ;v)

Um, in the field...  It's actually somewhat more, but never mind.

> … I think this is
> just serendipitous. Today is my 26th birthday, so you're either making me feel
> old or just experienced…

Not sure I follow your thinking there, but Happy Birthday.  Consider this bug report an early gift. :]
Comment 21 Paolo Carlini 2010-10-05 09:10:53 UTC
Let's rerecategorize this, then. David, I think it's perfectly fine adjusting also the generic code as part of resolving this PR (thus mentioning it in the ChangeLog entry) without actually closing it (which will happen when the target bits will be committed too). And yes, a testcase for that would be great.
Comment 22 Hans-Peter Nilsson 2010-10-05 17:03:09 UTC
Just a heads-up regarding issue #3.

(In reply to comment #19)
> Apparently
> reading after a write at EOF is not in the tests.

Hm, doesn't
27_io/basic_filebuf/sputbackc/char/9425.cc
test something like that, or at leas EOF after?  It doesn't fail for you?

It does for me (cris-axis-elf), seen as a regression with a fixed simulator.
Or maybe that's not the reason?  Ok, I'll look closer.

FWIW, due to <http://subversion.tigris.org/issues/show_bug.cgi?id=2333>
I also saw 27_io/basic_filebuf/sync/wchar_t/1.cc (a file removed in r164529) as regressing, which had me confused until I STFW and found that svn up to 1.7.0 (version according to the bug report, but I can confirm it includes 1.6.9) did not include (files in) deleted directories in diffs, like "svn diff -c164529". Note that sync/char/1.cc is included.

Looks like lots of libstdc++ file I/O test-cases are missing "// { dg-require-fileio "" }" lines.  I'll add them as obvious.
Comment 23 Paolo Carlini 2010-10-05 17:06:27 UTC
(In reply to comment #22)
> Hm, doesn't
> 27_io/basic_filebuf/sputbackc/char/9425.cc
> test something like that, or at leas EOF after?  It doesn't fail for you?

Just have a look to testresults: clean testsuite at least for x86_64-linux, both -m32 and -m64. I understand Darwin too.
Comment 24 David Krauss 2010-10-05 17:24:22 UTC
(In reply to comment #22)
> Just a heads-up regarding issue #3.
> 
> (In reply to comment #19)
> > Apparently
> > reading after a write at EOF is not in the tests.
> 
> Hm, doesn't
> 27_io/basic_filebuf/sputbackc/char/9425.cc
> test something like that, or at leas EOF after?  It doesn't fail for you?

The required sequence is a write up to EOF followed by a read. 9425 is a putback at the beginning of the file, which is neither.

> It does for me (cris-axis-elf), seen as a regression with a fixed simulator.
> Or maybe that's not the reason?  Ok, I'll look closer.
> 
> FWIW, due to <http://subversion.tigris.org/issues/show_bug.cgi?id=2333>
> I also saw 27_io/basic_filebuf/sync/wchar_t/1.cc (a file removed in r164529) as
> regressing, which had me confused until I STFW and found that svn up to 1.7.0
> (version according to the bug report, but I can confirm it includes 1.6.9) did
> not include (files in) deleted directories in diffs, like "svn diff -c164529".
> Note that sync/char/1.cc is included.

Did I follow the wrong process for deleting the file? Hmm, I'm using svn 1.6.5. The reason it was removed was that it verified the very behavior the patch was designed to eliminate. And no way to adapt it to do something useful. We will, presumably, re-add that directory with some other kind of test though.

> Looks like lots of libstdc++ file I/O test-cases are missing "// {
> dg-require-fileio "" }" lines.  I'll add them as obvious.

Good to know, thanks!
Comment 25 David Krauss 2010-10-05 17:26:18 UTC
> The required sequence is a write up to EOF followed by a read. 9425 is a
> putback at the beginning of the file, which is neither.

A putback at the beginning will attempt a backwards seek, though, which is exactly what you're looking at fixing.
Comment 26 Hans-Peter Nilsson 2010-10-05 18:45:52 UTC
(In reply to comment #24)
> Did I follow the wrong process for deleting the file?

No, the commit was fine, it's the "svn diff" that fails.

(In reply to comment #25)
> A putback at the beginning will attempt a backwards seek, though, which is
> exactly what you're looking at fixing.

But... again, this is with the *fixed* simulator.
Whatever: I had a look and the difference is:

(unfixed simulator)
open("filebuf_virtuals-1.txt", O_RDONLY|O_NONBLOCK) = 4
lseek(4, 4294967295, SEEK_CUR)          = 4294967295
read(4, "", 1023)                       = 0
close(4)                                = 0
(return with 0)

(fixed simulator)
open("filebuf_virtuals-1.txt", O_RDONLY|O_NONBLOCK) = 4
lseek(4, -1, SEEK_CUR)                  = -1 EINVAL (Invalid argument)
read(4, "// 990117 bkoz\n// test functiona"..., 1023) = 1023
close(4)                                = 0
write(2, "assertion \"", 11assertion ")            = 11
(failed assertion)

On a host (using gcc-4.4.3 and library), I don't get to the "read":
open("filebuf_virtuals-1.txt", O_RDONLY) = 3
lseek(3, -1, SEEK_CUR)                  = -1 EINVAL (Invalid argument)
close(3)                                = 0

So, it's (error/EOF) file state state that's lost; looks like I have to amend the check_v3_target_fileio some more and fix another simulator bug.  Film at 11.
Comment 27 Hans-Peter Nilsson 2010-10-05 18:59:00 UTC
(In reply to comment #26)
> looks like I have to amend
> the check_v3_target_fileio some more and fix another simulator bug.  Film at
> 11.

JFTR, the simulator part (the wrap function records errno for passing on to the target):

--- sim/common/callback.c.orig	Wed Jan 14 12:09:55 2009
+++ sim/common/callback.c	Tue Oct  5 20:56:21 2010
@@ -278,7 +278,7 @@ os_lseek (p, fd, off, way)
   result = fdbad (p, fd);
   if (result)
     return result;
-  result = lseek (fdmap (p, fd), off, way);
+  result = wrap (p, lseek (fdmap (p, fd), off, way));
   return result;
 }
Comment 28 David Krauss 2010-10-05 19:35:17 UTC
(In reply to comment #26)

> lseek(4, -1, SEEK_CUR)                  = -1 EINVAL (Invalid argument)
> read(4, "// 990117 bkoz\n// test functiona"..., 1023) = 1023
> 
> So, it's (error/EOF) file state state that's lost; looks like I have to amend
> the check_v3_target_fileio some more and fix another simulator bug.  Film at
> 11.

I'm a little confused. Is that supposed to be a sticky state? The file is trying to seek past the beginning, not past the end, so the read should succeed, right?

The program should terminate immediately without reading after the seek fails. 

We have basic_file_stdio.cc

    return lseek(this->fd(), __off, __way);

returns to seekoff in fstream.tcc

 	      off_type __file_off = _M_file.seekoff(0, ios_base::cur);
 	      if (__file_off != off_type(-1))
... escape scopes
      return __ret; // only ever assigned pos_type(off_type(-1))

returns to pbackfail in fstream.tcc

	  else if (this->seekoff(-1, ios_base::cur) != pos_type(off_type(-1)))
            {
              __tmp = this->underflow(); // read 1024, SHOULDN'T GET HERE
	  else return __ret; // equal to EOF

So the code path looks sterling to me, yet it reads 1024.

Perhaps the fixed simulator is now reporting a 32-bit return value from lseek but the the program is actually using a 64-bit, zero-extended value that doesn't indicate failure?
Comment 29 Paolo Carlini 2010-10-05 20:39:11 UTC
Can I ask a simple question? At this point in the analysis, do we have a testcase failing on Linux or Darwin, thus showing a clear regression in the generic code? If the answer is yes, do we believe a fix is reasonably simple? Otherwise, if the situation is too confused maybe we have to humbly take out the improvements to filebuf, allow HP to fix the other issues on the target, get back to zero fails there, and on top of that clean status reapply the filebuf changes.
Comment 30 David Krauss 2010-10-05 21:16:01 UTC
Also, I think we need to clarify whether 9425.cc is a regression. That was an expected failure before my revision, right?
Comment 31 Hans-Peter Nilsson 2010-10-05 21:26:23 UTC
(In reply to comment #28)
> I'm a little confused. Is that supposed to be a sticky state?

Nope.  Don't worry.  The patch I quoted just fixes a bug on the return path from the host file i/o call, which messed up the return value so the lseek syscall (as seen from the simulated syscall) returns -0 (just the expression, no sign-magnitude machine here :) instead of -EINVAL in Linux syscall parlance.
Comment 32 Hans-Peter Nilsson 2010-10-05 21:40:39 UTC
(In reply to comment #29)
> Otherwise, if the situation is too confused maybe we have to humbly take out
> the improvements to filebuf, allow HP to fix the other issues on the target,
> get back to zero fails there, and on top of that clean status reapply the
> filebuf changes.

Thanks for caring but FWIW, my work would not be helped by backing out any changes; I do all the work at a fix set of revisions, followed up if needed (rarely) at a later revision.  I certainly did not have zero fails before, but they will be much much fewer after this PR is closed!

before, at r164528:
                === libstdc++ Summary ===

# of expected passes            6518
# of unexpected failures        18
# of unexpected successes       1
# of expected failures          80
# of unsupported tests          496

at r164529, with one of the two found simulator bugs fixed and still with the one mentioned spurious fail due to "svn diff" failing (i.e. really down to 4):
                 === libstdc++ Summary ===

# of expected passes            6532
# of unexpected failures        6
# of unexpected successes       1
# of expected failures          80
# of unsupported tests          496
Comment 33 Paolo Carlini 2010-10-05 21:57:26 UTC
(In reply to comment #32)
> Thanks for caring but FWIW, my work would not be helped by backing out any
> changes; I do all the work at a fix set of revisions, followed up if needed
> (rarely) at a later revision.  I certainly did not have zero fails before, but
> they will be much much fewer after this PR is closed!

Good, I see you are doing an excellent work for your target. Thus, just let me know when / if you come to the conclusion that there are real issues in the generic library code, because this is an extremely crucial facility, and we want to be 100% sure we are not regressing for 4.6.0, even if that means not making progress.
Comment 34 David Krauss 2010-10-05 22:47:14 UTC
Posted a patch to fix my end of this, and a regression to verify that fix on working systems.

http://gcc.gnu.org/ml/libstdc++/2010-10/msg00015.html
Comment 35 Hans-Peter Nilsson 2010-10-05 23:16:37 UTC
(In reply to comment #30)
> Also, I think we need to clarify whether 9425.cc is a regression. That was an
> expected failure before my revision, right?

Yes and no.  By fixing one of the two (known :) simulator bugs and running the test-suite for r164529, the result of that test regressed from PASS to FAIL.
Comment 36 Hans-Peter Nilsson 2010-10-05 23:29:32 UTC
(In reply to comment #33)
> (In reply to comment #32)
> > Thanks for caring but FWIW, my work would not be helped by backing out any
> > changes; I do all the work at a fix set of revisions, followed up if needed
> > (rarely) at a later revision.  I certainly did not have zero fails before, but
> > they will be much much fewer after this PR is closed!
> 
> Good, I see you are doing an excellent work for your target. Thus, just let me
> know when / if you come to the conclusion that there are real issues in the
> generic library code, because this is an extremely crucial facility, and we
> want to be 100% sure we are not regressing for 4.6.0, even if that means not
> making progress.

To summarize the comments above, the real issues I know of at r164529 are:
1) an extra lseek (compared to r164529) for 27_io/basic_filebuf/seekoff/char/2-io.cc
2) erroneous behavior that David found, when reading past the end-of-file

(In reply to comment #34)
> Posted a patch to fix my end of this, and a regression to verify that fix on
> working systems.
> 
> http://gcc.gnu.org/ml/libstdc++/2010-10/msg00015.html

<nitpick>
David, regarding contracting the expression "regression test" into "regression": Don't.  It changes the meaning in a bad way: you add a "regression *test*" not a "regression".  I hope; at least eventually. :)

"Hey guys, I just committed a regression."
"Revert immediately and investigate in your local tree."
</nitpick>
Comment 37 David Krauss 2010-10-05 23:31:34 UTC
(In reply to comment #35)
> Yes and no.  By fixing one of the two (known :) simulator bugs and running the
> test-suite for r164529, the result of that test regressed from PASS to FAIL.

Is it ever a regression (using the proper sense of the word ;v) ) vs my patch?
Comment 38 Paolo Carlini 2010-10-05 23:34:37 UTC
> To summarize the comments above, the real issues I know of at r164529 are:
> 1) an extra lseek (compared to r164529) for
> 27_io/basic_filebuf/seekoff/char/2-io.cc
> 2) erroneous behavior that David found, when reading past the end-of-file

Ok, thanks for the summary. Thus it looks like we are in good shape (David said in a previous message that 1) isn't really worth optimizing for, if I remember correctly).
 
> (In reply to comment #34)
> > Posted a patch to fix my end of this, and a regression to verify that fix on
> > working systems.
> > 
> > http://gcc.gnu.org/ml/libstdc++/2010-10/msg00015.html

Good, I'm going to apply it here, give it a spin and commit it if everything looks fine.
 
> <nitpick>
> David, regarding contracting the expression "regression test" into
> "regression": Don't.  It changes the meaning in a bad way: you add a
> "regression *test*" not a "regression".  I hope; at least eventually. :)

I must say I totally agree. More substantively, please, at your ease, figure out a more complete and independent testcase. The normal rule is: one bug report, one testcase.
Comment 39 David Krauss 2010-10-05 23:37:55 UTC
> Good, I'm going to apply it here, give it a spin and commit it if everything
> looks fine.

Please hold off for a minute. I screwed that patch up about as much as it's possible for something so simple; although there's nothing damaging in there I might as well include the *intended* change set, fix the logfile, and properly implement the regression test.

I'll get the hang soon, it's been a while since I did any collaborative development.
Comment 40 Paolo Carlini 2010-10-05 23:40:56 UTC
Ok, no problem. While we are at it, I would recommend not including large formatting / indentation fixes, are hising too much the substance of the work. For a later patch...
Comment 41 David Krauss 2010-10-05 23:47:00 UTC
(In reply to comment #40)
> Ok, no problem. While we are at it, I would recommend not including large
> formatting / indentation fixes, are hising too much the substance of the work.
> For a later patch...

D'oh, I just sent you another one including that. My thinking is that it's not good to include substantive changes inside an indentation correction, because you can't see them. That code could use a few more changes eventually, so the indentation really does need to be fixed or it will keep getting less consistent with every patch.
Comment 42 Paolo Carlini 2010-10-05 23:48:52 UTC
No problem for this time. But the general rule stays: do not hide substantive changes in the middle of large cosmetic changes. Just send separate patches.
Comment 43 Hans-Peter Nilsson 2010-10-05 23:52:03 UTC
(In reply to comment #37)
> (In reply to comment #35)
> > Yes and no.  By fixing one of the two (known :) simulator bugs and running the
> > test-suite for r164529, the result of that test regressed from PASS to FAIL.
> 
> Is it ever a regression (using the proper sense of the word ;v) ) vs my patch?

I think we draw the line at committed revisions, so: no.
I just mentioned them as to clarify what I meant by issue #3 above.

(I haven't committed any of the mentioned simulator fixes yet; I haven't run the simulator test-suite.  At least now I've written two separate regression tests for those bugs and verified "manually", i.e. outside the test framework for src/sim, that they individually fail. ;-)
Comment 44 paolo@gcc.gnu.org 2010-10-06 00:17:38 UTC
Author: paolo
Date: Wed Oct  6 00:17:28 2010
New Revision: 165009

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

	PR libstdc++/45841
	* include/bits/fstream.h (basic_filebuf::underflow): Overflow
	success does not preclude returning failure.
	(basic_filebuf::pbackfail): Likewise.
	(basic_filebuf::xsputn): Fix indentation problem.
	(basic_filebuf::xsgetn): Likewise. Also, add similar overflow
	call to enable optimized case from write mode.
	* testsuite/27_io/basic_filebuf/underflow/char/45841.cc: New.
	* testsuite/27_io/basic_filebuf/underflow/wchar_t/45841.cc: Likewise.

Added:
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/underflow/char/45841.cc
    trunk/libstdc++-v3/testsuite/27_io/basic_filebuf/underflow/wchar_t/45841.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/fstream.tcc
Comment 45 Hans-Peter Nilsson 2010-10-07 21:46:55 UTC
Author: hp
Date: Thu Oct  7 21:46:51 2010
New Revision: 165136

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=165136
Log:
	PR libstdc++/45841
	* testsuite/lib/libstdc++.exp (check_v3_target_fileio): Rewrite to
	use an actual testsuite file and machinery, not ".".  Specifically
	check that incorrectly seeking backwards from 0 yields an error,
	and that reading, seeking backwards and reading again works.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/testsuite/lib/libstdc++.exp
Comment 46 Hans-Peter Nilsson 2010-10-07 22:14:40 UTC
I think we're done here; all issues (actual libstdc++-bug and "real" test-case, two simulator bugs, libstdc++ testsuite gate-function) fixed and committed.