[PATCH] Remove useless "if" tests before free. Deprecate "free" module.
Here's a proposed patch to remove all redundant "if"-before-free
tests in the tests/ directory and in the lib/*.c files not owned
by Bruno. It also deprecates (in comments and documentation) the
free module. Paul ok'd that part.
Simon, this affects two files that are yours: lib/gc-gnulib.c and
lib/getaddrinfo.c. Please let me know if this is ok with you.
If not, I'll be happy to remove them from the list.
-
Remove useless "if" tests before free. Deprecate "free" module.
* doc/posix-functions/free.texi: Mention that this
module is no longer useful.
* modules/free: Add comments to the same effect.
* modules/readutmp (Depends-on): Remove free.
* lib/save-cwd.c (free_cwd): Remove useless "if" before free.
* lib/putenv.c (putenv): Likewise.
* lib/gc-gnulib.c (gc_cipher_close): Likewise.
* lib/getaddrinfo.c (freeaddrinfo): Likewise.
* tests/test-c-strcasestr.c (main): Likewise.
* tests/test-c-strstr.c (main): Likewise.
* tests/test-mbscasestr1.c (main): Likewise.
* tests/test-mbscasestr2.c (main): Likewise.
* tests/test-mbsstr1.c (main): Likewise.
* tests/test-mbsstr2.c (main): Likewise.
* tests/test-memmem.c (main): Likewise.
* tests/test-strcasestr.c (main): Likewise.
* tests/test-striconv.c (main): Likewise.
* tests/test-striconveh.c (main): Likewise.
* tests/test-striconveha.c (main): Likewise.
* tests/test-strstr.c (main): Likewise.
Signed-off-by: Jim Meyering <[EMAIL PROTECTED]>
---
doc/posix-functions/free.texi |5 -
lib/gc-gnulib.c |3 +--
lib/getaddrinfo.c |2 +-
lib/putenv.c |5 ++---
lib/save-cwd.c|3 +--
modules/free |5 -
modules/readutmp |1 -
tests/test-c-strcasestr.c |6 ++
tests/test-c-strstr.c |6 ++
tests/test-mbscasestr1.c |6 ++
tests/test-mbscasestr2.c |6 ++
tests/test-mbsstr1.c |6 ++
tests/test-mbsstr2.c |6 ++
tests/test-memmem.c | 12
tests/test-strcasestr.c |6 ++
tests/test-striconv.c |3 +--
tests/test-striconveh.c |6 ++
tests/test-striconveha.c |3 +--
tests/test-strstr.c |6 ++
19 files changed, 37 insertions(+), 59 deletions(-)
diff --git a/doc/posix-functions/free.texi b/doc/posix-functions/free.texi
index a407dc2..10d95cd 100644
--- a/doc/posix-functions/free.texi
+++ b/doc/posix-functions/free.texi
@@ -9,7 +9,10 @@ Gnulib module: free
Portability problems fixed by Gnulib:
@itemize
@item
-On old platforms, @code{free (NULL)} is not allowed.
+On old platforms such as SunOS4, @code{free (NULL)} fails.
+However, since all such systems are so old as to no longer
+be considered ``reasonable portability targets,''
+this module is no longer useful.
@end itemize
Portability problems not fixed by Gnulib:
diff --git a/lib/gc-gnulib.c b/lib/gc-gnulib.c
index 94e0880..d535d03 100644
--- a/lib/gc-gnulib.c
+++ b/lib/gc-gnulib.c
@@ -546,8 +546,7 @@ gc_cipher_close (gc_cipher_handle handle)
{
_gc_cipher_ctx *ctx = handle;
- if (ctx)
-free (ctx);
+ free (ctx);
return GC_OK;
}
diff --git a/lib/getaddrinfo.c b/lib/getaddrinfo.c
index aa07903..c7a1ea9 100644
--- a/lib/getaddrinfo.c
+++ b/lib/getaddrinfo.c
@@ -326,7 +326,7 @@ freeaddrinfo (struct addrinfo *ai)
cur = ai;
ai = ai->ai_next;
- if (cur->ai_canonname) free (cur->ai_canonname);
+ free (cur->ai_canonname);
free (cur);
}
}
diff --git a/lib/putenv.c b/lib/putenv.c
index 351b403..d0573c6 100644
--- a/lib/putenv.c
+++ b/lib/putenv.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1991, 1994, 1997, 1998, 2000, 2003, 2004, 2005, 2006, 2007
+/* Copyright (C) 1991, 1994, 1997-1998, 2000, 2003-2008
Free Software Foundation, Inc.
NOTE: The canonical source of this file is maintained with the GNU C
@@ -122,8 +122,7 @@ putenv (char *string)
size * sizeof (char *));
new_environ[size] = (char *) string;
new_environ[size + 1] = NULL;
- if (last_environ != NULL)
- free (last_environ);
+ free (last_environ);
last_environ = new_environ;
environ = new_environ;
}
diff --git a/lib/save-cwd.c b/lib/save-cwd.c
index 7618f09..e158e8b 100644
--- a/lib/save-cwd.c
+++ b/lib/save-cwd.c
@@ -97,6 +97,5 @@ free_cwd (struct saved_cwd *cwd)
{
if (cwd->desc >= 0)
close (cwd->desc);
- if (cwd->name)
-free (cwd->name);
+ free (cwd->name);
}
diff --git a/modules/free b/modules/free
index 94b6a3a..7e4f18f 100644
--- a/modules/free
+++ b/modules/free
@@ -1,5 +1,9 @@
Description:
Work around incompatibility on older systems where free (NULL) fails.
+# Note: as of 2008, this module is no longer useful, since no
+# "reasonable portability target" (which excludes
Re: [PATCH] Remove useless "if" tests before free. Deprecate "free" module.
Jim Meyering wrote:
> --- a/modules/free
> +++ b/modules/free
> @@ -1,5 +1,9 @@
> Description:
> Work around incompatibility on older systems where free (NULL) fails.
> +# Note: as of 2008, this module is no longer useful, since no
> +# "reasonable portability target" (which excludes SunOS4) system
> +# requires the wrapper.
> +# FIXME: mark as obsolescent
You can "implement" this FIXME by adding a paragraph to the module description:
Notice:
This module is obsolete.
gnulib-tool will then print, when the module is used:
Notice from module free:
This module is obsolete.
See modules/TEMPLATE-EXTENDED for the full set of possible sections in a
module file.
Note also that the 'Description' field does not support comments. Its entire
contents is copied into the MODULES.html file when that is generated. Therefore
normally the description is kept short. The user can refer to the complete
doc of the module through doc/gnulib.{info.html}.
> Here's a proposed patch
Fine with me.
Bruno
new module memchr2
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
M4 has a situation where it wants to search for the first of two candidate
bytes within an arbitrary string. In fact, Bruno profiled m4 when running
autoconf on the gettext package, and found that more than 10% of the total
execution time was in a hot loop doing just that (searching for the first
of '[' and ']' to find the end of a quoted string).
strcspn isn't good enough - it doesn't handle embedded NUL (as a side
note, you should never pass an arbitrary string as the second argument of
strcspn, since glibc's implementation is quadratic if called like
strcspn("a",""..."bbb")).
Two calls to memchr isn't good enough. You end up traversing the prefix
of the string twice instead of once, and long enough strings can even
trigger cache flushes before the second search.
Byte-wise traversal isn't good enough - the naive loop with a
double-conditional every byte is processor intensive, compared to more
efficient vectorized algorithms present in good memchr implementations.
getndelim2 isn't good enough - it supports the search for two characters,
but must be based on a FILE, and fmemopen isn't portable (and probably not
efficient, either).
So I'm checking in this new module. At this point, I'm wondering if we
should rewrite getndelim2 to take advantage of memchr2 and
freadahead/freadptr/freadseek, rather than operate a byte at a time.
- --
Don't work too hard, make some time for fun as well!
Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFHyWk484KuGfSFAYARAlEGAJ46eyHP1oPDBh9SLL837XkkiD/73wCgpdiu
wgAS1jo+iKrtRXPSUvn8N8Q=
=5mAE
-END PGP SIGNATURE-
>From f91b9f973226f2b434ba24e32845f252a3d6e64b Mon Sep 17 00:00:00 2001
From: Eric Blake <[EMAIL PROTECTED]>
Date: Sat, 1 Mar 2008 06:54:29 -0700
Subject: [PATCH] New module 'memchr2'.
* modules/memchr2: New file.
* modules/memchr2-tests: Likewise.
* lib/memchr2.h: Likewise.
* lib/memchr2.c: Likewise, based on memchr.c.
* tests/test-memchr2.c: New test.
* MODULES.html.sh (String handling): Add memchr2.
Signed-off-by: Eric Blake <[EMAIL PROTECTED]>
---
ChangeLog | 10 +++
MODULES.html.sh |1 +
lib/memchr2.c | 194 +
lib/memchr2.h | 31
modules/memchr2 | 24 ++
modules/memchr2-tests | 10 +++
tests/test-memchr2.c | 89 ++
7 files changed, 359 insertions(+), 0 deletions(-)
create mode 100644 lib/memchr2.c
create mode 100644 lib/memchr2.h
create mode 100644 modules/memchr2
create mode 100644 modules/memchr2-tests
create mode 100644 tests/test-memchr2.c
diff --git a/ChangeLog b/ChangeLog
index ace7d2a..0d3e4ba 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2008-03-01 Eric Blake <[EMAIL PROTECTED]>
+
+ New module 'memchr2'.
+ * modules/memchr2: New file.
+ * modules/memchr2-tests: Likewise.
+ * lib/memchr2.h: Likewise.
+ * lib/memchr2.c: Likewise, based on memchr.c.
+ * tests/test-memchr2.c: New test.
+ * MODULES.html.sh (String handling): Add memchr2.
+
2008-02-29 Bruno Haible <[EMAIL PROTECTED]>
* modules/freadseek-tests: New file.
diff --git a/MODULES.html.sh b/MODULES.html.sh
index 16c3552..683bb69 100755
--- a/MODULES.html.sh
+++ b/MODULES.html.sh
@@ -1653,6 +1653,7 @@ func_all_modules ()
func_begin_table
func_module bcopy
+ func_module memchr2
func_module memmem
func_module memmem-simple
func_module mempcpy
diff --git a/lib/memchr2.c b/lib/memchr2.c
new file mode 100644
index 000..540ed9f
--- /dev/null
+++ b/lib/memchr2.c
@@ -0,0 +1,194 @@
+/* Copyright (C) 1991, 1993, 1996, 1997, 1999, 2000, 2003, 2004, 2006,
+ 2008 Free Software Foundation, Inc.
+
+ Based on strlen implementation by Torbjorn Granlund ([EMAIL PROTECTED]),
+ with help from Dan Sahlin ([EMAIL PROTECTED]) and
+ commentary by Jim Blandy ([EMAIL PROTECTED]);
+ adaptation to memchr suggested by Dick Karpinski ([EMAIL PROTECTED]),
+ and implemented in glibc by Roland McGrath ([EMAIL PROTECTED]).
+ Extension to memchr2 implemented by Eric Blake ([EMAIL PROTECTED]).
+
+This program is free software: you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by the
+Free Software Foundation; either version 3 of the License, or any
+later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program. If not, see
[PATCH] (getdelim): Don't leak memory upon failed realloc.
Hi Simon,
I noticed that our getdelim replacement can leak when the initial realloc
fails. While that can happen only when *N is 0 and *LINEPTR is malloc'd,
I think it's worth fixing, if only to avoid further discussion on the topic.
Here's the fix.
Ok to apply?
* lib/getdelim.c (getdelim): Don't leak memory upon failed realloc.
diff --git a/lib/getdelim.c b/lib/getdelim.c
index 0547c7f..7c6f326 100644
--- a/lib/getdelim.c
+++ b/lib/getdelim.c
@@ -69,13 +69,15 @@ getdelim (char **lineptr, size_t *n, int delimiter, FILE
*fp)
if (*lineptr == NULL || *n == 0)
{
+ char *new_lineptr;
*n = 120;
- *lineptr = (char *) realloc (*lineptr, *n);
- if (*lineptr == NULL)
+ new_lineptr = (char *) realloc (*lineptr, *n);
+ if (new_lineptr == NULL)
{
result = -1;
goto unlock_return;
}
+ *lineptr = new_lineptr;
}
for (;;)
--
1.5.4.3.341.g956c8c
Re: [PATCH] Remove useless "if" tests before free. Deprecate "free" module.
Bruno Haible <[EMAIL PROTECTED]> wrote: > Jim Meyering wrote: >> --- a/modules/free >> +++ b/modules/free >> @@ -1,5 +1,9 @@ >> Description: >> Work around incompatibility on older systems where free (NULL) fails. >> +# Note: as of 2008, this module is no longer useful, since no >> +# "reasonable portability target" (which excludes SunOS4) system >> +# requires the wrapper. >> +# FIXME: mark as obsolescent > > You can "implement" this FIXME by adding a paragraph to the module > description: > > Notice: > This module is obsolete. > > gnulib-tool will then print, when the module is used: > > Notice from module free: > This module is obsolete. Thanks. I've adjusted my patch.
[PATCH] bootstrap: sync from coreutils
FYI, I've just pushed this:
bootstrap: sync from coreutils
* build-aux/bootstrap (update_po_files): Copy a .po file into place
also when the target doesn't exist.
diff --git a/build-aux/bootstrap b/build-aux/bootstrap
index 87a2798..d68d3f4 100755
--- a/build-aux/bootstrap
+++ b/build-aux/bootstrap
@@ -278,6 +278,7 @@ update_po_files() {
new_po="$ref_po_dir/$po.po"
cksum_file="$ref_po_dir/$po.s1"
if ! test -f "$cksum_file" ||
+ ! test -f "$po_dir/$po.po" ||
! sha1sum -c --status "$cksum_file" < "$new_po" > /dev/null; then
echo "updated $po_dir/$po.po..."
cp "$new_po" "$po_dir/$po.po" && sha1sum < "$new_po" > "$cksum_file"
--
1.5.4.3.341.g956c8c
Re: [PATCH] bootstrap: sync from coreutils
Jim Meyering <[EMAIL PROTECTED]> wrote: > FYI, I've just pushed this: I've just pushed the ChangeLog entry, too.
Re: new module memchr2
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
According to Bruce Korb on 3/1/2008 7:57 AM:
|> + charmask1 |= charmask2 << 16;
|
| Have you tested this? :(
Yes, but unfortunately, my tests didn't trigger every possible alignment.
~ Also, the tests made use of the GNU extension of adding int + void*,
which is invalid in C89. Fixed thusly:
- --
Don't work too hard, make some time for fun as well!
Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFHyXxe84KuGfSFAYARAh2xAKCxcI4YZ8ZtOuUuopriMs9uivZ2EgCgraiD
9ATnEeG7tqvXXNBrYuPuCwU=
=HfOr
-END PGP SIGNATURE-
>From cdc13566c60ec33afcce63957c58df27016e6200 Mon Sep 17 00:00:00 2001
From: Eric Blake <[EMAIL PROTECTED]>
Date: Sat, 1 Mar 2008 08:40:22 -0700
Subject: [PATCH] Fix bugs in last patch.
* lib/memchr2.c (memchr2): Fix typo.
* tests/test-memchr2.c: Test previous bug, and don't use GNU
extension.
Reported by Bruce Korb.
Signed-off-by: Eric Blake <[EMAIL PROTECTED]>
---
ChangeLog|6 +
lib/memchr2.c|4 +-
tests/test-memchr2.c | 58 +
3 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 0d3e4ba..d660676 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
2008-03-01 Eric Blake <[EMAIL PROTECTED]>
+ Fix bugs in last patch.
+ * lib/memchr2.c (memchr2): Fix typo.
+ * tests/test-memchr2.c: Test previous bug, and don't use GNU
+ extension.
+ Reported by Bruce Korb.
+
New module 'memchr2'.
* modules/memchr2: New file.
* modules/memchr2-tests: Likewise.
diff --git a/lib/memchr2.c b/lib/memchr2.c
index 540ed9f..d5b0a78 100644
--- a/lib/memchr2.c
+++ b/lib/memchr2.c
@@ -81,8 +81,8 @@ memchr2 (void const *s, int c1_in, int c2_in, size_t n)
magic_bits = 0xfefefefe;
charmask1 = c1 | (c1 << 8);
charmask2 = c2 | (c2 << 8);
- charmask1 |= charmask2 << 16;
- charmask1 |= charmask2 << 16;
+ charmask1 |= charmask1 << 16;
+ charmask2 |= charmask2 << 16;
#if 0xU < UINTMAX_MAX
magic_bits |= magic_bits << 32;
charmask1 |= charmask1 << 32;
diff --git a/tests/test-memchr2.c b/tests/test-memchr2.c
index 68e8595..639ecd3 100644
--- a/tests/test-memchr2.c
+++ b/tests/test-memchr2.c
@@ -34,6 +34,10 @@
} \
while (0)
+/* Calculating void * + int is not portable, so this wrapper converts
+ to char * to make the tests easier to write. */
+#define MEMCHR2 (char *) memchr2
+
int
main ()
{
@@ -48,27 +52,28 @@ main ()
input[n - 2] = 'e';
input[n - 1] = 'a';
- ASSERT (memchr2 (input, 'a', 'b', n) == input);
- ASSERT (memchr2 (input, 'b', 'a', n) == input);
+ /* Basic behavior tests. */
+ ASSERT (MEMCHR2 (input, 'a', 'b', n) == input);
+ ASSERT (MEMCHR2 (input, 'b', 'a', n) == input);
- ASSERT (memchr2 (input, 'a', 'b', 0) == NULL);
- ASSERT (memchr2 (NULL, 'a', 'b', 0) == NULL);
+ ASSERT (MEMCHR2 (input, 'a', 'b', 0) == NULL);
+ ASSERT (MEMCHR2 (NULL, 'a', 'b', 0) == NULL);
- ASSERT (memchr2 (input, 'b', 'd', n) == input + 1);
- ASSERT (memchr2 (input + 2, 'b', 'd', n - 2) == input + 1026);
+ ASSERT (MEMCHR2 (input, 'b', 'd', n) == input + 1);
+ ASSERT (MEMCHR2 (input + 2, 'b', 'd', n - 2) == input + 1026);
- ASSERT (memchr2 (input, 'd', 'e', n) == input + 1026);
- ASSERT (memchr2 (input, 'e', 'd', n) == input + 1026);
+ ASSERT (MEMCHR2 (input, 'd', 'e', n) == input + 1026);
+ ASSERT (MEMCHR2 (input, 'e', 'd', n) == input + 1026);
- ASSERT (memchr2 (input + 1, 'a', 'e', n - 1) == input + n - 2);
- ASSERT (memchr2 (input + 1, 'e', 'a', n - 1) == input + n - 2);
+ ASSERT (MEMCHR2 (input + 1, 'a', 'e', n - 1) == input + n - 2);
+ ASSERT (MEMCHR2 (input + 1, 'e', 'a', n - 1) == input + n - 2);
- ASSERT (memchr2 (input, 'f', 'g', n) == NULL);
- ASSERT (memchr2 (input, 'f', '\0', n) == NULL);
+ ASSERT (MEMCHR2 (input, 'f', 'g', n) == NULL);
+ ASSERT (MEMCHR2 (input, 'f', '\0', n) == NULL);
- ASSERT (memchr2 (input, 'a', 'a', n) == input);
- ASSERT (memchr2 (input + 1, 'a', 'a', n - 1) == input + n - 1);
- ASSERT (memchr2 (input, 'f', 'f', n) == NULL);
+ ASSERT (MEMCHR2 (input, 'a', 'a', n) == input);
+ ASSERT (MEMCHR2 (input + 1, 'a', 'a', n - 1) == input + n - 1);
+ ASSERT (MEMCHR2 (input, 'f', 'f', n) == NULL);
/* Check that a very long haystack is handled quickly if one of the
two bytes is found near the beginning. */
@@ -76,10 +81,25 @@ main ()
size_t repeat = 1;
for (; repeat > 0; repeat--)
{
- ASSERT (memchr2 (input, 'c', 'e', n) == input + 2);
- ASSERT (memchr2 (input, 'e', 'c', n) == input + 2);
- ASSERT (memchr2 (input, 'c', '\0', n) == input + 2);
- ASSERT (memchr2 (input, '\0', 'c', n) == input +
Re: new module 'freadseek'
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
According to Bruno Haible on 2/29/2008 2:29 AM:
| Here's the side-effecting companion of freadptr.
| /* Increment the in-memory pointer. This is very cheap (no system
calls). */
| buffered = freadahead (fp);
| if (buffered > 0)
| {
| size_t increment = (buffered < offset ? buffered : offset);
| #elif defined __sferror /* FreeBSD, NetBSD, OpenBSD, MacOS
X, Cygwin */
| fp->_p += increment;
| fp->_r -= increment;
test-freadseek is failing on cygwin:
82ungetc ('@', stdin);
(gdb) s
83ASSERT (freadseek (stdin, nbytes6) == 0);
(gdb) s
freadseek (fp=0x61103080, offset=9) at ../../gllib/freadseek.c:33
33if (offset == 0)
(gdb) p *fp
$1 = {_p = 0x611030c6 "@", _r = 1, _w = 0, _flags = -31612, _file = 0, _bf = {
~_base = 0x660248 "#!/bin/sh\n\n./test-freadseek${EXEEXT} 5 19 6 7 18 9
19 < \"$srcdir/test-freadseek.sh\" || exit 1\ncat
\"$srcdir/test-freadseek.sh\" | ./test-freadseek${EXEEXT} 5 19 6 7 18 9 19
|| exit 1\nexit 0\n", _size = 65536},
~ _lbfsize = 0, _data = 0x0, _cookie = 0x61103080,
~ _read = 0x610f83f0 <__sread>, _write = 0x610f86c0 <__swrite64>,
~ _seek = 0x610f85b0 <__sseek>, _close = 0x610f8620 <__sclose>, _ub = {
~_base = 0x611030c4 "", _size = 3},
~ _up = 0x660280 "\"$srcdir/test-freadseek.sh\" || exit 1\ncat
\"$srcdir/test-freadseek.sh\" | ./test-freadseek${EXEEXT} 5 19 6 7 18 9 19
|| exit 1\nexit 0\n", _ur = 132, _ubuf = "\000\000@", _nbuf = "", _lb =
{_base = 0x0, _size = 0},
~ _blksize = 65536, _flags2 = 0, _offset = 188,
~ _seek64 = 0x610f8650 <__sseek64>, _lock = 0x12}
...
81fd = fileno (fp);
(gdb)
82if (fd >= 0 && lseek (fd, 0, SEEK_CUR) >= 0)
(gdb)
85return fseek (fp, offset, SEEK_CUR);
(gdb) p offset
$4 = 8
(gdb) fin
Run till exit from #0 freadseek (fp=0x61103080, offset=8)
Value returned is $5 = -1
Somehow, the tweaks you did to the raw _fp to account for the 1-byte
readahead buffer from the arbitrary ungetc trip up the subsequent fseek.
I'm in the process of compiling a debug version of cygwin to try to find
out more details why cygwin's fseek failed, but it may take a while.
- --
Don't work too hard, make some time for fun as well!
Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFHyZYN84KuGfSFAYARAgDmAJ9VcZkZfahfwz/hCd9nUtxKLzEFcwCfWH51
0jKsFINsyyjibHDpz79t+z8=
=kw0g
-END PGP SIGNATURE-
Re: new module 'freadseek'
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
According to Eric Blake on 3/1/2008 10:44 AM:
|
| Somehow, the tweaks you did to the raw _fp to account for the 1-byte
| readahead buffer from the arbitrary ungetc trip up the subsequent fseek.
| I'm in the process of compiling a debug version of cygwin to try to find
| out more details why cygwin's fseek failed, but it may take a while.
On further investigation, I think this is a true bug in newlib's stdio,
and not in your code. This simpler program fails some of the time, but
not if there was a previous ftell:
$ cat foo.c
#include
#include
int
main (int argc, char* argv[])
{
~ int i1, i2;
~ if (argc > 1)
~{
~ i2 = ftell (stdin);
~ printf ("i2=%d\n", i2);
~}
~ /* Test behaviour after arbitrary ungetc. */
~ fgetc (stdin);
~ if (argc > 1)
~{
~ i2 = ftell (stdin);
~ printf ("i2=%d\n", i2);
~}
~ ungetc ('@', stdin);
~ if (argc > 1)
~{
~ i2 = ftell (stdin);
~ printf ("i2=%d\n", i2);
~}
~ i1 = fseek (stdin, 2, SEEK_CUR);
~ i2 = ftell (stdin);
~ printf ("i1=%d i2=%d\n", i1, i2);
~ return 0;
}
$ ./foo < foo.c
i1=-1 i2=-536
$ ./foo a < foo.c
i2=0
i2=1
i2=0
i1=0 i2=2
- --
Don't work too hard, make some time for fun as well!
Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFHyb4684KuGfSFAYARAgoEAJ4j2E2sDIPckneFx7fN71gY9DxqbQCeMLoH
GIvjgX1hpmjqnhjZF9fErJA=
=pyQX
-END PGP SIGNATURE-
Re: new module 'freadseek'
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
According to Eric Blake on 3/1/2008 1:36 PM:
| On further investigation, I think this is a true bug in newlib's stdio,
| and not in your code.
Newlib has two bugs - first, fflush is failing to discard ungetc data when
changing the underlying fd offset. Second, ftell is improperly calling
fflush under the hood, which when coupled with the first bug, results in a
confused file offset, but even when the first bug is fixed, loses data
contrary to POSIX. (BSD's ftell probably also calls fflush under the
hood, but since BSD's fflush disobeys POSIX by treating fflush on a read
stream as a no-op rather than discarding unread data, these bugs are not
visible there).
$ cat foo.c
#include
#include
int
main (int argc, char* argv[])
{
~ int i1, i2;
~ i1 = fgetc (stdin);
~ printf ("i1=%c\n", i1);
~ i1 = ungetc ('@', stdin);
~ printf ("i1=%c\n", i1);
~ if (argc > 1)
~{
~ i2 = ftell (stdin);
~ printf ("i2=%d\n", i2);
~}
~ i1 = fgetc (stdin);
~ printf ("i1=%c\n", i1);
~ return 0;
}
$ ./foo < foo.c
i1=#
i1=@
i1=@
$ ./foo a < foo.c
i1=#
i1=@
i2=-337
i1=i
On Linux, and after my patch, it properly does:
% ./foo < foo.c
i1=#
i1=@
i1=@
% ./foo a < foo.c
i1=#
i1=@
i2=0
i1=@
OK to apply this to newlib? Patching gnulib to work around these newlib
bugs for older releases of cygwin seems daunting; it's hard to do anything
when you can't trust ftell or fflush after ungetc.
2008-03-01 Eric Blake <[EMAIL PROTECTED]>
Fix ftell bug after ungetc.
* libc/stdio/ftell.c (_ftell_r): Don't flush ungetc buffer on
ftell.
* libc/stdio64/ftello64.c (_ftello64_r): Likewise.
* libc/stdio/fflush.c (_fflush_r): Clear unget buffer when
repositioning underlying fd offset.
- --
Don't work too hard, make some time for fun as well!
Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFHyec984KuGfSFAYARAoezAKDC6snVwkdf1ZcSS0NbgTZ5fRMLBACaAnSl
Cixn14d6TZ56/1PH6WgR10A=
=/Dr1
-END PGP SIGNATURE-
Index: libc/stdio/fflush.c
===
RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
retrieving revision 1.11
diff -u -p -r1.11 fflush.c
--- libc/stdio/fflush.c 13 Jul 2007 20:37:53 - 1.11
+++ libc/stdio/fflush.c 1 Mar 2008 22:41:26 -
@@ -164,6 +164,8 @@ _DEFUN(_fflush_r, (ptr, fp),
fp->_p = fp->_bf._base;
if (fp->_flags & __SOFF)
fp->_offset = curoff;
+ if (HASUB (fp))
+ FREEUB (ptr, fp);
}
else
{
Index: libc/stdio/ftell.c
===
RCS file: /cvs/src/src/newlib/libc/stdio/ftell.c,v
retrieving revision 1.12
diff -u -p -r1.12 ftell.c
--- libc/stdio/ftell.c 13 Jul 2007 20:37:53 - 1.12
+++ libc/stdio/ftell.c 1 Mar 2008 22:41:26 -
@@ -118,9 +118,12 @@ _DEFUN(_ftell_r, (ptr, fp),
return -1L;
}
- /* Find offset of underlying I/O object, then
- adjust for buffered bytes. */
- _fflush_r (ptr, fp); /* may adjust seek offset on append stream */
+ /* Find offset of underlying I/O object, then adjust for buffered
+ bytes. Flush a write stream, since the offset may be altered if
+ the stream is appending. Do not flush a read stream, since we
+ must not lose the ungetc buffer. */
+ if (fp->_flags & __SWR)
+_fflush_r (ptr, fp);
if (fp->_flags & __SOFF)
pos = fp->_offset;
else
Index: libc/stdio64/ftello64.c
===
RCS file: /cvs/src/src/newlib/libc/stdio64/ftello64.c,v
retrieving revision 1.8
diff -u -p -r1.8 ftello64.c
--- libc/stdio64/ftello64.c 13 Jul 2007 20:37:53 - 1.8
+++ libc/stdio64/ftello64.c 1 Mar 2008 22:41:26 -
@@ -108,9 +108,12 @@ _DEFUN (_ftello64_r, (ptr, fp),
return -1L;
}
- /* Find offset of underlying I/O object, then
- adjust for buffered bytes. */
- _fflush_r (ptr, fp); /* may adjust seek offset on append stream */
+ /* Find offset of underlying I/O object, then adjust for buffered
+ bytes. Flush a write stream, since the offset may be altered if
+ the stream is appending. Do not flush a read stream, since we
+ must not lose the ungetc buffer. */
+ if (fp->_flags & __SWR)
+_fflush_r (ptr, fp);
if (fp->_flags & __SOFF)
pos = fp->_offset;
else
