Patchwork [RFA:] Stricter libstdc++ testsuite gate for working target file I/O

login
register
mail settings
Submitter Hans-Peter Nilsson
Date Oct. 6, 2010, 1:43 a.m.
Message ID <201010060143.o961h3bi032147@ignucius.se.axis.com>
Download mbox | patch
Permalink /patch/66886/
State New
Headers show

Comments

Hans-Peter Nilsson - Oct. 6, 2010, 1:43 a.m.
First but incidentally, we never did any useful target file I/O
in the the check_v3_target_fileio gate, we just tried to open
"." and seek to 0 (a nop at best; could also be construed as
invalid).  The test-suite does actual file I/O gated by that
'// { dg-require-fileio "" }' line: reading, writing, seeking,
so I think the gate function should try that, using the same
machinery as the actual tests.

Several tests require a working lseek primitive.  I think it's
easy enough to get seeking with a negative offset wrong in a
simulator(*) that it's worth testing for that.  Also, the
generic framework for the lseek syscall in the GNU simulators at
src/sim has a bug causing an invalid seek, like backwards from
position 0, to return 0 instead of properly yielding an error.
(I have patches for those simulator bugs, upcoming on
gdb-patches@, and I'll delay committing this patch until those
patches are applied.)

((*) where the API to the host uses "long" for syscall parameters
like src/sim struct cb_syscall, and the target is ILP32 like
cris-*-*, and if you handle your targets addresses as
zero-extended (as most targets except MIPS and SH/SH64, IIUC) so
you handle syscall parameters generally using unsigned 32-bit
numbers and you don't special-case the lseek position parameter
to properly get it sign-extended rather than zero-extended when
on a LP64 host...)

Here's a patch fixing the check_v3_target_fileio gate function.
I can't think of a system that this fixup would *incorrectly*
flag as not working.  It catches both the mentioned simulator
bugs (thus avoiding spurious failures leading to bogus bug
reports wasting maintainer time :) but passes for a fixed
(cris-elf) simulator and natively on a x86_64-unknown-linux-gnu
(manually testing the code).  Also, as a sanity check, ran a
complete regtest for cris-elf (fixed simulator) at r164529 with
no regressions.

While working on this I noticed that several tests miss the
// { dg-require-fileio "" }
line, which I'll add as obvious, later.

The bugs at this PR didn't stop there (with this one in the
test-suite, two simulator bugs and actually also one in
libstdc++ proper ironically found by the buggy simulator; a
patch has already been posted to add a properly "working"
testcase IIUC).  I also ran into
<http://subversion.tigris.org/issues/show_bug.cgi?id=2333> which
manifested as "svn diff -c164529" not showing
27_io/basic_filebuf/sync/wchar_t/1.cc as a deleted file, as it
dies for 27_io/basic_filebuf/sync/char/1.cc.  So be warned:
don't ultimately trust applying patches from svn diff -rOLD:NEW
as a way of patching up your tree; beware of deleted directories.
At least with subversion lower than 1.7.0 (to wit, 1.6.9).

Ok to commit?  Would it be ok to backport to open branches?

	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.


brgds, H-P
Paolo Carlini - Oct. 6, 2010, 9:47 a.m.
Hi,
> First but incidentally, we never did any useful target file I/O
> in the the check_v3_target_fileio gate, we just tried to open
> "." and seek to 0 (a nop at best; could also be construed as
> invalid).  The test-suite does actual file I/O gated by that
> '// { dg-require-fileio "" }' line: reading, writing, seeking,
> so I think the gate function should try that, using the same
> machinery as the actual tests.
>
> Several tests require a working lseek primitive.  I think it's
> easy enough to get seeking with a negative offset wrong in a
> simulator(*) that it's worth testing for that.  Also, the
> generic framework for the lseek syscall in the GNU simulators at
> src/sim has a bug causing an invalid seek, like backwards from
> position 0, to return 0 instead of properly yielding an error.
> (I have patches for those simulator bugs, upcoming on
> gdb-patches@, and I'll delay committing this patch until those
> patches are applied.)
>   
The approach makes sense, in general. My only doubt is that I don't
think we want to add the gate to a large part of the test files in
27_io/basic_filebuf, essentially. Can't we skip the entire set at once,
like we do for wchar_t or thread testcases, and leave the gate for those
specific tests which are really sensitive to an lseek fully functional?
I'm adding in CC people interested in this gate in first place...
> The bugs at this PR didn't stop there (with this one in the
> test-suite, two simulator bugs and actually also one in
> libstdc++ proper ironically found by the buggy simulator; a
> patch has already been posted to add a properly "working"
> testcase IIUC).
It's already in actually ;)

Paolo.

Patch

--- libstdc++-v3/testsuite/lib/libstdc++.exp.orig	Wed Sep 15 23:27:44 2010
+++ libstdc++-v3/testsuite/lib/libstdc++.exp	Wed Oct  6 00:01:46 2010
@@ -631,6 +631,7 @@  proc check_v3_target_fileio { } {
     global et_fileio_saved
     global et_fileio_target_name
     global tool	
+    global srcdir
 
     if { ![info exists et_fileio_target_name] } {
 	set et_fileio_target_name ""
@@ -656,6 +657,8 @@  proc check_v3_target_fileio { } {
 	# the file functions
 	set src fileio[pid].cc
 	set exe fileio[pid].x
+	set testfile "cin_unget-1.txt"
+	v3-copy-files "$srcdir/data/$testfile"
 
 	set f [open $src "w"]
 	puts $f "#include <sys/types.h>"
@@ -663,20 +666,26 @@  proc check_v3_target_fileio { } {
 	puts $f "#include <fcntl.h>"
 	puts $f "#include <unistd.h>"
 	puts $f "#include <errno.h>"
+	puts $f "#include <string.h>"
 	puts $f "using namespace std;"	
 	puts $f "int main ()"
 	puts $f "{"
-	puts $f "  int fd  = open (\".\", O_RDONLY);"
+	puts $f "  int fd  = open (\"$testfile\", O_RDONLY);"
 	puts $f "  int ret = 0;"
+	puts $f "  char buf\[10\];"
 	puts $f "  if (fd == -1)"
-	puts $f "  {"
-	puts $f "    int err = errno;"
-	puts $f "    if (err == EIO || err == ENOSYS)"
-	puts $f "      ret = 1;"
-	puts $f "  }"
+	puts $f "    ret = 1;"
 	puts $f "  else"
 	puts $f "  {"
-	puts $f "    if (lseek (fd, 0, SEEK_CUR) == -1)"
+	puts $f "    if (lseek (fd, -1, SEEK_CUR) != -1 || errno != EINVAL)"
+	puts $f "      ret = 1;"
+	puts $f "    errno = 0;"
+	puts $f "    if (lseek (fd, 0, SEEK_CUR) != 0"
+	puts $f "        || read (fd, buf, 4) != 4"
+	puts $f "        || memcmp (buf, \"1234\", 4) != 0"
+	puts $f "        || lseek (fd, -2, SEEK_CUR) != 2"
+	puts $f "        || read (fd, buf, 2) != 2"
+	puts $f "        || memcmp (buf, \"34\", 2) != 0)"
 	puts $f "      ret = 1;"
 	puts $f "    close (fd);"
 	puts $f "  }"