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.
Well, that's almost certainly my fault, because that patch directly implements that testcase. However
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.
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...
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)
Challenging to the codecvt?!? Isn't this a *char* testcase?
(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?
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.
(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. ;-)
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().
(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.
(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.
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)
(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.
Ok..
(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 ...
(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!
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.
(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
(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…
(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. :]
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.
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.
(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.
(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!
> 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.
(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.
(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; }
(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?
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.
Also, I think we need to clarify whether 9425.cc is a regression. That was an expected failure before my revision, right?
(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.
(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
(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.
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
(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.
(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>
(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?
> 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.
> 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.
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...
(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.
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.
(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. ;-)
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
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
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.