Re: guile 1.9.0 scm_read_hash_extend gc trouble

[ludo@gnu.org (Ludovic Courtès)]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludo@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'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 4032 bytes --]

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 64-bit offsets */
-      return scm_i_fport_seek (fd_port, offset, how);
-    }
-  else if (SCM_OPPORTP (fd_port))
+  if (SCM_OPPORTP (fd_port))
     {
       scm_t_ptob_descriptor *ptob = scm_ptobs + SCM_PTOBNUM (fd_port);
       off_t_or_off64_t off = scm_to_off_t_or_off64_t (offset);
@@ -1488,11 +1483,6 @@ SCM_DEFINE (scm_truncate_file, "truncate-file", 1, 1, 0,
       SCM_SYSCALL (rv = ftruncate_or_ftruncate64 (scm_to_int (object),
                                                   c_length));
     }
-  else if (SCM_OPOUTFPORTP (object))
-    {
-      /* go direct to fport code to allow 64-bit offsets */
-      rv = scm_i_fport_truncate (object, length);
-    }
   else if (SCM_OPOUTPORTP (object))
     {
       off_t_or_off64_t c_length = scm_to_off_t_or_off64_t (length);