`scm_c_read ()' and `swap_buffer' trick harmful

`scm_c_read ()' and `swap_buffer' trick harmful

[Ludovic Courtès]

Hello!

I just discovered undesirable side effects of commit
b5cb4464ca4e23d077a9777bbc17835feb0f4374 "Make multi-byte reads on
unbuffered ports more efficient."

An example application that breaks in the presence of this patch are
"custom binary input ports" (aka. CBIPs [0]) in Guile-R6RS-Libs [1].  The
CBIP implementation [2] works as follows:

  1. make_cbip ()
       /* Create a bytevector for use as the CBIP's internal buffer.  */
       SCM bv = scm_r6rs_c_make_bytevector (c_len);
       c_bv = (char *) SCM_R6RS_BYTEVECTOR_CONTENTS (bv);
       c_port->read_pos = c_port->read_buf = (unsigned char *) c_bv;
       c_port->read_end = (unsigned char *) c_bv;

       /* Store BV for later reuse.  */
       SCM_SETSTREAM (port, SCM_UNPACK (bv, and other things));

  2. cbip_fill_input (port)
       if (c_port->read_pos >= c_port->read_end)
         {
           /* Invoke the user's `read!' procedure.  */
           bv = SCM_R6RS_CBIP_BYTEVECTOR (port);

           octets = scm_call_3 (read_proc, bv, SCM_INUM0,
                                SCM_I_MAKINUM (CBIP_BUFFER_SIZE));
           c_octets = scm_to_uint (octets);

           c_port->read_pos = (unsigned char *) SCM_R6RS_BYTEVECTOR_CONTENTS (bv);
           c_port->read_end = (unsigned char *) c_port->read_pos + c_port->c_octets;
         }

IOW, the CBIP `fill_input' method does *not* directly pass
`c_port->read_buf' to the user's `read!' method but instead passes it
its bytevector, which it assumes to wrap its internal.  Thus, if
`c_port->read_buf' happens to point to something other than BV's
contents, it is just left untouched.

Worse, `cbip_fill_input ()' updates `read_pos' and `read_end' but does
not touch `read_buf', leading to an inconsistent state that will confuse
later `scm_fill_input ()' calls on that port (e.g., in the loop for
`scm_c_read ()'), and possibly to heap corruption.

So where to go from here?  I think this example shows that the
`swap_buffer' trick is too risky, unfortunately.  Thus, we may need to
revert it, at least in 1.8.  Second, I think that a `read' method as a
replacement for `fill_input', as I proposed back then [3], would be
safer; maybe 1.9 would be a nice place to add it.  Neil: what do you
think?

Thanks,
Ludo'.

[0] http://www.r6rs.org/final/html/r6rs-lib/r6rs-lib-Z-H-1.html#node_toc_node_sec_8.2.7

[1] http://www.fdn.fr/~lcourtes/software/guile/guile-r6rs-libs-0.1.tar.gz
    and http://repo.or.cz/w/guile-r6rs-libs.git

[2] http://repo.or.cz/w/guile-r6rs-libs.git?a=blob;f=src/ports.c;#l210

[3] http://thread.gmane.org/gmane.lisp.guile.devel/7292

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Neil Jerram]

2008/11/15 Ludovic Courtès <ludo@gnu.org>:
>
> So where to go from here?  I think this example shows that the
> `swap_buffer' trick is too risky, unfortunately.  Thus, we may need to
> revert it, at least in 1.8.  Second, I think that a `read' method as a
> replacement for `fill_input', as I proposed back then [3], would be
> safer; maybe 1.9 would be a nice place to add it.  Neil: what do you
> think?

I think this means we should only do the swap_buffer trick for
unbuffered ports.  That would solve the original problem, and avoid
breaking cases like this.

It _could_ be argued that cbip_fill_input() should take more care to
use the port's actual current buffer.  e.g. it should construct a new
R6RS bytevector around the current port->read_buf, and pass that to
the read! procedure.  But I'm not going to argue that, because the
generalization of that would be to say that C code that implements a
buffered port cannot assume that the buffer stays as originally set;
and, with hindsight, it seems that C code _should_ be able to rely on
this.  (Plus constructing a new bytevector would probably be bad for
performance.)

I recall you queried before whether we should apply this fix to
buffered ports too, and I confidently said yes; sorry about that!

Do you agree that doing the trick only for unbuffered ports would be a
good approach?  If so I'll see what that means in terms of code.

       Neil

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Ludovic Courtès]

Hello Neil,

"Neil Jerram" <neiljerram@googlemail.com> writes:

> 2008/11/15 Ludovic Courtès <ludo@gnu.org>:
>>
>> So where to go from here?  I think this example shows that the
>> `swap_buffer' trick is too risky, unfortunately.  Thus, we may need to
>> revert it, at least in 1.8.  Second, I think that a `read' method as a
>> replacement for `fill_input', as I proposed back then [3], would be
>> safer; maybe 1.9 would be a nice place to add it.  Neil: what do you
>> think?
>
> I think this means we should only do the swap_buffer trick for
> unbuffered ports.  That would solve the original problem, and avoid
> breaking cases like this.

In theory, yes.  In practice, the notion of "unbuffered port" is
ill-defined, I'm afraid.  The `SCM_BUF0' flag probably can't be relied
on, as it appears to be only really used on `fports.c'.  Actually, for
some reason (probably copy & paste), make_cbip() creates ports with
"SCM_OPN | SCM_RDNG | SCM_BUF0", although `SCM_BUF0' is probably not
needed.  So I think "unbuffered port" means "read_buf_size <= 1".

> It _could_ be argued that cbip_fill_input() should take more care to
> use the port's actual current buffer.  e.g. it should construct a new
> R6RS bytevector around the current port->read_buf, and pass that to
> the read! procedure.  But I'm not going to argue that, because the
> generalization of that would be to say that C code that implements a
> buffered port cannot assume that the buffer stays as originally set;
> and, with hindsight, it seems that C code _should_ be able to rely on
> this.  (Plus constructing a new bytevector would probably be bad for
> performance.)

Exactly: it's nice to be able to avoid the allocation of a new
bytevector.

> I recall you queried before whether we should apply this fix to
> buffered ports too, and I confidently said yes; sorry about that!

No problem.  The port "API" exposes too much internals, which makes it
hard to change it without breaking existing code.

> Do you agree that doing the trick only for unbuffered ports would be a
> good approach?  If so I'll see what that means in terms of code.

I think it *should* work, provided `SCM_BUF0' isn't relied on (at least
for the particular case I'm interested in...).

Thanks!

Ludo'.

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Neil Jerram]

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

2008/11/20 Ludovic Courtès <ludo@gnu.org>:
>
> In theory, yes.  In practice, the notion of "unbuffered port" is
> ill-defined, I'm afraid.  The `SCM_BUF0' flag probably can't be relied
> on, as it appears to be only really used on `fports.c'.  Actually, for
> some reason (probably copy & paste), make_cbip() creates ports with
> "SCM_OPN | SCM_RDNG | SCM_BUF0", although `SCM_BUF0' is probably not
> needed.  So I think "unbuffered port" means "read_buf_size <= 1".

That's what I was thinking too.  Please see the attached.

    Neil

[-- Attachment #2: 0001-Make-scm_c_read-use-caller-buffer-only-for-unbuffere.patch --]
[-- Type: text/plain, Size: 4372 bytes --]

From 996384a935224baa2480ab02e50faa91e24b5613 Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Thu, 20 Nov 2008 21:49:35 +0000
Subject: [PATCH] Make scm_c_read use caller buffer only for unbuffered ports.

We recently modified scm_c_read so that it temporarily swaps the
caller's buffer with the port's normal read buffer, in order to
improve performance in the case where the port is unbuffered (which
actually means having a single-byte buffer) - but we implemented the
swap in the buffered case too.  The latter turns out to be a bad idea
- because it means that the C code of a custom port implementation
cannot rely on a port's buffer always being the same as when it was
first set up - and so this commit reverts that.  The buffer swapping
trick now applies to unbuffered ports only.
---
 libguile/ports.c |   72 ++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/libguile/ports.c b/libguile/ports.c
index 2b1c5e1..bfccd0b 100644
--- a/libguile/ports.c
+++ b/libguile/ports.c
@@ -1100,32 +1100,56 @@ scm_c_read (SCM port, void *buffer, size_t size)
   /* Now we will call scm_fill_input repeatedly until we have read the
      requested number of bytes.  (Note that a single scm_fill_input
      call does not guarantee to fill the whole of the port's read
-     buffer.)  For these calls, since we already have a buffer here to
-     read into, we bypass the port's own read buffer (if it has one),
-     by saving it off and modifying the port structure to point to our
-     own buffer.
-
-     We need to make sure that the port's normal buffer is reinstated
-     in case one of the scm_fill_input () calls throws an exception;
-     we use the scm_dynwind_* API to achieve that. */
-  psb.pt = pt;
-  psb.buffer = buffer;
-  psb.size = size;
-  scm_dynwind_begin (SCM_F_DYNWIND_REWINDABLE);
-  scm_dynwind_rewind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
-  scm_dynwind_unwind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
-
-  /* Call scm_fill_input until we have all the bytes that we need, or
-     we hit EOF. */
-  while (pt->read_buf_size && (scm_fill_input (port) != EOF))
+     buffer.) */
+  if (pt->read_buf_size <= 1)
     {
-      pt->read_buf_size -= (pt->read_end - pt->read_pos);
-      pt->read_pos = pt->read_buf = pt->read_end;
-    }
-  n_read += pt->read_buf - (unsigned char *) buffer;
+      /* The port that we are reading from is unbuffered - i.e. does
+	 not have its own persistent buffer - but we have a buffer,
+	 provided by our caller, that is the right size for the data
+	 that is wanted.  For the following scm_fill_input calls,
+	 therefore, we use the buffer in hand as the port's read
+	 buffer.
+
+	 We need to make sure that the port's normal (1 byte) buffer
+	 is reinstated in case one of the scm_fill_input () calls
+	 throws an exception; we use the scm_dynwind_* API to achieve
+	 that. */
+      psb.pt = pt;
+      psb.buffer = buffer;
+      psb.size = size;
+      scm_dynwind_begin (SCM_F_DYNWIND_REWINDABLE);
+      scm_dynwind_rewind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
+      scm_dynwind_unwind_handler (swap_buffer, &psb, SCM_F_WIND_EXPLICITLY);
+
+      /* Call scm_fill_input until we have all the bytes that we need,
+	 or we hit EOF. */
+      while (pt->read_buf_size && (scm_fill_input (port) != EOF))
+	{
+	  pt->read_buf_size -= (pt->read_end - pt->read_pos);
+	  pt->read_pos = pt->read_buf = pt->read_end;
+	}
+      n_read += pt->read_buf - (unsigned char *) buffer;
 
-  /* Reinstate the port's normal buffer. */
-  scm_dynwind_end ();
+      /* Reinstate the port's normal buffer. */
+      scm_dynwind_end ();
+    }
+  else
+    {
+      /* The port has its own buffer.  It is important that we use it,
+	 even if it happens to be smaller than our caller's buffer, so
+	 that a custom port implementation's entry points (in
+	 particular, fill_input) can rely on the buffer always being
+	 the same as they first set up. */
+      while (size && (scm_fill_input (port) != EOF))
+	{
+	  n_available = min (size, pt->read_end - pt->read_pos);
+	  memcpy (buffer, pt->read_pos, n_available);
+	  buffer = (char *) buffer + n_available;
+	  pt->read_pos += n_available;
+	  n_read += n_available;
+	  size -= n_available;
+	} 
+    }
 
   return n_read;
 }
-- 
1.5.6.5

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Ludovic Courtès]

Hi Neil,

"Neil Jerram" <neiljerram@googlemail.com> writes:

> 2008/11/20 Ludovic Courtès <ludo@gnu.org>:
>>
>> In theory, yes.  In practice, the notion of "unbuffered port" is
>> ill-defined, I'm afraid.  The `SCM_BUF0' flag probably can't be relied
>> on, as it appears to be only really used on `fports.c'.  Actually, for
>> some reason (probably copy & paste), make_cbip() creates ports with
>> "SCM_OPN | SCM_RDNG | SCM_BUF0", although `SCM_BUF0' is probably not
>> needed.  So I think "unbuffered port" means "read_buf_size <= 1".
>
> That's what I was thinking too.  Please see the attached.

Thanks for the quick fix!

I confirm it works with CBIPs in Guile-R6RS-Libs, but I had to make this
small change:

  http://repo.or.cz/w/guile-r6rs-libs.git?a=commitdiff;h=dfab2fe21f82bc40cfdf9a3a3eeb6936d5935c8c

IOW, with Guile up to 1.8.5, it was harmless to leave `read_buf_size'
uninitialized (actually, zeroed), which was OK since CBIPs don't use it,
but the change in `scm_c_read ()' makes it necessary to initialize it.
It's probably a reasonable expectation, though.

Please commit!

Thanks,
Ludo'.

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Ludovic Courtès]

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

Hello,

ludo@gnu.org (Ludovic Courtès) writes:

> Thanks for the quick fix!

Attach is a test case for this, please apply (with git-am(1)) and commit
right after your fix.

Once we're done with this, I think we should release 1.8.6.  I'm should
be able to do it this week-end.

Thanks,
Ludo'.


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

From ca72bdfec2457b035836bc43afbc5ea20fe8af02 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Sat, 22 Nov 2008 15:48:16 +0100
Subject: [PATCH] Add C unit test for `scm_c_read ()' and the port API.

* test-suite/standalone/Makefile.am (check_PROGRAMS, TESTS): Add
  `test-scm-c-read'.
  (test_scm_c_read_SOURCES, test_scm_c_read_CFLAGS,
  test_scm_c_read_LDADD): New.
---
 test-suite/standalone/.gitignore        |    1 +
 test-suite/standalone/Makefile.am       |    7 ++
 test-suite/standalone/test-scm-c-read.c |  130 +++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 0 deletions(-)
 create mode 100644 test-suite/standalone/test-scm-c-read.c

diff --git a/test-suite/standalone/.gitignore b/test-suite/standalone/.gitignore
index 78af304..b1f4088 100644
--- a/test-suite/standalone/.gitignore
+++ b/test-suite/standalone/.gitignore
@@ -7,3 +7,4 @@
 /test-with-guile-module
 /test-use-srfi
 /test-scm-with-guile
+/test-scm-c-read
diff --git a/test-suite/standalone/Makefile.am b/test-suite/standalone/Makefile.am
index 6ae5e88..3afd7e5 100644
--- a/test-suite/standalone/Makefile.am
+++ b/test-suite/standalone/Makefile.am
@@ -107,6 +107,13 @@ TESTS += test-conversion
 check_SCRIPTS += test-use-srfi
 TESTS += test-use-srfi
 
+# test-scm-c-read
+test_scm_c_read_SOURCES = test-scm-c-read.c
+test_scm_c_read_CFLAGS = ${test_cflags}
+test_scm_c_read_LDADD = ${top_builddir}/libguile/libguile.la
+check_PROGRAMS += test-scm-c-read
+TESTS += test-scm-c-read
+
 if BUILD_PTHREAD_SUPPORT
 
 # test-with-guile-module
diff --git a/test-suite/standalone/test-scm-c-read.c b/test-suite/standalone/test-scm-c-read.c
new file mode 100644
index 0000000..be8ea80
--- /dev/null
+++ b/test-suite/standalone/test-scm-c-read.c
@@ -0,0 +1,130 @@
+/* Copyright (C) 2008 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 as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* Exercise `scm_c_read ()' and the port type API.  Verify assumptions that
+   can be made by port type implementations.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <libguile.h>
+#include <assert.h>
+
+
+\f
+/* Size of our port's internal buffer.  */
+#define PORT_BUFFER_SIZE 1024
+
+/* Return a new port of type PORT_TYPE.  */
+static inline SCM
+make_port (scm_t_bits port_type)
+{
+  SCM port;
+  char *c_buffer;
+  scm_t_port *c_port;
+
+  c_buffer = scm_gc_calloc (PORT_BUFFER_SIZE, "custom-port-buffer");
+
+  port = scm_new_port_table_entry (port_type);
+
+  /* Attach it the underlying u8vector, which we'll use as PORT's buffer.  */
+  SCM_SETSTREAM (port, (scm_t_bits) c_buffer);
+
+  /* Have the port directly access the buffer (bytevector).  */
+  c_port = SCM_PTAB_ENTRY (port);
+  c_port->read_pos = c_port->read_buf = (unsigned char *) c_buffer;
+  c_port->read_end = (unsigned char *) c_buffer + PORT_BUFFER_SIZE;
+  c_port->read_buf_size = PORT_BUFFER_SIZE;
+
+  /* Mark PORT as open and readable.  */
+  SCM_SET_CELL_TYPE (port, port_type | SCM_OPN | SCM_RDNG);
+
+  return port;
+}
+
+/* Read one byte from PORT.  */
+static int
+fill_input (SCM port)
+{
+  int result;
+  scm_t_port *c_port = SCM_PTAB_ENTRY (port);
+
+  /* Make sure that C_PORT's internal buffer wasn't changed behind our back.
+     See http://lists.gnu.org/archive/html/guile-devel/2008-11/msg00042.html
+     for an example where this assumption matters.  */
+  assert (c_port->read_buf == (unsigned char *) SCM_STREAM (port));
+  assert (c_port->read_buf_size == PORT_BUFFER_SIZE);
+
+  if (c_port->read_pos >= c_port->read_end)
+    result = EOF;
+  else
+    result = (int) *c_port->read_pos++;
+
+  return result;
+}
+
+/* Return true (non-zero) if BUF contains only zeros.  */
+static inline int
+zeroed_buffer_p (const char *buf, size_t len)
+{
+  size_t i;
+
+  for (i = 0; i < len; i++)
+    if (buf[i] != 0)
+      return 0;
+
+  return 1;
+}
+
+/* Run the test.  */
+static void *
+do_start (void *arg)
+{
+  SCM port;
+  scm_t_bits port_type;
+  char buffer[PORT_BUFFER_SIZE + (PORT_BUFFER_SIZE / 2)];
+  size_t read, last_read;
+
+  port_type = scm_make_port_type ("custom-input-port", fill_input, NULL);
+  port = make_port (port_type);
+
+  read = 0;
+  do
+    {
+      last_read = scm_c_read (port, &buffer[read], 123);
+      assert (last_read <= 123);
+      assert (zeroed_buffer_p (&buffer[read], last_read));
+
+      read += last_read;
+    }
+  while (last_read > 0 && read < sizeof (buffer));
+
+  /* We shouldn't be able to read more than what's in PORT's buffer.  */
+  assert (read == PORT_BUFFER_SIZE);
+
+  return NULL;
+}
+
+\f
+int
+main (int argc, char *argv[])
+{
+  scm_with_guile (do_start, NULL);
+
+  return 0;
+}
-- 
1.6.0.3

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Neil Jerram]

2008/11/22 Ludovic Courtès <ludo@gnu.org>:
> Hello,
>
> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Thanks for the quick fix!
>
> Attach is a test case for this, please apply (with git-am(1)) and commit
> right after your fix.

Cool, committing this now also.

> Once we're done with this, I think we should release 1.8.6.  I'm should
> be able to do it this week-end.

I'm afraid there's not much week-end left now, but feel free to proceed!

       Neil

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Neil Jerram]

2008/11/21 Ludovic Courtès <ludo@gnu.org>:
> Hi Neil,
>
> "Neil Jerram" <neiljerram@googlemail.com> writes:
>
>> 2008/11/20 Ludovic Courtès <ludo@gnu.org>:
>>>
>>> In theory, yes.  In practice, the notion of "unbuffered port" is
>>> ill-defined, I'm afraid.  The `SCM_BUF0' flag probably can't be relied
>>> on, as it appears to be only really used on `fports.c'.  Actually, for
>>> some reason (probably copy & paste), make_cbip() creates ports with
>>> "SCM_OPN | SCM_RDNG | SCM_BUF0", although `SCM_BUF0' is probably not
>>> needed.  So I think "unbuffered port" means "read_buf_size <= 1".
>>
>> That's what I was thinking too.  Please see the attached.
>
> Thanks for the quick fix!
>
> I confirm it works with CBIPs in Guile-R6RS-Libs, but I had to make this
> small change:
>
>  http://repo.or.cz/w/guile-r6rs-libs.git?a=commitdiff;h=dfab2fe21f82bc40cfdf9a3a3eeb6936d5935c8c
>
> IOW, with Guile up to 1.8.5, it was harmless to leave `read_buf_size'
> uninitialized (actually, zeroed), which was OK since CBIPs don't use it,
> but the change in `scm_c_read ()' makes it necessary to initialize it.
> It's probably a reasonable expectation, though.

Hmm.  I agree that this isn't ideal.  I guess it's OK for now though.

> Please commit!

Thanks, doing that now.

       Neil

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Miroslav Lichvar]

On Sun, Nov 23, 2008 at 10:30:19PM +0000, Neil Jerram wrote:
> 2008/11/21 Ludovic Courtès <ludo@gnu.org>:
> > I confirm it works with CBIPs in Guile-R6RS-Libs, but I had to make this
> > small change:
> >
> >  http://repo.or.cz/w/guile-r6rs-libs.git?a=commitdiff;h=dfab2fe21f82bc40cfdf9a3a3eeb6936d5935c8c
> >
> > IOW, with Guile up to 1.8.5, it was harmless to leave `read_buf_size'
> > uninitialized (actually, zeroed), which was OK since CBIPs don't use it,
> > but the change in `scm_c_read ()' makes it necessary to initialize it.
> > It's probably a reasonable expectation, though.
> 
> Hmm.  I agree that this isn't ideal.  I guess it's OK for now though.
> 
> > Please commit!
> 
> Thanks, doing that now.

It seems that the optimizations broke lilypond, it's now failing with
this message:

ports.c:978: scm_fill_input: Assertion `pt->read_pos == pt->read_end' failed.

A bug report is here:
https://bugzilla.redhat.com/show_bug.cgi?id=477007

Reverting bed2e15f and 1d17348b fixes the problem, is this a lilypond
or guile bug?

Thanks,

-- 
Miroslav Lichvar

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Ludovic Courtès]

Hello,

Miroslav Lichvar <mlichvar@redhat.com> writes:

> On Sun, Nov 23, 2008 at 10:30:19PM +0000, Neil Jerram wrote:
>> 2008/11/21 Ludovic Courtès <ludo@gnu.org>:
>> > I confirm it works with CBIPs in Guile-R6RS-Libs, but I had to make this
>> > small change:
>> >
>> >  http://repo.or.cz/w/guile-r6rs-libs.git?a=commitdiff;h=dfab2fe21f82bc40cfdf9a3a3eeb6936d5935c8c
>> >
>> > IOW, with Guile up to 1.8.5, it was harmless to leave `read_buf_size'
>> > uninitialized (actually, zeroed), which was OK since CBIPs don't use it,
>> > but the change in `scm_c_read ()' makes it necessary to initialize it.
>> > It's probably a reasonable expectation, though.
>> 
>> Hmm.  I agree that this isn't ideal.  I guess it's OK for now though.
>> 
>> > Please commit!
>> 
>> Thanks, doing that now.
>
> It seems that the optimizations broke lilypond, it's now failing with
> this message:
>
> ports.c:978: scm_fill_input: Assertion `pt->read_pos == pt->read_end' failed.

Ouch!

> A bug report is here:
> https://bugzilla.redhat.com/show_bug.cgi?id=477007

Can you get a full C backtrace?

A quick grep reveals that Lilypond does not create any custom port type,
which isn't reassuring...

Thanks,
Ludo'.

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Miroslav Lichvar]

On Fri, Dec 19, 2008 at 09:25:27PM +0100, Ludovic Courtès wrote:
> > It seems that the optimizations broke lilypond, it's now failing with
> > this message:
> >
> > ports.c:978: scm_fill_input: Assertion `pt->read_pos == pt->read_end' failed.
> 
> Ouch!
> 
> > A bug report is here:
> > https://bugzilla.redhat.com/show_bug.cgi?id=477007
> 
> Can you get a full C backtrace?

Here it is, let me know if you need anything else.

(gdb) bt
#0  0x00007f6b232b6f05 in raise () from /lib64/libc.so.6
#1  0x00007f6b232b8a73 in abort () from /lib64/libc.so.6
#2  0x00007f6b232afef9 in __assert_fail () from /lib64/libc.so.6
#3  0x0000000000d52b92 in scm_fill_input (port=0x7f6b1d4b7420) at ports.c:978
#4  0x0000000000638b66 in internal_ly_parse_scm (ps=0x7fff2b827bc0) at parse-scm.cc:43
#5  0x0000000000638d38 in ly_parse_scm (
    s=0x1fe907c "(if (and #t (defined? 'set-debug-cell-accesses!))\n  (set-debug-cell-accesses! 5000))\n\n\\version \"2.10.0\"\n\n\\include \"declarations-init.ly\"\n\n\n#(ly:set-option 'old-relative #f)\n#(define toplevel-scores (l"..., n=0x7fff2b827dcc, i=
      {start_ = 0x1fe907c "(if (and #t (defined? 'set-debug-cell-accesses!))\n  (set-debug-cell-accesses! 5000))\n\n\\version \"2.10.0\"\n\n\\include \"declarations-init.ly\"\n\n\n#(ly:set-option 'old-relative #f)\n#(define toplevel-scores (l"..., end_ = 0x1fe907c "(if (and #t (defined? 'set-debug-cell-accesses!))\n  (set-debug-cell-accesses! 5000))\n\n\\version \"2.10.0\"\n\n\\include \"declarations-init.ly\"\n\n\n#(ly:set-option 'old-relative #f)\n#(define toplevel-scores (l"..., source_file_ = 0x208cdd0}, safe=false, parser=0x2083ee0)
    at parse-scm.cc:131
#6  0x0000000000751c0b in Lily_lexer::yylex (this=0x1fc2d40) at lexer.ll:340
#7  0x000000000075506e in yylex (s=0x7fff2b82a4a0, loc=0x7fff2b82a480, v=0x2083ee0) at parser.yy:2696
#8  0x000000000075680f in yyparse (my_lily_parser=0x2083ee0) at out/parser.cc:2612
#9  0x0000000000762d31 in Lily_parser::do_yyparse (this=0x2083ee0) at parser.yy:2476
#10 0x00000000005a3278 in Lily_parser::parse_file (this=0x2083ee0, init=
        {static npos = 18446744073709551615, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x2000f38 "init.ly"}}, name=
        {static npos = 18446744073709551615, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x2083a18 "/tmp/lilypond-2.11.65/ly/generate-documentation.ly"}}, out_name=
        {static npos = 18446744073709551615, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x2040a18 "generate-documentation"}})
    at lily-parser.cc:114
#11 0x00000000005a1118 in ly_parse_file (name=0x7f6b21d1b260) at lily-parser-scheme.cc:124
#12 0x0000000000d1d2b4 in deval (x=0x404, env=0x7f6b1d4b57b0) at eval.c:4232
#13 0x0000000000d1f512 in scm_dapply (proc=0x7f6b1d4b58c0, arg1=0x404, args=0x7f6b1d4b57b0) at eval.c:5012
#14 0x0000000000d13e73 in scm_apply (proc=0x7f6b1d4b58a0, arg1=0x404, args=0x404) at eval.c:4811
#15 0x0000000000d13bb8 in scm_call_0 (proc=0x7f6b1d4b58a0) at eval.c:4666
#16 0x0000000000d826b1 in scm_body_thunk (body_data=0x7fff2b82d100) at throw.c:355
#17 0x0000000000d821c9 in scm_c_catch (tag=0x7f6b21d66860, body=0xd82690 <scm_body_thunk>, 
    body_data=0x7fff2b82d100, handler=0xd826b3 <scm_handle_by_proc>, handler_data=0x7fff2b82d0e8, 
    pre_unwind_handler=0, pre_unwind_handler_data=0x7fff2b82d0e0) at throw.c:203
#18 0x0000000000d82b72 in scm_catch_with_pre_unwind_handler (key=0x7f6b21d66860, thunk=0x7f6b1d4b58a0, 
    handler=0x7f6b1d4b5830, pre_unwind_handler=0x204) at throw.c:587
#19 0x0000000000d3438c in scm_gsubr_apply (args=0x404) at gsubr.c:223
#20 0x0000000000d1efcc in scm_dapply (proc=0x7f6b21fb84f0, arg1=0x7f6b229ab8e0, args=0x7f6b1d4b5800)
    at eval.c:4930
#21 0x0000000000d1dda2 in deval (x=0x7f6b20320f90, env=0x7f6b1d4b5950) at eval.c:4378
#22 0x0000000000d17c02 in deval (x=0x7f6b20324010, env=0x7f6b1d4b5a60) at eval.c:3397
#23 0x0000000000d1f512 in scm_dapply (proc=0x7f6b1d4b3db0, arg1=0x404, args=0x7f6b1d4b3d20) at eval.c:5012
#24 0x0000000000d13e73 in scm_apply (proc=0x7f6b1d4b3d90, arg1=0x7f6b21d1b260, args=0x7f6b229aa700)
    at eval.c:4811
#25 0x0000000000d13be2 in scm_call_1 (proc=0x7f6b1d4b3d90, arg1=0x7f6b21d1b260) at eval.c:4672
#26 0x00007f6b20ad295b in scm_srfi1_for_each (proc=0x7f6b1d4b3d90, arg1=0x7f6b20283060, args=0x404)
    at srfi-1.c:1516
#27 0x0000000000d1dca3 in deval (x=0x404, env=0x7f6b20282930) at eval.c:4367
#28 0x0000000000d17c02 in deval (x=0x7f6b20323790, env=0x7f6b20282930) at eval.c:3397
#29 0x0000000000d19c96 in deval (x=0x7f6b20282b60, env=0x7f6b20282fa0) at eval.c:3648
#30 0x0000000000d1f512 in scm_dapply (proc=0x7f6b203290f0, arg1=0x404, args=0x7f6b20282fa0) at eval.c:5012
#31 0x0000000000d13e73 in scm_apply (proc=0x7f6b20328f40, arg1=0x7f6b20283060, args=0x7f6b229aa700)
    at eval.c:4811
#32 0x0000000000d13be2 in scm_call_1 (proc=0x7f6b20328f40, arg1=0x7f6b20283060) at eval.c:4672
#33 0x00000000005bb2a1 in main_with_guile () at main.cc:426
#34 0x0000000000d38aee in invoke_main_func (body_data=0x7fff2b830890) at init.c:367
#35 0x0000000000cfc9ea in c_body (d=0x7fff2b8307d0) at continuations.c:349
#36 0x0000000000d821c9 in scm_c_catch (tag=0x104, body=0xcfc9c5 <c_body>, body_data=0x7fff2b8307d0, 
    handler=0xcfc9fc <c_handler>, handler_data=0x7fff2b8307d0, 
    pre_unwind_handler=0xd82a02 <scm_handle_by_message_noexit>, pre_unwind_handler_data=0x0) at throw.c:203
#37 0x0000000000cfc98e in scm_i_with_continuation_barrier (body=0xcfc9c5 <c_body>, 
    body_data=0x7fff2b8307d0, handler=0xcfc9fc <c_handler>, handler_data=0x7fff2b8307d0, 
    pre_unwind_handler=0xd82a02 <scm_handle_by_message_noexit>, pre_unwind_handler_data=0x0)
    at continuations.c:325
#38 0x0000000000cfca72 in scm_c_with_continuation_barrier (func=0xd38aa1 <invoke_main_func>, 
    data=0x7fff2b830890) at continuations.c:367
#39 0x0000000000d8035d in scm_i_with_guile_and_parent (func=0xd38aa1 <invoke_main_func>, 
    data=0x7fff2b830890, parent=0x0) at threads.c:726
#40 0x0000000000d8032a in scm_with_guile (func=0xd38aa1 <invoke_main_func>, data=0x7fff2b830890)
    at threads.c:714
#41 0x0000000000d38a82 in scm_boot_guile (argc=3, argv=0x7fff2b830a08, 
    main_func=0x5bad7a <main_with_guile>, closure=0x0) at init.c:350
#42 0x00000000005bad6c in main (argc=3, argv=0x7fff2b830a08, envp=0x7fff2b830a28) at main.cc:622


-- 
Miroslav Lichvar

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Ludovic Courtès]

Hi,

Miroslav Lichvar <mlichvar@redhat.com> writes:

> Here it is, let me know if you need anything else.
>
> (gdb) bt
> #0  0x00007f6b232b6f05 in raise () from /lib64/libc.so.6
> #1  0x00007f6b232b8a73 in abort () from /lib64/libc.so.6
> #2  0x00007f6b232afef9 in __assert_fail () from /lib64/libc.so.6
> #3  0x0000000000d52b92 in scm_fill_input (port=0x7f6b1d4b7420) at ports.c:978
> #4  0x0000000000638b66 in internal_ly_parse_scm (ps=0x7fff2b827bc0) at parse-scm.cc:43

This function reads this:

  SCM form = scm_read (port);


  /* Reset read_buf for scm_ftell.
     Shouldn't scm_read () do this for us?  */
  scm_fill_input (port);

The answer to the question here is "yes": `scm_fill_input ()' is really
an internal function (although its name suggests otherwise) and
shouldn't be called directly.  When calling `scm_read ()',
`scm_fill_input ()' is called behind the scenes when PORT's buffer needs
to be refilled.

Thus, I would suggest removing the call to `scm_fill_input ()'.  Can you
tell whether it fixes the problem?

Thanks in advance,
Ludo'.

Re: `scm_c_read ()' and `swap_buffer' trick harmful

[Miroslav Lichvar]

On Sat, Dec 20, 2008 at 06:10:31PM +0100, Ludovic Courtès wrote:
> > #4  0x0000000000638b66 in internal_ly_parse_scm (ps=0x7fff2b827bc0) at parse-scm.cc:43
> 
> This function reads this:
> 
>   SCM form = scm_read (port);
> 
> 
>   /* Reset read_buf for scm_ftell.
>      Shouldn't scm_read () do this for us?  */
>   scm_fill_input (port);
> 
> The answer to the question here is "yes": `scm_fill_input ()' is really
> an internal function (although its name suggests otherwise) and
> shouldn't be called directly.  When calling `scm_read ()',
> `scm_fill_input ()' is called behind the scenes when PORT's buffer needs
> to be refilled.
> 
> Thus, I would suggest removing the call to `scm_fill_input ()'.  Can you
> tell whether it fixes the problem?

Removing the line fixes the crash. Thanks for the help!

-- 
Miroslav Lichvar