Re: guile 1.9.0 scm_read_hash_extend gc trouble
Hi Neil, l...@gnu.org (Ludovic Courtès) writes: Yes. What do you think of the attached patch? For the record, I went ahead and committed it: http://git.savannah.gnu.org/cgit/guile.git/commit/?id=0a94eb002eba6539879f2cddf3e45fb25976af8d Thanks, Ludo'.
Re: guile 1.9.0 scm_read_hash_extend gc trouble
l...@gnu.org (Ludovic Courtès) writes: Hi Neil, l...@gnu.org (Ludovic Courtès) writes: Yes. What do you think of the attached patch? For the record, I went ahead and committed it: http://git.savannah.gnu.org/cgit/guile.git/commit/?id=0a94eb002eba6539879f2cddf3e45fb25976af8d Sorry for the delay. It looks great, anyway. Neil
Re: guile 1.9.0 scm_read_hash_extend gc trouble
l...@gnu.org (Ludovic Courtès) writes: The attached patch adds a new `scm_t_off' type, whose definition does not depend on the application's `_FILE_OFFSET_BITS' value. Can you confirm that it allows you to build Guile with 32-bit offsets and Snd with 64-bit offsets (or vice versa)? Neil: Does this sound like the right approach? Yes, this looks great. (In case it makes a difference, I've reviewed the master commit, not the patch that you put in email.) Also I think it means that we can delete some code that was needed to handle the possibility that off_t might be 32-bit on a platform that also supports large files. - In scm_seek (), the if (SCM_OPFPORTP (fd_port)) block is now not needed, because the following more general if (SCM_OPPORTP (fd_port)) case will handle 64-bit correctly. - Therefore scm_i_fport_seek () can be removed. - The #if GUILE_USE_64_CALLS HAVE_STAT64 SIZEOF_OFF_T != SIZEOF_OFF64_T implementation of fport_seek () can be removed, because fport_seek () is now always identical to fport_seek_or_seek64 (). - Plus the same things again but for truncate instead of seek. Would you agree? Regards, Neil
Re: guile 1.9.0 scm_read_hash_extend gc trouble
Hi,
Neil Jerram n...@ossau.uklinux.net writes:
l...@gnu.org (Ludovic Courtès) writes:
The attached patch adds a new `scm_t_off' type, whose definition does
not depend on the application's `_FILE_OFFSET_BITS' value. Can you
confirm that it allows you to build Guile with 32-bit offsets and Snd
with 64-bit offsets (or vice versa)?
Neil: Does this sound like the right approach?
Yes, this looks great. (In case it makes a difference, I've reviewed
the master commit, not the patch that you put in email.)
Perfect, thank you!
Also I think it means that we can delete some code that was needed to
handle the possibility that off_t might be 32-bit on a platform that
also supports large files.
- In scm_seek (), the if (SCM_OPFPORTP (fd_port)) block is now not
needed, because the following more general if (SCM_OPPORTP
(fd_port)) case will handle 64-bit correctly.
- Therefore scm_i_fport_seek () can be removed.
- The #if GUILE_USE_64_CALLS HAVE_STAT64 SIZEOF_OFF_T !=
SIZEOF_OFF64_T implementation of fport_seek () can be removed,
because fport_seek () is now always identical to
fport_seek_or_seek64 ().
- Plus the same things again but for truncate instead of seek.
Also:
- `scm_t_off' and `off_t_or_off64_t' are now identical, so the latter
could be removed.
Would you agree?
Yes. What do you think of the attached patch?
On my GNU/Linux machine where 64-bit offsets are used, I see this:
--8---cut here---start-8---
scheme@(guile-user) (open-input-file /dev/zero)
$1 = #input: /dev/zero 70
scheme@(guile-user) (seek $1 (expt 2 33) SEEK_SET)
$2 = 0
--8---cut here---end---8---
I can't think of a better test case, though.
Thanks,
Ludo'.
diff --git a/libguile/fports.c b/libguile/fports.c
index f6e0556..cfb8b25 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -610,8 +610,8 @@ fport_fill_input (SCM port)
}
}
-static off_t_or_off64_t
-fport_seek_or_seek64 (SCM port, off_t_or_off64_t offset, int whence)
+static scm_t_off
+fport_seek (SCM port, scm_t_off offset, int whence)
{
scm_t_port *pt = SCM_PTAB_ENTRY (port);
scm_t_fport *fp = SCM_FSTREAM (port);
@@ -662,39 +662,6 @@ fport_seek_or_seek64 (SCM port, off_t_or_off64_t offset, int whence)
return result;
}
-/* If we've got largefile and off_t isn't already off64_t then
- fport_seek_or_seek64 needs a range checking wrapper to be fport_seek in
- the port descriptor.
-
- Otherwise if no largefile, or off_t is the same as off64_t (which is the
- case on NetBSD apparently), then fport_seek_or_seek64 is right to be
- fport_seek already. */
-
-#if GUILE_USE_64_CALLS HAVE_STAT64 SIZEOF_OFF_T != SIZEOF_OFF64_T
-static scm_t_off
-fport_seek (SCM port, scm_t_off offset, int whence)
-{
- off64_t rv = fport_seek_or_seek64 (port, (off64_t) offset, whence);
- if (rv OFF_T_MAX || rv OFF_T_MIN)
-{
- errno = EOVERFLOW;
- scm_syserror (fport_seek);
-}
- return (scm_t_off) rv;
-
-}
-#else
-#define fport_seek fport_seek_or_seek64
-#endif
-
-/* `how' has been validated and is one of SEEK_SET, SEEK_CUR or SEEK_END */
-SCM
-scm_i_fport_seek (SCM port, SCM offset, int how)
-{
- return scm_from_off_t_or_off64_t
-(fport_seek_or_seek64 (port, scm_to_off_t_or_off64_t (offset), how));
-}
-
static void
fport_truncate (SCM port, scm_t_off length)
{
@@ -704,13 +671,6 @@ fport_truncate (SCM port, scm_t_off length)
scm_syserror (ftruncate);
}
-int
-scm_i_fport_truncate (SCM port, SCM length)
-{
- scm_t_fport *fp = SCM_FSTREAM (port);
- return ftruncate_or_ftruncate64 (fp-fdes, scm_to_off_t_or_off64_t (length));
-}
-
/* helper for fport_write: try to write data, using multiple system
calls if required. */
#define FUNC_NAME write_all
diff --git a/libguile/fports.h b/libguile/fports.h
index 2687504..cbef0f8 100644
--- a/libguile/fports.h
+++ b/libguile/fports.h
@@ -3,7 +3,7 @@
#ifndef SCM_FPORTS_H
#define SCM_FPORTS_H
-/* Copyright (C) 1995,1996,1997,1998,1999,2000,2001, 2006, 2008 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1997,1998,1999,2000,2001, 2006, 2008, 2009 Free Software Foundation, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public License
@@ -59,8 +59,6 @@ SCM_INTERNAL void scm_init_fports (void);
/* internal functions */
SCM_INTERNAL SCM scm_i_fdes_to_port (int fdes, long mode_bits, SCM name);
-SCM_INTERNAL int scm_i_fport_truncate (SCM, SCM);
-SCM_INTERNAL SCM scm_i_fport_seek (SCM, SCM, int);
#endif /* SCM_FPORTS_H */
diff --git a/libguile/ports.c b/libguile/ports.c
index 98207b0..627fd3f 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -1390,12 +1390,7 @@ SCM_DEFINE (scm_seek, seek, 3, 0, 0,
if (how != SEEK_SET how != SEEK_CUR how != SEEK_END)
SCM_OUT_OF_RANGE (3, whence);
- if (SCM_OPFPORTP (fd_port))
-{
- /* go direct to fport code to allow
Re: guile 1.9.0 scm_read_hash_extend gc trouble
Can you confirm that it allows you to build Guile with 32-bit offsets and Snd with 64-bit offsets (or vice versa)? Yes, I think it is ok.
Re: guile 1.9.0 scm_read_hash_extend gc trouble
() Bill Schottstaedt b...@ccrma.stanford.edu
() Sun, 21 Jun 2009 05:10:09 -0700
code to implement #|..|# block comment processing triggers either a
glibc memory complaint or a segfault.
FWIW, below is the implementation from Guile 1.4.1.118 (not yet released).
It handles nesting (per R6RS, i believe) and a weird lookahead case.
thi
_
/* Skip #|...|# block comments. */
static void
skip_hashpipe_block_comment (SCM port)
{
#define FUNC_NAME s_scm_read
int c;
bool pipep = false;
int oline = SCM_LINUM (port);
int ocol = SCM_COL (port);
for (;;)
{
if (EOF == (c = GETC ()))
toosoon:
{
char buf[149];
snprintf (buf, 149, %s\n%s:%d:%d: (starting here),
unterminated `#| ... |#' comment,
c_port_filename (port),
1 + oline, ocol - 1);
BADNESS (0, buf);
}
if (pipep '#' == c)
return;
/* Handle nested comments. */
retry:
if ('#' == c)
{
if (EOF == (c = GETC ()))
goto toosoon;
if ('|' == c)
{
skip_hashpipe_block_comment (port);
pipep = false;
}
else
/* Don't get fooled by ##|...|# (ugh). */
goto retry;
}
pipep = ('|' == c);
}
#undef FUNC_NAME
}
Re: guile 1.9.0 scm_read_hash_extend gc trouble
This is trickier than I thought -- it's not the GC after all. In my configure script for Snd, autoconf defines _FILE_OFFSET_BITS to 64 if large files are implemented. The config.h include precedes everything else. When scm_getc is called within guile, it thinks sizeof(scm_t_port) is 92. Everything is fine until my skip comment procedure is called (with this 92 byte struct) -- it thinks the scm_t_port size is 104, and fields like rw_active (in scm_getc which is apparently expanded inline) are not where it expects them to be! If I undef _FILE_OFFSET_BITS, both agree on the struct size, and there is no problem.
Re: guile 1.9.0 scm_read_hash_extend gc trouble
Can you spot some way in which what you are doing is different to this? I believe this is a GC problem; you're doing exactly what I'm doing, but in a context where the GC is not called. If I place the skip comment function in its own file, and compile it with optimization turned off, everything is happy; if optimization is on (either -O or -O2), it dies. According to valgrind the problem is in scm_getc -- the SCM_PTAB_ENTRY pointer pt does not point to a valid structure, so the read and subsuequent write through pt goes off into unallocated memory. I haven't tracked down the actual problem yet, but gc-protecting the port variable does no good.
Re: guile 1.9.0 scm_read_hash_extend gc trouble
Bill Schottstaedt b...@ccrma.stanford.edu writes: I believe this is a GC problem; you're doing exactly what I'm doing, but in a context where the GC is not called. If I place the skip comment function in its own file, and compile it with optimization turned off, everything is happy; if optimization is on (either -O or -O2), it dies. I tried compiling my test with -O2; unfortunately it's still OK. According to valgrind the problem is in scm_getc -- the SCM_PTAB_ENTRY pointer pt does not point to a valid structure, so the read and subsuequent write through pt goes off into unallocated memory. I haven't tracked down the actual problem yet, but gc-protecting the port variable does no good. Do you mean you think that pt has already been freed, or that it was never valid? If the former, it sounds like it would be worth trying to prove this, by adding instrumentation to scm_gc_free that prints the pointer to stdout whenever what is port. You could also instrument (or set a breakpoint on) scm_igc, to investigate the GC factor. I'm sorry that we've apparently broken this for you, and that I can't help more yet... Regards, Neil
Re: guile 1.9.0 scm_read_hash_extend gc trouble
Bill Schottstaedt b...@ccrma.stanford.edu writes: static SCM g_skip_block_comment(SCM ch, SCM port) [...] Then call scm_read_hash_extend with | to activate it. How do you wrap g_skip_block_comment to get a suitable argument for scm_read_hash_extend () ? Regards, Neil
Re: guile 1.9.0 scm_read_hash_extend gc trouble
Neil Jerram n...@ossau.uklinux.net writes:
Bill Schottstaedt b...@ccrma.stanford.edu writes:
static SCM g_skip_block_comment(SCM ch, SCM port)
[...]
Then call scm_read_hash_extend with | to activate it.
How do you wrap g_skip_block_comment to get a suitable argument for
scm_read_hash_extend () ?
I had a go using scm_c_make_subr (). But with the attached program
and test input (and current Git HEAD) I can't reproduce the seg fault
that you described. Can you spot some way in which what you are doing
is different to this?
Regards,
Neil
#include libguile.h
static SCM g_skip_block_comment(SCM ch, SCM port)
{
int bang_seen = 0;
while (1)
{
int c;
c = scm_getc(port);
if (c == EOF)
{
fprintf(stderr, unterminated `#| ... |#' comment);
return(SCM_BOOL_F);
}
if (c == '|')
bang_seen = 1;
else
{
if ((c == '#') (bang_seen))
return(SCM_BOOL_F);
else bang_seen = 0;
}
}
return(SCM_BOOL_F);
}
void inner_main (void *closure, int argc, char **argv)
{
scm_read_hash_extend (scm_integer_to_char (scm_from_char ('|')),
scm_c_make_subr (skip-comment,
scm_tc7_subr_2,
g_skip_block_comment));
scm_shell (argc, argv);
}
int main (int argc, char **argv)
{
scm_boot_guile (argc, argv, inner_main, NULL);
}
/*
Local variables:
compile-command: gcc bill.c -o bill
End:
*/
bill.scm
Description: Binary data
