Re: patch for mmap and friends

[Matt Wette]

On 1/14/23 2:42 PM, Maxime Devos wrote:

    {
  /* Use the fd of the port under clobber protection from
 concurrency. As scm_dynwind_acquire_port assumes that
 FILE is a port, check that first. */
  SCM_VALIDATE_PORT (SCM_ARG5, file);
  scm_dynwind_acquire_port (file);
  c_fd = scm_fileno (file);
    }


Thanks.  I'll try this, modulo update to  scm_to_int (scm_fileno (file)).

Matt

Re: patch for mmap and friends

[Maxime Devos]

On 14-01-2023 23:08, Matt Wette wrote:

2) had to copy/modify dynwind_acquire_port and release_port from ports.c


Is it because of the 'static' qualifier?  If so, you could use the 
'SCM_INTERNAL [...] scm_i_[...]' pattern, e.g. see 
'scm_i_is_mutable_bitvector' in libguile/bitvectors.h and 
libguile/bitvectors.c.




   if (SCM_UNBNDP (file))
c_fd = -1;
  else {
/* Use the fd under clobber protection from GC or another thread. */
if (SCM_PORTP (file))
  c_fd = scm_to_int (scm_fileno (file));
else {
  c_fd = scm_to_int (file);
  file = SCM_CAR (scm_fdes_to_ports (file));
}
scm_dynwind_acquire_port (file);
  }


You need to acquire the port _before_ taking its file descriptor! 
Otherwise, it is protected too late.  I.e., scm_dynwind_acquire_port 
needs to be moved before the 'scm_fileno'.


On the second branch, in particular 'c_fd = scm_to_int (file);':
IIUC, the idea is to, when a raw fd is passed, look up the corresponding 
port to lock it, right?


If so, I think it's too late for that -- another thread might change 
things between 'c_fd = ...' and 'file = SCM_CAR (scm_fdes_to_ports (file))'.


More generally, when a raw fd is passed, I think it's impossible to 
solve the 'other thread/GC interfering' problem.


As such, to simplify things, I propose to only do the 
'scm_dynwind_acquire_port' when a port is passed, instead of failing to 
solve the interference problems (if the user passed a raw fd, then only 
they can make sure there are no problems, e.g. by changing their code to 
use ports or by not using move->fdes stuff).:


Proposed code (untested):

{
  void *c_mem, *c_addr;
  size_t c_len;
  int c_prot, c_flags, c_fd;
  scm_t_off c_offset;
  SCM pointer, bvec;

  if (SCM_POINTER_P (addr))
c_addr = SCM_POINTER_VALUE (addr);
  else if (scm_is_integer (addr))
c_addr = (void*) scm_to_uintptr_t (addr);
  else
scm_misc_error ("mmap", "bad addr", addr);

  c_len = scm_to_size_t (len);

  if (SCM_UNBNDP (prot))
c_prot = PROT_READ | PROT_WRITE;
  else
c_prot = scm_to_int (prot);

  if (SCM_UNBNDP (flags))
c_flags = MAP_ANONYMOUS | MAP_PRIVATE;
  else
c_flags = scm_to_int (flags);

  scm_dynwind_begin (0);
  if (SCM_UNBNDP (file))
c_fd = -1;
  else if (scm_is_integer (file))
c_fd = scm_to_int (file);
  else
{
  /* Use the fd of the port under clobber protection from
 concurrency. As scm_dynwind_acquire_port assumes that
 FILE is a port, check that first. */
  SCM_VALIDATE_PORT (SCM_ARG5, file);
  scm_dynwind_acquire_port (file);
  c_fd = scm_fileno (file);
}

  if (SCM_UNBNDP (offset))
c_offset = 0;
  else
c_offset = scm_to_off_t (offset);

  if ((c_addr == NULL) && (c_flags & MAP_FIXED))
scm_misc_error ("mmap", "cannot have NULL addr w/ MAP_FIXED", SCM_EOL);

  SCM_SYSCALL (c_mem = mmap(c_addr, c_len, c_prot, c_flags, c_fd, 
c_offset));

  if (c_mem == MAP_FAILED)
scm_syserror ("mmap");  /* errno set */

  /* The fd is free to go now. */
  scm_dynwind_end ();

  pointer = scm_cell (scm_tc7_pointer, (scm_t_bits) c_mem);
  bvec = scm_c_take_typed_bytevector((signed char *) c_mem + c_offset, 
c_len,

 SCM_ARRAY_ELEMENT_TYPE_VU8, pointer);
  scm_i_set_finalizer (SCM2PTR (bvec), mmap_finalizer, (void*) c_len);
  return bvec;
}
#undef FUNC_NAME




OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: patch for mmap and friends

[Matt Wette]

On 1/14/23 8:31 AM, Matt Wette wrote:

On 1/14/23 7:18 AM, Maxime Devos wrote:

\
Port objects should be accepted too, as previously asked on 
.
As implied by later comments, using a raw fd causes problems with 
'move->fdes'.  For the remaining response, I'll assume that the 
function accepts ports as well.





To avoid this problem, you can add

  scm_remember_upto_here_1 (fd);

after the SCM_SYSCALL.
\



IIRC there is a C trick involving fields, arrays and types to check 
this at compile-time instead.  Maybe:


struct whatever {
   /* if availability of zero-length arrays can be assumed */
   int foo[sizeof(size_t) - sizeof(void*)];
   /* alternatively, a weaker but portable check */
   int foo[sizeof(size_t) - sizeof(void*) + 1];
};

Greetings,
Maxime.


Thanks for the feedback.   I'm sorry I missed you comments on the 
previous round.

I did respond to the ones I did catch.    I'll work this and resubmit.

Matt




Here is another shot.
1) added port support
2) used dynwind to protect port/fd.
3) removed the assert: I don't know why I put it in there.

Notes:
1) four other guile headers needed: atomic-internal, foreign, finalizers 
and ioext

2) had to copy/modify dynwind_acquire_port and release_port from ports.c
3) one maybe-kludge is if an fd is passed I use (car (fd->ports fd)) for 
acquire_port


Matt

static void
mmap_finalizer (void *ptr, void *data)
{
  SCM bvec;
  void *c_addr;
  size_t c_len;
  int rv;

  bvec = SCM_PACK_POINTER (ptr);
  if (!SCM_BYTEVECTOR_P (bvec))
    scm_misc_error ("mmap", "expecting bytevector", SCM_EOL);

  c_addr = SCM_BYTEVECTOR_CONTENTS (bvec);
  c_len = SCM_BYTEVECTOR_LENGTH (bvec);
  SCM_SYSCALL (rv = munmap(c_addr, c_len));
  if (rv != 0)
    scm_misc_error ("mmap", "failed to munmap memory", SCM_EOL);
}

/* Code for scm_dynwind_acquire_port and release_port sourced from 
ports.c. */


static void
release_port (SCM port)
{
  scm_t_port *pt = SCM_PORT (port);
  uint32_t cur = 1, next = 0;
  while (!scm_atomic_compare_and_swap_uint32 (>refcount, , next))
    {
  if (cur == 0)
    return;
  next = cur - 1;
    }
 if (cur > 1)
    return;

  if (SCM_PORT_TYPE (port)->close)
    SCM_PORT_TYPE (port)->close (port);

  /* Skip encoding code from ports.c! */
}

static void
scm_dynwind_acquire_port (SCM port)
{
  scm_t_port *pt = SCM_PORT (port);
  uint32_t cur = 1, next = 2;
  while (!scm_atomic_compare_and_swap_uint32 (>refcount, , next))
    {
  if (cur == 0)
    scm_wrong_type_arg_msg (NULL, 0, port, "open port");
  next = cur + 1;
    }
  scm_dynwind_unwind_handler_with_scm (release_port, port,
   SCM_F_WIND_EXPLICITLY);
}

SCM_DEFINE (scm_mmap_search, "mmap/search", 2, 4, 0,
    (SCM addr, SCM len, SCM prot, SCM flags, SCM file, SCM offset),
    "Create a memory mapping, returning a bytevector.. @var{addr},\n"
    "if non-zero, is the staring address; or, if zero, is assigned 
by\n"

    "the system.  @var{prot}, if provided, assigns protection.\n"
    "@var{file}, a port or fd, if provided associates the memory\n"
    "region with a file starting at @var{offset}, if provided.\n"
    "The region returned by mmap WILL be searched by the garbage\n"
    "collector for pointers.  See also mmap.  Note that the\n"
    "finalizer for the returned bytevector will call munmap.\n"
    "Defaults for optional arguments are\n"
    "@table @asis\n"
    "@item prot\n(logior PROT_READ PROT_WRITE)\n"
    "@item flags\n(logior MAP_ANONYMOUS MAP_PRIVATE)\n"
    "@item fd\n-1\n"
    "@item offset\n0\n"
    "@end table")
#define FUNC_NAME s_scm_mmap_search
{
  void *c_mem, *c_addr;
  size_t c_len;
  int c_prot, c_flags, c_fd;
  scm_t_off c_offset;
  SCM pointer, bvec;

  if (SCM_POINTER_P (addr))
    c_addr = SCM_POINTER_VALUE (addr);
  else if (scm_is_integer (addr))
    c_addr = (void*) scm_to_uintptr_t (addr);
  else
    scm_misc_error ("mmap", "bad addr", addr);

  c_len = scm_to_size_t (len);

  if (SCM_UNBNDP (prot))
    c_prot = PROT_READ | PROT_WRITE;
  else
    c_prot = scm_to_int (prot);

  if (SCM_UNBNDP (flags))
    c_flags = MAP_ANONYMOUS | MAP_PRIVATE;
  else
    c_flags = scm_to_int (flags);

  scm_dynwind_begin (0);

  if (SCM_UNBNDP (file))
    c_fd = -1;
  else {
    /* Use the fd under clobber protection from GC or another thread. */
    if (SCM_PORTP (file))
  c_fd = scm_to_int (scm_fileno (file));
    else {
  c_fd = scm_to_int (file);
  file = SCM_CAR (scm_fdes_to_ports (file));
    }
    scm_dynwind_acquire_port (file);
  }

  if (SCM_UNBNDP (offset))
    c_offset = 0;
  else
    c_offset = scm_to_off_t (offset);

  if ((c_addr == NULL) && (c_flags & MAP_FIXED))
    scm_misc_error ("mmap", "cannot have NULL addr w/ MAP_FIXED", SCM_EOL);

  SCM_SYSCALL (c_mem = mmap(c_addr, c_len, c_prot, c_flags, c_fd, 
c_offset));

  if (c_mem == MAP_FAILED)
    

Re: patch for mmap and friends

[tomas]

On Sat, Jan 14, 2023 at 04:18:58PM +0100, Maxime Devos wrote:

[...]

> (While it is recommended for Scheme code to keep a reference to the port to
> manually close afterwards, to free resources faster than waiting for GC, it
> is not actually required.)

Oh, oh. I've got a little anecdote to share here. The context was a Java
application, running on Sun medium-sized iron. It was slow & clumsy and
the customer decided to double the machine's RAM (these were times where
200 MB were quite a thing).

The application crashed a couple of times a day. Some log file poking
later it became clear: at the other side there was an Oracle database
and the app was exhausting the (limited) max number of connections
allowed for the license they had. More RAM -> less GC and the app was
relying on the object destructors to dispose of the unneeded database
connections.

There I learnt one shouldn't ever use memory as proxy for other
resources :-)

(The end of the story was that we could convince the user to have an
application under a free license and a free database :)

Cheers
-- 
t



signature.asc
Description: PGP signature

Re: patch for mmap and friends

[Matt Wette]

On 1/14/23 7:18 AM, Maxime Devos wrote:

\
Port objects should be accepted too, as previously asked on 
.
As implied by later comments, using a raw fd causes problems with 
'move->fdes'.  For the remaining response, I'll assume that the 
function accepts ports as well.





To avoid this problem, you can add

  scm_remember_upto_here_1 (fd);

after the SCM_SYSCALL.
\



IIRC there is a C trick involving fields, arrays and types to check 
this at compile-time instead.  Maybe:


struct whatever {
   /* if availability of zero-length arrays can be assumed */
   int foo[sizeof(size_t) - sizeof(void*)];
   /* alternatively, a weaker but portable check */
   int foo[sizeof(size_t) - sizeof(void*) + 1];
};

Greetings,
Maxime.


Thanks for the feedback.   I'm sorry I missed you comments on the 
previous round.

I did respond to the ones I did catch.    I'll work this and resubmit.

Matt

Re: [PATCH] Add 'bytevector-slice'.

[Ludovic Courtès]

Hello!

I pushed the patch as commit e441c34f1666921f6b15597c1aa3a50596a129d7
with the following changes taking into account your comments:

  • adjusted copyright years;

  • removed comment about ‘SCM_F_BYTEVECTOR_CONTIGUOUS’ since it’s quite
clear from the discussion in this thread that this flag is
vestigial;

  • added an overflow check for ‘c_offset + c_size’ and a corresponding
test (really glad you reported this one!);

  • added another missing test that you mentioned.

I did not update the license header as you suggested, but I think we
should run a script on all the repo to homogenize those but it’s a bit
messy right now.

Thanks,
Ludo’.

Re: patch for mmap and friends

[Maxime Devos]

On 14-01-2023 01:49, Matt Wette wrote:

Please consider this patch for adding mmap(), munmap() and msync()
  to libguile/filesys.c.  Included is update for posix.texi and test 
file mman.test.

Once included, feature 'mman should be #t.

Matt






+  if (SCM_UNBNDP (fd))
+c_fd = -1;
+  else
+c_fd = scm_to_int (fd);


Port objects should be accepted too, as previously asked on 
.
As implied by later comments, using a raw fd causes problems with 
'move->fdes'.  For the remaining response, I'll assume that the function 
accepts ports as well.


 (---)

After this code, the port 'fd' becomes unreferenced by this function.


+  if (SCM_UNBNDP (offset))
+c_offset = 0;
+  else
+c_offset = scm_to_off_t (offset);
+
+  if ((c_addr == NULL) && (c_flags & MAP_FIXED))
+scm_misc_error ("mmap", "cannot have NULL addr w/ MAP_FIXED", SCM_EOL);


Hence, if the GC is run here, its possible for fd to be automatically 
closed here.



+  SCM_SYSCALL (c_mem = mmap(c_addr, c_len, c_prot, c_flags, c_fd, c_offset));


As such, C 'mmap' could be called on a closed file descriptor even even 
if the argument to Scheme 'mmap' was an open port, which isn't going to 
work.


(While it is recommended for Scheme code to keep a reference to the port 
to manually close afterwards, to free resources faster than waiting for 
GC, it is not actually required.)


Worse, if the port is closed (by GC), in the mean time another thread 
may have opened a new port, whose file descriptor coincides with c_fd.


To avoid this problem, you can add

  scm_remember_upto_here_1 (fd);

after the SCM_SYSCALL.

Even then, a problem remains -- a concurrent thread might be using 
'move->fdes' to 'move' the file descriptor, hence invalidating c_fd.
Functions like 'scm_seek' handle this with 'scm_dynwind_acquire_port' 
(IIUC, it takes a mutex to delay concurrent 'move->fdes' until finished).


IIUC, the solution then looks like (ignoring wrapping) (the lack of 
scm_remember_upto_here_1 is intentional):


scm_dynwind_begin (0);
scm_dynwind_acquire_port (fd); // (accepts both ports and numbers, IIUC)
// needs to be after scm_dynwind_acquire_port
if (SCM_UNBNDP (fd))
  c_fd = -1;
else
  c_fd = scm_to_int (fd);

SCM_SYSCALL (c_mem = mmap(c_addr, c_len, c_prot, c_flags, c_fd, c_offset));
if (c_mem == MAP_FAILED)
scm_syserror ("mmap");
scm_dynwind_end ();

(I'm not really familiar with scm_dynwind_begin & friends, I'm mostly 
copy-pasting from libguile/ports.c here.)



+  if (c_mem == MAP_FAILED)
+scm_syserror ("mmap");  /* errno set */
+
+  pointer = scm_cell (scm_tc7_pointer, (scm_t_bits) c_mem);
+  bvec = scm_c_take_typed_bytevector((signed char *) c_mem + c_offset, c_len,
+SCM_ARRAY_ELEMENT_TYPE_VU8, pointer);



+  assert(sizeof(void*) <= sizeof(size_t));


IIRC there is a C trick involving fields, arrays and types to check this 
at compile-time instead.  Maybe:


struct whatever {
   /* if availability of zero-length arrays can be assumed */
   int foo[sizeof(size_t) - sizeof(void*)];
   /* alternatively, a weaker but portable check */
   int foo[sizeof(size_t) - sizeof(void*) + 1];
};

Greetings,
Maxime.


OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCHv2] Extensions for SRFI-171 (Transducers)

[Ludovic Courtès]

Hi Colin and all!

These patches look like a nice addition.

First, you have the option of assigning your copyright for this
contribution (and future Guile contributions if you wish) to the FSF, or
you can choose not to:

  https://lists.gnu.org/archive/html/guile-devel/2022-10/msg8.html

Please take a look at the message above and let us know what you’d like
to do.  If you choose not to assign copyright, you’ll have to add
copyright lines for you (or whatever entity holds copyright on your
work) in the modified files.

Overall the changes LGTM; I have minor comments and suggestions:

Colin Woodbury  skribis:

> From 96856b184a507886db2c5c20323983ae125a6bdb Mon Sep 17 00:00:00 2001
> From: Colin Woodbury 
> Date: Mon, 19 Dec 2022 09:39:37 +0900
> Subject: [PATCH 1/4] srfi-171: add twindow and various reducers
>
> This adds a number of reduction primitives often seen in other languages
> to Guile's SRFI-171 extensions.
>
> Most critical may be `rfold`, which could be called the fundamental
> reducer, as it's likely that all other reducers could be defined in
> terms of it (though not all are). While `tfold` already exists in
> Guile's SRFI-171 extension as a transducer, folding is in essence a
> reduction. Also without a primative like `rlast` (also introduced here),
> the results of `tfold` are difficult to consume. This is avoided by
> providing `rfold` directly as a generalised means to collapse an entire
> transduction down into a single value (i.e. the whole point of
> reducers). `rfold` is also useful for the creation of ad-hoc reducers,
> as any 2-arg function can be passed to it to fold the stream of values.
>
> `rfirst`, `rlast`, and `rfind` are common idioms and so have been added.
>
> The equivalent of `rmax` and `rmin` are easy to write manually via
> `rfold`, but they have been provided here as a convenience in the same
> spirit as `rcons`.
>
> `rfor-each` also cannot be forgotten as a classic adaptation of its
> SRFI-1 cousin.
>
> Also added is `twindow`, handy for analysing groups of adjacent items.

[...]

> From 58e7ca2718a860ca2fb5692684d6d128a7c1ae75 Mon Sep 17 00:00:00 2001
> From: Colin Woodbury 
> Date: Tue, 20 Dec 2022 09:41:51 +0900
> Subject: [PATCH 2/4] doc: add new SRFI-171 reducers to the manual
>
> ---
>  doc/ref/srfi-modules.texi | 96 +--

[...]

> From 7b7538c61799fa0fa0e2fa18efba98b7de7da1ca Mon Sep 17 00:00:00 2001
> From: Colin Woodbury 
> Date: Wed, 21 Dec 2022 09:30:50 +0900
> Subject: [PATCH 3/4] srfi-171: add unit tests for new functions
>
> These tests mainly match the examples shown in the docs.
> ---
>  test-suite/tests/srfi-171.test | 66 ++

We’d squash these three commits together to provide a single
self-contained commit with code and the corresponding tests and doc.

The convention in Guile is for commit logs to follow the ChangeLog style
(see ‘git log’ for examples).  If you’re not sure how to do that, I can
do it on your behalf as a welcome present.  ;-)

> From 87a74d106f11680c4924befb664d7ef685c16b06 Mon Sep 17 00:00:00 2001
> From: Colin Woodbury 
> Date: Thu, 22 Dec 2022 20:32:33 +0900
> Subject: [PATCH 4/4] doc: added a guide for writing custom reducers
>
> The guide previously explained what reducers were, but not the specifics
> of how to write them yourself. This commits rectifies this.

Nice!

> +++ b/doc/ref/srfi-modules.texi
> @@ -5966,6 +5966,82 @@ Yield the maximum (or minimum) value of the 
> transduction, or the
>  @var{seed} value if there is none.
>  @end deffn
>  
> +@subheading Writing your own reducers
> +If you want to reduce some values via an ordinary function that you

Please capitalize section titles and leave a blank line below it (same
for the section that follows).

> +However, if you want more customized behaviour (such as early
> +termination and/or arbitrary manipulation of the input values) then
> +you're free to write a reducer manually. To do so, we need to write a

Normally we leave two spaces after end-of-sentence periods, to ease
navigation in Emacs and please Texinfo (info "(texinfo) Ending a
Sentence").

Could you send updated patches?

Thanks for your work!

Ludo’.

Re: [PATCH] Document R7RS bytevector functions

[Ludovic Courtès]

Hi Daniel,

lloda  skribis:

> Right now the manual just mentions (scheme base), but not the contents. This 
> patch at least makes sure that at least the bytevector-related R7RS functions 
> appear in the index.
>
> The patch documents a first group of purely bytevector functions and a second 
> group of binary I/O that are not elsewhere in Guile/R6RS or that exist but 
> have different definitions.

Nice.

> From 2b751615071aca023dac59db866fb09894fa2b57 Mon Sep 17 00:00:00 2001
> From: Daniel Llorens 
> Date: Fri, 13 Jan 2023 16:26:17 +0100
> Subject: [PATCH] Document R7RS functions related to bytevectors
>
> * doc/ref/api-data.texi: Document: bytevector bytevector-copy
>   bytevector-copy! bytevector-append
>   (r6:bytevector-copy): Index need not be positive (can be 0).
> * doc/ref/api-io.texi: Document: open-output-bytevector write-u8 read-u8 
> peek-u8
>   get-output-bytevector open-input-bytevector read-bytevector! read-bytevector
>   write-bytevector

Please specify the node names (see ‘git log’ for examples).

>  doc/ref/api-data.texi| 102 ++---
>  doc/ref/api-io.texi  | 121 +++
>  lib/iconv_open-aix.h |  68 +++---
>  lib/iconv_open-hpux.h|  92 ++---
>  lib/iconv_open-irix.h|  42 +++---
>  lib/iconv_open-osf.h |  80 +-
>  lib/iconv_open-solaris.h |  30 +-

I guess the lib/ changes are unintended.  :-)

> +++ b/doc/ref/api-data.texi
> @@ -6188,9 +6188,9 @@ thus created is determined implicitly by the number of 
> arguments given.
>  Return a newly allocated vector composed of the
>  given arguments.  Analogous to @code{list}.
>  
> -@lisp
> +@example
>  (vector 'a 'b 'c) @result{} #(a b c)
> -@end lisp
> +@end example

Please keep @lisp as this gives information that can then be exploited
for syntax highlighting (‘texinfo --html’ doesn’t take advantage of it
so far, but in Guix we have machinery to do that.)

> +@ref{R7RS Support,R7RS} defines another set of bytevector functions in
> +the module @code{(scheme base)}. These functions are also described in this
> +section.

Please leave two spaces after and end-of-sentence period, to ease
navigation in Emacs and to please Texinfo (info "(texinfo) Ending a
Sentence").

Maybe these two sentences should go to the intro of the new subsection
(see below).

> @@ -6726,9 +6730,10 @@ procedures and C functions.
>  @deffnx {C Function} scm_c_make_bytevector (size_t len)
>  Return a new bytevector of @var{len} bytes.  Optionally, if @var{fill}
>  is given, fill it with @var{fill}; @var{fill} must be in the range
> -[-128,255].
> +[-128,255].@footnote{This function is defined both by R6RS in @code{(rnrs 
> bytevectors)} and by @ref{R7RS Standard Libraries,R7RS} in @code{(scheme 
> base)}.}.
>  @end deffn
>  
> +@anchor{x-bytevector?}
>  @deffn {Scheme Procedure} bytevector? obj
>  @deffnx {C Function} scm_bytevector_p (obj)
>  Return true if @var{obj} is a bytevector.
> @@ -6757,26 +6762,32 @@ length and contents.
>  @deffnx {C Function} scm_bytevector_fill_x (bv, fill)
>  Fill positions [@var{start} ... @var{end}) of bytevector @var{bv} with
>  byte @var{fill}. @var{start} defaults to 0 and @var{end} defaults to the
> -length of @var{bv}.@footnote{R6RS defines @code{(bytevector-fill! bv
> +length of @var{bv}.@footnote{R6RS only defines @code{(bytevector-fill! bv
>  fill)}. Arguments @var{start} and @var{end} are a Guile extension
>  (cf. @ref{x-vector-fill!,@code{vector-fill!}},
>  @ref{x-string-fill!,@code{string-fill!}}).}
>  @end deffn
>  
> +@anchor{x-r6:bytevector-copy!}
>  @deffn {Scheme Procedure} bytevector-copy! source source-start target 
> target-start len
>  @deffnx {C Function} scm_bytevector_copy_x (source, source_start, target, 
> target_start, len)
>  Copy @var{len} bytes from @var{source} into @var{target}, starting
> -reading from @var{source-start} (a positive index within @var{source})
> +reading from @var{source-start} (an index index within @var{source})
>  and writing at @var{target-start}.
>  
>  It is permitted for the @var{source} and @var{target} regions to
>  overlap. In that case, copying takes place as if the source is first
>  copied into a temporary bytevector and then into the destination.
> +
> +See also @ref{x-r7:bytevector-copy!,the R7RS version}.
>  @end deffn
>  
> +@anchor{x-r6:bytevector-copy}
>  @deffn {Scheme Procedure} bytevector-copy bv
>  @deffnx {C Function} scm_bytevector_copy (bv)
>  Return a newly allocated copy of @var{bv}.
> +
> +See also @ref{x-r7:bytevector-copy,the R7RS version}.
>  @end deffn

We should keep the manual in sync with docstrings in bytevectors.c.

Thus, my suggestion would be to not insert comments and footnotes about
R7RS in the existing sections, but instead to do that in the new section.

> +@subsubheading Bytevector functions in R7RS

Please capitalize and use “procedure”:

  Bytevector Procedures in R7RS.

Here maybe you 

Re: [PATCH] Add 'bytevector-slice'.

[Maxime Devos]

On 14-01-2023 00:48, Ludovic Courtès wrote:

Ah yes, that’s right, I misunderstood the comment.

In the example above, where we’re only dealing with slices, we could
“skip” the parent (i.e., have each slice’s parent point to the “root” of
the hierarchy), but I don’t think we can assume this to be the case
generally.

Ludo’.


Why not?  I.e., where would things go wrong it the parent is "skipped" 
in other cases?  Something about GC I guess, but I don't follow what 
this "something" would be.


My guess is that you are thinking of the interaction with weak key-value 
maps, e.g. a map


   root-bytevector-> stuff
   [other entries]

where the user might expect a slice of the root bytevector to keep the 
root itself intact -- which would be the case with your patch, but not 
with my "skip the parent" proposal.


I suppose there might be use cases for such things, in which case I 
wouldn't mind the original behaviour (though I'll have to look over 
Scheme-GNUnet code to verify no long chains are constructed), but if 
this is supposed to be supported, it should be documented. E.g.:


‘The returned slice keeps a reference to @var{bv} and not only to the 
underlying bytes of the bytevector.  Usually, this is of no importance, 
but this information is relevant when using GC data structures such as 
guardians and weak hash tables.’


Greetings,
Maxime.


OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature