bug#72215: Compiler error building 3.0.10 on 32-bit platforms

[Ludovic Courtès]

Hi,

The following compilation error occurs on i686-linux:

--8<---cut here---start->8---
  BOOTSTRAP(stage1) GUILEC ice-9/control.go
wrote `language/cps/verify.go'
  BOOTSTRAP(stage1) GUILEC ice-9/copy-tree.go
wrote `ice-9/copy-tree.go'
  BOOTSTRAP(stage1) GUILEC ice-9/curried-definitions.go
Backtrace:
In ice-9/boot-9.scm:
  1755:12 19 (with-exception-handler # # #:unwind? _ #:unwind-for-type _)
In system/base/compile.scm:
69:11 18 (_)
   190:11 17 (_ #)
309:6 16 (read-and-compile # #:from _ #:to _ #:env _ 
#:optimization-level _ #:warning-level _ #:opts _)
   352:28 15 (compile # ?)
   265:44 14 (_ # ?)
   261:33 13 (_ # #)
In language/cps/optimize.scm:
136:2 12 (_ _ #)
111:3 11 (optimize-first-order-cps _ _)
In language/cps/switch.scm:
414:6 10 (optimize-branch-chains _)
In language/cps/intmap.scm:
519:6  9 (visit-branch #(#(#(# (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) #f) (absent) (absent) (absent) #((absent) 
(absent) # (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) # ?) ?) ?) ?)
519:6  8 (visit-branch #(#(# (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) #f) (absent) (absent) (absent) #((absent) 
(absent) # (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (#) # ?) ?) ?)
519:6  7 (visit-branch #((absent) (absent) # (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) (absent) (absent) 
(absent) (absent) (absent) (absent) (absent) (absent) # (absent) (absent) (absent) (absent) (absent) #f) _ 128 _)
In language/cps/switch.scm:
99:36  6 (fold-branch-chains # _ _ # _)
343:8  5 (optimize-branch-chain 100 151 (132 134 136 138 140) _)
In ice-9/boot-9.scm:
   260:13  4 (for-each1 ((call-with-escape-continuation . 135) (call/ec . 133) 
(reset* . 139) (shift* . 137)))
In language/cps/switch.scm:
   340:32  3 (_ (call-with-escape-continuation . 135))
In language/cps/guile-vm.scm:
89:31  2 (target-symbol-hash _)
41:18  1 (jenkins-lookup3-hashword2 "call-with-escape-continuation")
In ice-9/boot-9.scm:
  1676:22  0 (lp 0)

ice-9/boot-9.scm:1676:22: In procedure lp:
Value out of range 0 to< 18446744073709551615: -512332661
make[2]: *** [Makefile:2517: ice-9/control.go] Error 1
make[2]: *** Waiting for unfinished jobs
--8<---cut here---end--->8---

According to , this
regression was introduced by
.

(This affects 3.0.10.)

Ludo’.

bug#70144: system* affects signal handlers

[Ludovic Courtès]

Hi,

Josselin Poiret  skribis:

> Yes, I believe this is all taken care of by our use of posix_spawn
> (which was the point in the first place :) ).

Yup!  I pushed the fix as 4ae33f76d6b33ea0bedfa36050d44c88d08c2823.

Thanks,
Ludo’.

bug#67063: unread-string does not match documented functionality

[Ludovic Courtès]

Hi Juliana,

Juliana Sims  skribis:

> This patch simply fixes bug 67063.
>
> Thanks,
> Juli
>
> * doc/ref/api-io.texi (Venerable Port Interfaces): Bring unread-string
> procedure documentation in line with other procedures in the section.
> * libguile/ports.c (scm_unread_string): Make port argument optional.
> * test-suite/tests/ports.test: Test unread-char and unread-string
> without ports.

Finally applied with these changes:

diff --git a/NEWS b/NEWS
index 8a61bf65d..3c4854ca9 100644
--- a/NEWS
+++ b/NEWS
@@ -59,6 +59,8 @@ files.  See "Random Access" in the manual for details.
()
 ** 'read-u8' in (scheme base) now defaults to (current-input-port)
()
+** Second argument of 'unread-string' is now optional, as previously documented
+   ()
 ** 'ftw' now correctly deals with directory permissions
()
 ** 'make-custom-port' now honors its #:conversion-strategy argument
diff --git a/doc/ref/api-io.texi b/doc/ref/api-io.texi
index 3a0f7a9a1..79bc9e9d6 100644
--- a/doc/ref/api-io.texi
+++ b/doc/ref/api-io.texi
@@ -2001,6 +2001,7 @@ current input port, and the arguments are swapped.  @xref{Textual I/O}.
 @end deffn
 
 @deffn {Scheme Procedure} unread-string str [port]
+@deffnx {C Function} scm_unread_string (str, port)
 The same as @code{unget-string}, except that @var{port} defaults to the
 current input port, and the arguments are swapped.  @xref{Textual I/O}.
 @end deffn

Not sure why the C functions aren’t documented for the other ones.

Thanks,
Ludo’.

bug#70142: [PATCH] Fix error messages containing format strings

[Ludovic Courtès]

Hi Michael,

Michael Käppler  skribis:

> The reason is that format strings occurring in the message are
> escaped, see `module/ice-9/boot-9.scm`  and
> `module/language/tree-il/primitives.scm`.
>
> So a call of
>
> `(error "Wrong argument: ~a" 42)`
>
> is rendered as
>
> "Wrong argument: ~a 42"
> Some callers did not take this behavior into account.

Good catch, applied.  Thanks!

Ludo’.

bug#69921: [PATCH] eval-string: Fix setting port column

[Ludovic Courtès]

Jonas Hahnfeld  skribis:

> On Thu, 2024-03-14 at 17:11 +0600, Nikita Domnitskii wrote:
>> ---
>>  module/ice-9/eval-string.scm | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/module/ice-9/eval-string.scm b/module/ice-9/eval-string.scm
>> index ea0f1..9cac03632 100644
>> --- a/module/ice-9/eval-string.scm
>> +++ b/module/ice-9/eval-string.scm
>> @@ -81,7 +81,7 @@
>>(if line
>>(set-port-line! port line))
>>(if column
>> -  (set-port-column! port line))
>> +  (set-port-column! port column))
>>  
>>(if (or compile? (not (language-evaluator lang)))
>>((load-thunk-from-memory
>> 
>> 
>
> LGTM (wrong since 2011, it seems), but needs somebody with commit
> access to push.

This was pushed as 025bb024ae4b586dc721e3d2e3471fbea5f8cc81.

Closing!

Ludo’.

bug#69315: [PATCH] Build system fixes

[Ludovic Courtès]

Hi Jonas,

Jonas Hahnfeld  skribis:

> From 428d1b17c5f664d3cb8da4cd5687bd47bdd87877 Mon Sep 17 00:00:00 2001
> From: Jonas Hahnfeld 
> Date: Thu, 22 Feb 2024 21:57:41 +0100
> Subject: [PATCH 1/2] build: Make sed invocation fully portable
>
> Commit 08041d216f attempted to make the "invocation compatible with
> BSD sed", but moving '-i' first does not solve the problem because
> it still requires to pass an argument. Instead just redirect the
> instantiated output into a temporary file and install that.
>
> * libguile/Makefile.am: Remove '-i' from INSTANTIATE.

[...]

> From ad01912391b449fcf547ac52ed468f9b572cb0ad Mon Sep 17 00:00:00 2001
> From: Jonas Hahnfeld 
> Date: Thu, 22 Feb 2024 22:10:06 +0100
> Subject: [PATCH 2/2] build: Fix cross-compilation in out-of-tree-builds
>
> gen-scmconfig.h is generated in libguile, not $(top_builddir).
>
> * libguile/Makefile.am: Add '-I.' when compiling gen-scmconfig.o.

Finally applied both, thanks!

Ludo’.

bug#69314: [PATCH] Speed up stage0 bootstrap build using prebuilts

[Ludovic Courtès]

Hi Jonas,

Jonas Hahnfeld  skribis:

> On Thu, 2024-01-04 at 11:57 +0100, Jonas Hahnfeld wrote:
>> From 95f15821c535537c7ad4fdae1988855314d56ece Mon Sep 17 00:00:00 2001
>> From: Jonas Hahnfeld 
>> Date: Thu, 4 Jan 2024 11:44:55 +0100
>> Subject: [PATCH] Speed up stage0 bootstrap build using prebuilts
>> 
>> Use prebuilt bytecode of ice-9/eval.go and others for all of stage0,
>> it is optimized and evaluation is much faster. In my environment,
>> this speeds up the build of guile-3.0.9 from around 29 minutes to
>> only 19 minutes.
>> 
>> * meta/build-env.in: In stage0, prefer prebuilt bytecode over just
>> compiled stage0 files.
>> ---
>>  meta/build-env.in | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/meta/build-env.in b/meta/build-env.in
>> index bdc88ded4..446a536af 100644
>> --- a/meta/build-env.in
>> +++ b/meta/build-env.in
>> @@ -58,7 +58,7 @@ then
>>  fi
>>  export GUILE_LOAD_PATH
>>  case "$GUILE_BOOTSTRAP_STAGE" in
>> -stage0) 
>> GUILE_LOAD_COMPILED_PATH="${top_builddir}/stage0:${top_srcdir}/prebuilt/@SCM_PREBUILT_BINARIES@"
>>  ;;
>> +stage0) 
>> GUILE_LOAD_COMPILED_PATH="${top_srcdir}/prebuilt/@SCM_PREBUILT_BINARIES@:${top_builddir}/stage0"
>>  ;;

I don’t understand why changing the order would make a difference.
Surely if .go files are available under prebuilt/, they’ll be found,
even if that directory comes second?  Or am I missing something?

Thanks for the patch!

Ludo’.

bug#68878: [PATCH] Fix typos throughout codebase.

[Ludovic Courtès]

Hi Morgan,

Applied, with one exception:

Morgan Smith  skribis:

> * lib/alloca.in.h:
> * lib/c-ctype.h:
> * lib/malloca.h:
> * lib/nstrftime.c:
> * lib/verify.h:
> * lib/xalloc-oversized.h:

Since these files come from Gnulib, not from Guile, I left them
unchanged.

Thanks!

Ludo’.

bug#69730: Segfault in (spawn) when passed wrong port

[Ludovic Courtès]

Hi,

Tomas Volf <~@wolfsden.cz> skribis:

> When I pass #f to spawn as a port, it just segfaults:
>
> $ guile -c '(spawn "true" (list "") #:error #f)'
> Segmentation fault

This was fixed a while back in the ‘main’ branch:

--8<---cut here---start->8---
$ guile -c '(spawn "true" (list "") #:error #f)'
Backtrace:
In ice-9/boot-9.scm:
  1755:12  6 (with-exception-handler _ _ #:unwind? _ # _)
In unknown file:
   5 (apply-smob/0 #)
In ice-9/boot-9.scm:
724:2  4 (call-with-prompt ("prompt") # …)
In ice-9/eval.scm:
619:8  3 (_ #(#(#)))
In ice-9/command-line.scm:
   185:19  2 (_ #)
In unknown file:
   1 (eval (spawn "true" (list "") #:error #f) #)
   0 (spawn "true" ("") #:error #f)

ERROR: In procedure spawn:
In procedure spawn: Wrong type argument in position 5 (expecting open file 
port): #f
$ guix describe
Generation 301  May 05 2024 23:32:10(current)
  shepherd 08dfcde
repository URL: https://git.savannah.gnu.org/git/shepherd.git
branch: devel
commit: 08dfcde725e3b26f1e542a520c97a16aa9482802
  guile 3b76a30
repository URL: https://git.savannah.gnu.org/git/guile.git
branch: main
commit: 3b76a30e3ca1f0b7ee7944836c2fc5660596b3bd
  guix d6c95d4
repository URL: https://git.savannah.gnu.org/git/guix.git
branch: master
commit: d6c95d4208a0cbc3ab742732865f9481c637660f
--8<---cut here---end--->8---

We ought to cut a release!

See  and
5b42f8c154906584455a4989038406c88b723cb0.

Thanks,
Ludo’.

bug#70144: system* affects signal handlers

[Ludovic Courtès]

Hi,

Mikael Djurfeldt  skribis:

> system* temporarily re-binds signal handlers to prevent the child process
> from killing the parent. Thus, it is not thread safe with regard to SIGINT
> (or SIGQUIT if available). So, your code has a race condition with respect
> to the signal handler. This common resource can, in principle, be handled
> the usual way by, for example, utilizing a mutex:

[...]

> I'm leaving this bug open. *Should* system* re-bind the signal handlers?
> Should it really protect itself from the child? If so, we should probably
> document this behaviour in the reference manual.

Unless I’m mistaken, we can remove the ‘scm_dynwind_sigaction’ calls
from ‘scm_system_star’: now that we use ‘posix_spawn’, this is all taken
care of.

This can be seen by running:

  strace -o /tmp/log.strace -f guile -c '(system* "/bin/sh" "-c" "echo foo $$")'

… which shows pre-fork signal blocking (this is
‘internal_signal_block_all’ in spawni.c in glibc) followed, in the child
(PID 28592 here), by a long series of ‘sigaction’ calls to reset
handlers to their default behavior:

--8<---cut here---start->8---
28586 rt_sigprocmask(SIG_BLOCK, ~[], [], 8) = 0
28586 clone3({flags=CLONE_VM|CLONE_VFORK, exit_signal=SIGCHLD, 
stack=0x7f73b39b2000, stack_size=0x9000}, 88 
28592 rt_sigprocmask(SIG_BLOCK, NULL, ~[KILL STOP], 8) = 0
28592 rt_sigaction(SIGHUP, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 
8) = 0
28592 rt_sigaction(SIGHUP, {sa_handler=SIG_DFL, sa_mask=[], 
sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
28592 rt_sigaction(SIGINT, NULL, {sa_handler=SIG_IGN, sa_mask=[], 
sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, 8) = 0
28592 rt_sigaction(SIGQUIT, NULL, {sa_handler=SIG_IGN, sa_mask=[], 
sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, 8) = 0
28592 rt_sigaction(SIGILL, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 
8) = 0
28592 rt_sigaction(SIGILL, {sa_handler=SIG_DFL, sa_mask=[], 
sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
28592 rt_sigaction(SIGTRAP, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 
8) = 0
28592 rt_sigaction(SIGTRAP, {sa_handler=SIG_DFL, sa_mask=[], 
sa_flags=SA_RESTORER, sa_restorer=0x7f73b432d2a0}, NULL, 8) = 0
--8<---cut here---end--->8---

Josselin, can you confirm?

Thanks,
Ludo’.

bug#68504: [PATCH v3] Add copy-on-write support to scm_copy_file.

[Ludovic Courtès]

Hi,

Tomas Volf <~@wolfsden.cz> skribis:

> Sure, I am willing to do my part.  I managed to find this blog post[0], so 
> after
> some minor troubles I did manage to get a VM with GNU/Hurd running.  Next I 
> will
> read up on copy_file_range and try to put together a patch.

It’s really just (service hurd-vm-service-type) on Guix System:

  
https://guix.gnu.org/manual/devel/en/html_node/Virtualization-Services.html#The-Hurd-in-a-Virtual-Machine

> Just to make sure, your idea here is exactly what?  Always try to use
> copy_file_range before the regular copy?  So the flow would be
>
> For 'always case:
>
> CoW  ---fail-->  FAIL
>
> For 'auto case:
>
> CoW  ---fail-->  copy_file_range  ---fail-->  current copy  ---fail-->  
> FAIL
>
> For 'never case:
>
> copy_file_range  ---fail-->  current copy  ---fail-->  FAIL
>
> Is that an accurate summary?

Yes, that’s exactly what I had in mind.

Actually it might be better to use sendfile(2), which is slightly less
generic but otherwise equivalent AFAICS, and which happens to have a
Hurd implementation in glibc.

Thanks for your help!

Ludo’.

bug#68504: [PATCH v3] Add copy-on-write support to scm_copy_file.

[Ludovic Courtès]

Hi Tomas,

Tomas Volf <~@wolfsden.cz> skribis:

> On modern file-systems (BTRFS, ZFS) it is possible to copy a file using
> copy-on-write method.  For large files it has the advantage of being
> much faster and saving disk space (since identical extents are not
> duplicated).  This feature is stable and for example coreutils' `cp'
> does use it automatically (see --reflink).
>
> This commit adds support for this feature into our copy-file procedure.
> Same as `cp', it defaults to 'auto, meaning the copy-on-write is
> attempted, and in case of failure the regular copy is performed.
>
> No tests are provided, because the behavior depends on the system,
> underlying file-system and its configuration.  That makes it challenging
> to write a test for it.  Manual testing was performed instead:
>
> $ btrfs filesystem du /tmp/cow*
>  Total   Exclusive  Set shared  Filename
>   36.00KiB36.00KiB   0.00B  /tmp/cow
>
> $ cat cow-test.scm
> (copy-file "/tmp/cow" "/tmp/cow-unspecified")
> (copy-file "/tmp/cow" "/tmp/cow-always" #:copy-on-write 'always)
> (copy-file "/tmp/cow" "/tmp/cow-auto" #:copy-on-write 'auto)
> (copy-file "/tmp/cow" "/tmp/cow-never" #:copy-on-write 'never)
> (copy-file "/tmp/cow" "/dev/shm/cow-unspecified")
> (copy-file "/tmp/cow" "/dev/shm/cow-auto" #:copy-on-write 'auto)
> (copy-file "/tmp/cow" "/dev/shm/cow-never" #:copy-on-write 'never)
> $ ./meta/guile -s cow-test.scm
>
> $ btrfs filesystem du /tmp/cow*
>  Total   Exclusive  Set shared  Filename
>   36.00KiB   0.00B36.00KiB  /tmp/cow
>   36.00KiB   0.00B36.00KiB  /tmp/cow-always
>   36.00KiB   0.00B36.00KiB  /tmp/cow-auto
>   36.00KiB36.00KiB   0.00B  /tmp/cow-never
>   36.00KiB   0.00B36.00KiB  /tmp/cow-unspecified
>
> $ sha1sum /tmp/cow* /dev/shm/cow*
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-always
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-auto
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-never
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-unspecified
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /dev/shm/cow-auto
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /dev/shm/cow-never
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /dev/shm/cow-unspecified
>
> This commit also adds to new failure modes for (copy-file).
>
> Failure to copy-on-write when 'always was passed in:
>
> scheme@(guile-user)> (copy-file "/tmp/cow" "/dev/shm/cow" #:copy-on-write 
> 'always)
> ice-9/boot-9.scm:1676:22: In procedure raise-exception:
> In procedure copy-file: copy-on-write failed: Invalid cross-device link
>
> Passing in invalid value for the #:copy-on-write keyword argument:
>
> scheme@(guile-user)> (copy-file "/tmp/cow" "/dev/shm/cow" #:copy-on-write 
> 'nevr)
> ice-9/boot-9.scm:1676:22: In procedure raise-exception:
> In procedure copy-file: invalid value for #:copy-on-write: nevr
>
> * NEWS: Add note for copy-file supporting copy-on-write.
> * configure.ac: Check for linux/fs.h.
> * doc/ref/posix.texi (File System)[copy-file]: Document the new
> signature.
> * libguile/filesys.c (clone_file): New function cloning a file using
> FICLONE, if supported.
> (k_copy_on_write): New keyword.
> (sym_always, sym_auto, sym_never): New symbols.
> (scm_copy_file2): Renamed from scm_copy_file.  New #:copy-on-write
> keyword argument.  Attempt copy-on-write copy by default.
> (scm_copy_file): Call scm_copy_file2.
> * libguile/filesys.h: Add scm_copy_file2 as SCM_INTERNAL.
> ---
> v2: Introduce scm_copy_file2 in order to preserve backwards compatibility.
>
> v3: Remove mention of scm_copy_file from the commit message.

Finally pushed as e1690f3fd251d69b3687ec12c6f4b41034047f0f.  Note that I
added copyright lines for you, let me know if I got it wrong.

As a followup, we should add support for ‘copy_file_range’ when FICLONE
cannot be used; glibc supports it on all platforms but it returns ENOSYS
on GNU/Hurd currently.

WDYT?

Thank you!

Ludo’.

bug#66531: [PATCH] ftw: Fix getuid-or-false, getgid-or-false macros.

[Ludovic Courtès]

Tomas Volf  skribis:

> Both macros were missing a quote for the procedure call, causing the
> actual return value to be compiled into the ftw.go, instead of the
> procedure call.  Snippet from disassembly of ftw.go does confirm that:
>
>   55(make-immediate 2 3990) ;; 997at 
> ice-9/ftw.scm:319:46
>   56(make-long-immediate 1 120002)  ;; 3  at 
> ice-9/ftw.scm:320:46
>
> That effectively prevented ftw from entering directories without access
> for others.  Simple reproduction:
>
> scheme@(guile-user)> ,use (ice-9 ftw)
> scheme@(guile-user)> (mkdir "/tmp/")
> scheme@(guile-user)> (chmod "/tmp/" #o0700)
> scheme@(guile-user)> (ftw "/tmp/" (lambda (_ __ f) (pk f) #t))
>
> ;;; (directory-not-readable)
> $1 = #t
> scheme@(guile-user)> (system "ls -al /tmp/")
> total 0
> drwx-- 1 wolf wolf   0 Oct 11 22:54 .
> drwxrwxrwt 1 root root 888 Oct 11 22:54 ..
> $2 = 0
>
> The fix is to quote the procedure call, leading to the intended
> behavior.
>
> This fixes bug 55344.
>
> * module/ice-9/ftw.scm (getuid-or-false): Quote the (getuid).
> (getgid-or-false): Quote the (getgid).

Applied, thanks!

bug#68505: [PATCH v2] Add more detailed instructions into the HACKING file.

[Ludovic Courtès]

Tomas Volf <~@wolfsden.cz> skribis:

> Until now, the ./meta/guile was not mentioned anywhere, and therefore it
> was not obvious how to run the locally compiled Guile without installing
> it.
>
> While modifying the file, I took the liberty to also mention a bit about
> compiling Guile using Guix.
>
> Finally, the header lines where cleaned up, ensuring all of them end at
> 70 and have a leading space.
>
> * HACKING (Hacking It Yourself): Add Guix instructions.  Add a note
> about meta/guile script.
> (Sample GDB Initialization File),
> (Naming conventions): Clean up the header line.

Applied, thanks!

bug#61058: [PATCH v2] Fix asymetric mutex locking when joining thread.

[Ludovic Courtès]

Hello,

Olivier Dion  skribis:

> If `join-thread' timeout, the thread mutex is not unlocked, resulting in
> deadlock to the next call to it or deadlock of the thread itself when it
> terminates.
>
> Thus, always unlock the mutex.
>
> Fix: #55356
>
> * module/ice-9/threads.scm (join-thread): Always unlock thread mutex.
> * test-suite/tests/threads.test (join-thread): New test to ensure the
> mutex is released

Pushed as 455ee49f5573baa1bc5237a8d49083ce588a13ee with a ‘NEWS’ entry
and an additional comment in the test.

Thanks!

Ludo’.

bug#68087: Signal handlers not called after ‘primitive-fork’

[Ludovic Courtès]

Ludovic Courtès  skribis:

> Fixes <https://bugs.gnu.org/68087>.
>
> * libguile/scmsigs.h (scm_i_signals_pre_fork, scm_i_signals_post_fork):
> New declarations.
> (scm_i_signal_delivery_thread): Change type to SCM..
> * libguile/threads.c (scm_all_threads): Adjust accordingly and exclude
> threads that have ‘t->exited’.  Access ‘thread_count’ after grabbing
> ‘thread_admin_mutex’.
> * libguile/posix.c (scm_fork): Add calls to ‘scm_i_signals_pre_fork’ and
> ‘scm_i_signals_post_fork’.
> * libguile/scmsigs.c (signal_delivery_thread): Close signal_pipe[0] upon
> exit and set it to -1.
> (once): New file-global variable, moved from…
> (scm_i_ensure_signal_delivery_thread): … here.
> (stop_signal_delivery_thread, scm_i_signals_pre_fork)
> (scm_i_signals_post_fork): New functions.
> * test-suite/standalone/test-sigaction-fork: New file.
> * test-suite/standalone/Makefile.am (check_SCRIPTS, TESTS): Add it.

Pushed as 5a8502a4946e8a5b5c40a127aa240fc6ad960d03.

Ludo’.

bug#68504: [PATCH] Add copy-on-write support to scm_copy_file.

[Ludovic Courtès]

Hi,

Tomas Volf <~@wolfsden.cz> skribis:

> On modern file-systems (BTRFS, ZFS) it is possible to copy a file using
> copy-on-write method.  For large files it has the advantage of being
> much faster and saving disk space (since identical extents are not
> duplicated).  This feature is stable and for example coreutils' `cp'
> does use it automatically (see --reflink).
>
> This commit adds support for this feature into our
> copy-file (scm_copy_file) procedure.  Same as `cp', it defaults to
> 'auto, meaning the copy-on-write is attempted, and in case of failure
> the regular copy is performed.
>
> No tests are provided, because the behavior depends on the system,
> underlying file-system and its configuration.  That makes it challenging
> to write a test for it.  Manual testing was performed instead:
>
> $ btrfs filesystem du /tmp/cow*
>  Total   Exclusive  Set shared  Filename
>   36.00KiB36.00KiB   0.00B  /tmp/cow
>
> $ cat cow-test.scm
> (copy-file "/tmp/cow" "/tmp/cow-unspecified")
> (copy-file "/tmp/cow" "/tmp/cow-always" #:copy-on-write 'always)
> (copy-file "/tmp/cow" "/tmp/cow-auto" #:copy-on-write 'auto)
> (copy-file "/tmp/cow" "/tmp/cow-never" #:copy-on-write 'never)
> (copy-file "/tmp/cow" "/dev/shm/cow-unspecified")
> (copy-file "/tmp/cow" "/dev/shm/cow-auto" #:copy-on-write 'auto)
> (copy-file "/tmp/cow" "/dev/shm/cow-never" #:copy-on-write 'never)
> $ ./meta/guile -s cow-test.scm
>
> $ btrfs filesystem du /tmp/cow*
>  Total   Exclusive  Set shared  Filename
>   36.00KiB   0.00B36.00KiB  /tmp/cow
>   36.00KiB   0.00B36.00KiB  /tmp/cow-always
>   36.00KiB   0.00B36.00KiB  /tmp/cow-auto
>   36.00KiB36.00KiB   0.00B  /tmp/cow-never
>   36.00KiB   0.00B36.00KiB  /tmp/cow-unspecified
>
> $ sha1sum /tmp/cow* /dev/shm/cow*
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-always
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-auto
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-never
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /tmp/cow-unspecified
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /dev/shm/cow-auto
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /dev/shm/cow-never
> 4c665f87b5dc2e7d26279c4b48968d085e1ace32  /dev/shm/cow-unspecified
>
> This commit also adds to new failure modes for (copy-file).
>
> Failure to copy-on-write when 'always was passed in:
>
> scheme@(guile-user)> (copy-file "/tmp/cow" "/dev/shm/cow" #:copy-on-write 
> 'always)
> ice-9/boot-9.scm:1676:22: In procedure raise-exception:
> In procedure copy-file: copy-on-write failed: Invalid cross-device link
>
> Passing in invalid value for the #:copy-on-write keyword argument:
>
> scheme@(guile-user)> (copy-file "/tmp/cow" "/dev/shm/cow" #:copy-on-write 
> 'nevr)
> ice-9/boot-9.scm:1676:22: In procedure raise-exception:
> In procedure copy-file: invalid value for #:copy-on-write: nevr
>
> * NEWS: Add note for copy-file supporting copy-on-write.
> * configure.ac: Check for linux/fs.h.
> * doc/ref/posix.texi (File System)[copy-file]: Document the new
> signature.
> * libguile/filesys.c (clone_file): New function cloning a file using
> FICLONE, if supported.
> (k_copy_on_write): New keyword.
> (sym_always, sym_auto, sym_never): New symbols.
> (scm_copy_file): New #:copy-on-write keyword argument.  Attempt
> copy-on-write copy by default.
> * libguile/filesys.h: Update signature for scm_copy_file.

The patch looks great (and very useful) to me, modulo one issue:

> -SCM_API SCM scm_copy_file (SCM oldfile, SCM newfile);
> +SCM_API SCM scm_copy_file (SCM oldfile, SCM newfile, SCM rest);

Since this is a public interface, we cannot change this function’s
signature during the 3.0 stable series.

Thus, I would suggest keeping the public ‘scm_copy_file’ unchanged and
internally having a three-argument variant.  The Scheme-level
‘copy-file’ would map to that three-argument variant.  (See how
‘scm_pipe’ and ‘scm accept’ as examples.)

Could you send an updated patch?

BTW, copyright assignment to the FSF is now optional but encouraged.
Please see
.

Thanks,
Ludo’.

bug#68505: [PATCH v2] Add more detailed instructions into the HACKING file.

[Ludovic Courtès]

Hi,

Tomas Volf <~@wolfsden.cz> skribis:

> Until now, the ./meta/guile was not mentioned anywhere, and therefore it
> was not obvious how to run the locally compiled Guile without installing
> it.
>
> While modifying the file, I took the liberty to also mention a bit about
> compiling Guile using Guix.
>
> Finally, the header lines where cleaned up, ensuring all of them end at
> 70 and have a leading space.
>
> * HACKING (Hacking It Yourself): Add Guix instructions.  Add a note
> about meta/guile script.
> (Sample GDB Initialization File),
> (Naming conventions): Clean up the header line.
> ---
> v2:
> Add note regarding JIT and GNU Hurd.  Add note regarding -fexcess-precision.
> Add --disable-static and explain it.

Much welcome addition!

> +You can spawn a shell with all the required dependencies using GNU Guix
> +by running the following command:
> +
> +guix shell -D -f guix.scm --pure

I would suggest running:

  guix shell -CP

(Currently ‘README’ suggests ‘guix shell’.)

The rationale is: it’s shorter, using a container avoids interference,
and ‘-P’ works well with ‘config.cache’ & co. (because the cached file
names then are $HOME/.guix-profile/…).

WDYT?

Thanks,
Ludo’.

bug#68507: [PATCH] doc: Fix example in list-transduce example.

[Ludovic Courtès]

Hi,

Tomas Volf <~@wolfsden.cz> skribis:

> While the `.' might be correct from a grammatical point of view (I do
> not know), it turns the example into invalid scheme code, which is not
> ideal.  New users (like me) might try to copy the whole line and wonder
> why it does not work (like I did).  So delete it.
>
> * doc/ref/srfi-modules.texi (SRFI-171 General Discussion): Delete the
> trailing . from the example.

Applied, thanks!

Ludo’.

bug#63366: Docs: Revise or delete reference to “benevolent dictators” in Manual Introduction

[Ludovic Courtès]

Hi,

Sebastian Carlos  skribis:

> My suggestion is to delete the sentence altogether. I think the paragraph
> still makes sense without it, as the reader gets enough information to
> understand how Scheme differs from other programming languages.

I’ve (finally) removed that sentence.  Thanks!

Ludo’.

bug#68087: [PATCH] Stop signal thread before forking, restart it afterwards.

[Ludovic Courtès]

Fixes .

* libguile/scmsigs.h (scm_i_signals_pre_fork, scm_i_signals_post_fork):
New declarations.
(scm_i_signal_delivery_thread): Change type to SCM..
* libguile/threads.c (scm_all_threads): Adjust accordingly and exclude
threads that have ‘t->exited’.  Access ‘thread_count’ after grabbing
‘thread_admin_mutex’.
* libguile/posix.c (scm_fork): Add calls to ‘scm_i_signals_pre_fork’ and
‘scm_i_signals_post_fork’.
* libguile/scmsigs.c (signal_delivery_thread): Close signal_pipe[0] upon
exit and set it to -1.
(once): New file-global variable, moved from…
(scm_i_ensure_signal_delivery_thread): … here.
(stop_signal_delivery_thread, scm_i_signals_pre_fork)
(scm_i_signals_post_fork): New functions.
* test-suite/standalone/test-sigaction-fork: New file.
* test-suite/standalone/Makefile.am (check_SCRIPTS, TESTS): Add it.
---
 libguile/posix.c  |  6 ++
 libguile/scmsigs.c| 62 -
 libguile/scmsigs.h|  6 +-
 libguile/threads.c| 17 +++--
 test-suite/standalone/Makefile.am |  6 +-
 test-suite/standalone/test-sigaction-fork | 85 +++
 6 files changed, 168 insertions(+), 14 deletions(-)
 create mode 100755 test-suite/standalone/test-sigaction-fork

Hello!

Here’s a fix I’ve come up with: just like for the finalizer thread, stop
the signal thread before forking, and in this case start it again after
fork if needed.

It fixes the immediate problem, but the test also shows the perils with
all this asynchronous signal handling.  I’d like to investigate ways to
make signal handling somewhat more predictable (pselect(2), as suggested
on Mastodon by Sergey Bugaev, is probably part of the solution).

Thoughts?

Ludo’.

diff --git a/libguile/posix.c b/libguile/posix.c
index 6ce78ee18..f7d68200b 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1295,7 +1295,10 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
 #define FUNC_NAME s_scm_fork
 {
   int pid;
+
   scm_i_finalizer_pre_fork ();
+  scm_i_signals_pre_fork ();
+
   if (scm_ilength (scm_all_threads ()) != 1)
 /* Other threads may be holding on to resources that Guile needs --
it is not safe to permit one thread to fork while others are
@@ -1317,6 +1320,9 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
 
   if (pid == -1)
 SCM_SYSERROR;
+
+  scm_i_signals_post_fork ();
+
   return scm_from_int (pid);
 }
 #undef FUNC_NAME
diff --git a/libguile/scmsigs.c b/libguile/scmsigs.c
index f7c3d7fbd..3d4e72a2b 100644
--- a/libguile/scmsigs.c
+++ b/libguile/scmsigs.c
@@ -86,7 +86,7 @@ static SCM signal_handler_asyncs;
 static SCM signal_handler_threads;
 
 /* The signal delivery thread.  */
-scm_thread *scm_i_signal_delivery_thread = NULL;
+SCM scm_i_signal_delivery_thread = SCM_BOOL_F;
 
 /* The mutex held when launching the signal delivery thread.  */
 static scm_i_pthread_mutex_t signal_delivery_thread_mutex =
@@ -196,6 +196,9 @@ signal_delivery_thread (void *data)
perror ("error in signal delivery thread");
 }
 
+  close (signal_pipe[0]);
+  signal_pipe[0] = -1;
+
   return SCM_UNSPECIFIED; /* not reached unless all other threads exited */
 }
 
@@ -211,18 +214,35 @@ start_signal_delivery_thread (void)
   signal_thread = scm_spawn_thread (signal_delivery_thread, NULL,
scm_handle_by_message,
"signal delivery thread");
-  scm_i_signal_delivery_thread = SCM_I_THREAD_DATA (signal_thread);
+  scm_i_signal_delivery_thread = signal_thread;
 
   scm_i_pthread_mutex_unlock (_delivery_thread_mutex);
 }
 
+static scm_i_pthread_once_t once = SCM_I_PTHREAD_ONCE_INIT;
+
 void
 scm_i_ensure_signal_delivery_thread ()
 {
-  static scm_i_pthread_once_t once = SCM_I_PTHREAD_ONCE_INIT;
   scm_i_pthread_once (, start_signal_delivery_thread);
 }
 
+static void
+stop_signal_delivery_thread ()
+{
+  scm_i_pthread_mutex_lock (_delivery_thread_mutex);
+
+  if (scm_is_true (scm_i_signal_delivery_thread))
+{
+  close (signal_pipe[1]);
+  signal_pipe[1] = -1;
+  scm_join_thread (scm_i_signal_delivery_thread);
+  scm_i_signal_delivery_thread = SCM_BOOL_F;
+}
+
+  scm_i_pthread_mutex_unlock (_delivery_thread_mutex);
+}
+
 #else /* !SCM_USE_PTHREAD_THREADS */
 
 static void
@@ -248,8 +268,44 @@ scm_i_ensure_signal_delivery_thread ()
   return;
 }
 
+static void
+stop_signal_delivery_thread ()
+{
+  return;
+}
+
 #endif /* !SCM_USE_PTHREAD_THREADS */
 
+/* Perform pre-fork cleanup by stopping the signal delivery thread.  */
+void
+scm_i_signals_pre_fork ()
+{
+  stop_signal_delivery_thread ();
+}
+
+/* Perform post-fork setup by restarting the signal delivery thread if
+   it was active before fork.  This happens in both the parent and the
+   child process.  */
+void
+scm_i_signals_post_fork ()
+{
+  int active = 0;
+
+  for (int sig = 0; sig < NSIG; sig++)
+{
+  if (scm_is_true (SCM_SIMPLE_VECTOR_REF 

bug#68087: Signal handlers not called after ‘primitive-fork’

[Ludovic Courtès]

In 3.0.9 and current ‘main’, I get this:

--8<---cut here---start->8---
$ cat sigaction-fork.scm
(use-modules (ice-9 match))

;; This call spawns the signal delivery thread as a side effect.
(sigaction SIGALRM
  (lambda (signal)
(pk 'got-signal! signal)))

(match (primitive-fork)
  (0
   (pk 'child (getpid))
   (sigaction SIGALRM
 (lambda (signal)
   ;; This handler is never called!
   (pk 'got-signal-child! signal)))
   (kill 0 SIGALRM)
   (pk 'alarm-sent))
  (_
   (primitive-exit 0)))
$ guile sigaction-fork.scm

;;; (child 30308)

;;; (alarm-sent)
--8<---cut here---end--->8---

Everything works fine if we remove the pre-fork ‘sigaction’ call.

Ludo’.

bug#64293: [PATCH] doc: Use archived URL from Internet Archive for syntax-rules primer.

[Ludovic Courtès]

Hi,

Bruno Victal  skribis:

> * doc/ref/api-macros.texi (Syntax Rules): Use archived URL from
> Internet Archive for syntax-rules primer.

Finally applied, thanks!

Ludo’.

bug#62690: Guile 3.0.9 (read-u8) defaults to current-output-port

[Ludovic Courtès]

Hi,

Rui Zhang  skribis:

> In scheme/base.scm:
>
> (define* (read-u8 #:optional (port (current-output-port)))
>   (get-u8 port))
>
> I think this is a typo, where current-output-port should be 
> current-input-port?

Oops, fixed, thanks!

Ludo’.

bug#62691: Calling system* in the module body hangs Guile, while calling open-pipe* does not

[Ludovic Courtès]

Hi,

Timothy Sample  skribis:

>>From 6b6792c7a21de9be1825719bfca0f01177381cf9 Mon Sep 17 00:00:00 2001
> From: Timothy Sample 
> Date: Tue, 11 Apr 2023 10:22:46 -0600
> Subject: [PATCH] Avoid module resolution in 'call-with-new-thread'.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Fixes .
> Reported by Михаил Бахтерев .
>
> * module/ice-9/threads.scm (call-with-new-thread): Do not use 'set!'
> to set object properties while the calling thread is waiting on the
> new thread to initialize.

Woow, good catch!  Finally, applied.  Thanks!

Ludo’.

bug#62729: [PATCH] Fix dangling pointers in `environ'

[Ludovic Courtès]

Hi Olivier,

Olivier Dion  skribis:

> From: Olivier Dion 
>
> When calling `environ', Guile set the global variable `environ' to a
> list allocated with the GC.  Strings in it are also allocated with the
> GC.
>
> However, if an user call the Scheme setenv() procedure, the resulting
> call to putenv() in libc might reallocate `environ' to a new pointer
> while copying sub-pointers owned by Guile in it.
>
> This results in the GC marking these strings for reclamation when they
> are actually still present in `environ'.  Thus, the values in the
> environment are now undefined.
>
> To fix this, Guile should only manipulate the `environ' using the
> standard libc functions.  This ensures that concurrent modification of
> it is safe in multi-threaded program.  Therefore, the procedure
> `environ' now call the libc clearenv() procedure to purge the
> environment.  Then, the desired values are put in `environ' using
> scm_putenv().  At the end, no GC allocated memory is put in `environ'.
>
> Also, since `environ' can be changed at anytime in a multi-thread
> program, emit a warning stipulating that the result is undefined
> behavior if multiple threads are created in the program.  Consider for
> example a thread iterating over `environ' while another one do a call to
> putenv().  The latter would do a realloc() on `environ' and thus the old
> array read by the former now contains garbage.
>
> On system where clearenv() is not present, an atomic store of NULL with
> sequential consistency to `environ' should be sufficient but see the
> NOTES of clearenv(3).
>
> * libguile/posix.c (scm_environ): Do not store GC allocated memory in
> environ.

Thanks for the clear explanation and patch.  Finally applied with an
added comment in the code.

Ludo’.

bug#63805: [PATCH] Fix typos throughout codebase

[Ludovic Courtès]

Hi,

Morgan Smith  skribis:

> Fix typos.
> ---
>
> I do hope the commit message is good enough.  Anything more specific would be
> hard.
>
> This was more of an undertaking then I initally thought so I didn't actually
> get through as much of the repository as I wanted.

I can imagine.  :-)

Finally applied, thanks!

Ludo’.

bug#64666: Nested ‘scm_sigaction_for_thread’ calls lead to deadlock

[Ludovic Courtès]

Ludovic Courtès  skribis:

> It’s possible for ‘sigaction’ (aka. ‘scm_sigaction_for_thread’) to run
> asyncs, which in turn call ‘scm_sigaction_for_thread’ for the very same
> thread, leading to a deadlock:

Fixed in 85520354a8f5de0366c4ac3eb5403aeb27c9515e.

Ludo’.

bug#64666: Nested ‘scm_sigaction_for_thread’ calls lead to deadlock

[Ludovic Courtès]

Hello,

It’s possible for ‘sigaction’ (aka. ‘scm_sigaction_for_thread’) to run
asyncs, which in turn call ‘scm_sigaction_for_thread’ for the very same
thread, leading to a deadlock:

--8<---cut here---start->8---
(gdb) bt
#0  0x7f823bcdf32b in __lll_lock_wait ()
   from /gnu/store/gsjczqir1wbz8p770zndrpw4rnppmxi3-glibc-2.35/lib/libc.so.6
#1  0x7f823bce5572 in pthread_mutex_lock@@GLIBC_2.2.5 ()
   from /gnu/store/gsjczqir1wbz8p770zndrpw4rnppmxi3-glibc-2.35/lib/libc.so.6
#2  0x7f823c278e45 in scm_pthread_mutex_lock (mutex=)
at /home/ludo/src/guile-3.0/libguile/threads.c:1616
#3  0x7f823c27cc79 in scm_dynwind_pthread_mutex_lock (
mutex=0x7f823c2fa240 )
at /home/ludo/src/guile-3.0/libguile/threads.c:1629
#4  0x7f823c25d254 in scm_sigaction_for_thread (signum=, 
handler=0x7f8239940258, flags=0x904, thread=0x7f823ba38320)
at /home/ludo/src/guile-3.0/libguile/scmsigs.c:339
#5  0x7f823990f257 in ?? ()
#6  0x01918f38 in ?? ()
#7  0x7f8239910b50 in ?? ()
#8  0x7f823ba1ad80 in ?? ()
#9  0x7f823c22556c in scm_jit_enter_mcode (thread=0x7f823ba1ad80, 
mcode=0x191f55c "\034\r\002") at 
/home/ludo/src/guile-3.0/libguile/jit.c:6061
#10 0x7f823c2812c5 in vm_regular_engine (thread=0x7f823c330d68)
at /home/ludo/src/guile-3.0/libguile/vm-engine.c:360
#11 0x7f823c28af95 in scm_call_n (proc=, argv=, nargs=0)
at /home/ludo/src/guile-3.0/libguile/vm.c:1616
#12 0x7f823c1f25a0 in scm_async_tick () at 
/home/ludo/src/guile-3.0/libguile/async.c:154
#13 0x7f823c1f8545 in scm_dynstack_unwind_frame (dynstack=0x7f823ba1af88)
at /home/ludo/src/guile-3.0/libguile/dynstack.c:628
#14 scm_dynwind_end () at /home/ludo/src/guile-3.0/libguile/dynwind.c:71
#15 0x7f823c25d478 in scm_sigaction_for_thread (signum=, 
handler=0x7f8239940258, flags=, thread=0x7f823ba38320)
at /home/ludo/src/guile-3.0/libguile/scmsigs.c:447
--8<---cut here---end--->8---

Notice the nested ‘scm_sigaction_for_thread’ call for 0x7f823ba38320.

This happens while letting the code below run for a minute or so:

--8<---cut here---start->8---
(use-modules (ice-9 match))

(setvbuf (current-output-port) 'line)

(match (primitive-fork)
  (0
   (format #t "child: ~a~%" (getpid))
   (letrec ((handler (lambda args
   (pk 'SIGUSR1! args)
   (sigaction SIGUSR1 handler
 (sigaction SIGUSR1 handler))
   (let loop ()
 (pk 'slept (sleep 10))
 (loop)))
  (pid
   (sleep 1)
   (let loop ()
 (usleep 1)
 (kill pid SIGUSR1)
 (loop
--8<---cut here---end--->8---

Ludo’.

bug#62501: [3.0.9] ‘spawn’ test fails on GNU/Hurd, due to $LD_ORIGIN_PATH

[Ludovic Courtès]

Ludovic Courtès  skribis:

> On GNU/Hurd, the ‘exec’ server always sets ‘LD_ORIGIN_PATH’ in the
> environment of programs it executes—see ‘do_exec’ in ‘exec/exec.c’.
> Consequently, the ‘spawn’ test that checks environment variables fails:
>
> Running posix.test
> UNRESOLVED: posix.test: affinity: getaffinity
> UNRESOLVED: posix.test: affinity: setaffinity
> FAIL: posix.test: spawn: env with #:environment and #:output - arguments: 
> (expected-value "GNU=guile\n" actual-value 
> "LD_ORIGIN_PATH=/gnu/store/9ln4hd7c1p7vn31wi0cf4pp8r9sy6pvw-coreutils-8.32/bin\nGNU=guile\n")
> UNRESOLVED: posix.test: spawn: ls /proc/self/fd

Fixed in e93525e549ba4d5ec52ec90aaf051f546d854cea.

Ludo'.

bug#61095: possible misuse of posix_spawn API on non-linux OSes

[Ludovic Courtès]

Hi!

Omar Polo  skribis:

> On 2023/03/30 22:21:28 +0200, Josselin Poiret  wrote:
>> Hi Ludo,
>> 
>> Ludovic Courtès  writes:
>> 
>> > Coming next is an updated patch series addressing this as proposed
>> > above.  Let me know what y’all think!
>> >
>> > I tested the ‘posix_spawn_file_actions_addclosefrom_np’ path by building 
>> > in:
>> >
>> >   guix time-machine --branch=core-updates -- shell -CP -D -f guix.scm
>> 
>> I didn't test, but this LGTM!  Maybe someone on OpenBSD could test this
>> patchset?
>
> % gmake check
> 
> gmake[5]: Entering directory '/home/op/w/guile/test-suite/standalone'
> PASS: test-system-cmds
>
> it seems to work on OpenBSD 7.3 :)

Awesome!  Pushed as 9cc85a4f52147fcdaa4c52a62bcc87bdb267d0a9.

> but note that our libc doesn't have posix_spawn_file_actions_addclosefrom_np,
> so this is using the "racy" code path.

Yeah, not great.  :-/  I hope that function will be adopted by other
libcs, especially since ‘closefrom’ is already available.

> Just for curiosity, as it's outside the scope of the bug, what's the
> reason posix_spawn was used instead of a more classic fork() +
> closefrom()?

There’s a long discussion at:

  https://issues.guix.gnu.org/52835

Essentially, ‘fork’ is unusable in multi-threaded context, in addition
to being inefficient.

Thanks,
Ludo’.

bug#61095: [PATCH 1/3] 'spawn' closes only open file descriptors on non-GNU/Linux systems.

[Ludovic Courtès]

Fixes .
Reported by Omar Polo .

* libguile/posix.c (close_inherited_fds_slow): On systems other than
GNU/Linux, call 'addclose' only when 'fcntl' succeeds on MAX_FD.
---
 libguile/posix.c | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 3a8be94e4..68e9bfade 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1326,7 +1326,24 @@ static void
 close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
 {
   while (--max_fd > 2)
-posix_spawn_file_actions_addclose (actions, max_fd);
+{
+  /* Adding a 'close' action for a file descriptor that is not open
+ causes 'posix_spawn' to fail on GNU/Hurd and on OpenBSD, but
+ not on GNU/Linux: .  Hence this
+ strategy:
+
+   - On GNU/Linux, close every FD, since that's the only
+ race-free way to make sure the child doesn't inherit one.
+   - On other systems, only close FDs currently open in the
+ parent; it works, but it's racy (XXX).
+
+ The only reliable option is 'addclosefrom'.  */
+#if ! (defined __GLIBC__ && defined __linux__)
+  int flags = fcntl (max_fd, F_GETFD, NULL);
+  if (flags >= 0)
+#endif
+posix_spawn_file_actions_addclose (actions, max_fd);
+}
 }
 
 static void

base-commit: e334e59589c3cbfc68d3f7d0d739000e0876b36d
-- 
2.39.2

bug#61095: [PATCH 2/3] Remove racy optimized file descriptor closing loop in 'spawn'.

[Ludovic Courtès]

This reverts 9332b632407894c2e1951cce1bc678f19e1fa8e4, thereby
reinstating the performance issue in .

This optimization was subject to race conditions in multi-threaded code:
new file descriptors could pop up at any time and thus leak in the
child.

* libguile/posix.c (close_inherited_fds): Remove.
(close_inherited_fds_slow): Rename to...
(close_inherited_fds): ... this.
---
 libguile/posix.c | 30 +-
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 68e9bfade..b5830c43b 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1323,7 +1323,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
 #endif /* HAVE_FORK */
 
 static void
-close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
+close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
 {
   while (--max_fd > 2)
 {
@@ -1346,34 +1346,6 @@ close_inherited_fds_slow (posix_spawn_file_actions_t 
*actions, int max_fd)
 }
 }
 
-static void
-close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
-{
-  DIR *dirp;
-  struct dirent *d;
-  int fd;
-
-  /* Try to use the platform-specific list of open file descriptors, so
- we don't need to use the brute force approach. */
-  dirp = opendir ("/proc/self/fd");
-
-  if (dirp == NULL)
-return close_inherited_fds_slow (actions, max_fd);
-
-  while ((d = readdir (dirp)) != NULL)
-{
-  fd = atoi (d->d_name);
-
-  /* Skip "." and "..", garbage entries, stdin/stdout/stderr. */
-  if (fd <= 2)
-continue;
-
-  posix_spawn_file_actions_addclose (actions, fd);
-}
-
-  closedir (dirp);
-}
-
 static pid_t
 do_spawn (char *exec_file, char **exec_argv, char **exec_env,
   int in, int out, int err, int spawnp)
-- 
2.39.2

bug#61095: possible misuse of posix_spawn API on non-linux OSes

[Ludovic Courtès]

Hi!

Josselin Poiret  skribis:

> Ludovic Courtès  writes:
>
>> -  posix_spawn_file_actions_addclose (actions, fd);
>> +  /* Adding invalid file descriptors to an 'addclose' action leads
>> + to 'posix_spawn' failures on some operating systems:
>> + <https://bugs.gnu.org/61095>.  Hence the extra check.  */
>> +  int flags = fcntl (max_fd, F_GETFD, NULL);
>> +  if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0))
>> +posix_spawn_file_actions_addclose (actions, max_fd);
>>  }
>
> I'm worried about TOCTOU in multi-threaded contexts here,

Yes, that’s a problem.  The current /proc/self/fd optimization has that
problem too.  :-/

> which is why I opted for the heavy handed-approach here.  In general I
> don't think we can know in advance which fdes to close :/ It's a shame
> that the posix_spawn actions fails on other kernels, since I don't
> really see a way to mitigate this problem (apart from the new
> posix_spawn_file_actions_addclosefrom_np as you mentioned).  I don't
> know what we could do here.  Maybe not provide spawn?  Or provide it
> in spite of the broken fd closing?

Not providing ‘spawn’ is no longer an option.

We can expect the problem to practically vanish “soon” on GNU variants
with ‘closefrom’ (glibc 2.34 was released in Aug. 2021).

On Linux with glibc < 2.34, we’d keep the current code (maybe without
the /proc/self/fd optimization?).

On other systems, we can have the racy code above as a last resort or
OS-specific tricks, like Omar was suggesting for OpenBSD.  It sucks but
what else can we do?

(BTW,
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn_file_actions_addclose.html>
reads:

  It shall not be considered an error for the fildes argument passed to
  these functions to specify a file descriptor for which the specified
  operation could not be performed at the time of the call. Any such
  error will be detected when the associated file actions object is
  later used during a posix_spawn() or posix_spawnp() operation.

OpenBSD and GNU/Hurd follow this to the letter.

OTOH ‘linux/spawni.c’ in glibc is purposefully more liberal:

  /* Signal errors only for file descriptors out of range.  */
)

>> +  /* Duplicate IN, OUT, and ERR unconditionally to clear their
>> + FD_CLOEXEC flag, if any.  */
>> +  posix_spawn_file_actions_adddup2 (, in, STDIN_FILENO);
>> +  posix_spawn_file_actions_adddup2 (, out, STDOUT_FILENO);
>> +  posix_spawn_file_actions_adddup2 (, err, STDERR_FILENO);
>
> This won't work, and actually this was one of the original logic bugs I
> was trying to fix.  If err is equal to, let's say, STDIN_FILENO, then
> the first call will overwrite the initial file descriptor at
> STDIN_FILENO, and the second call won't do what the caller intended.
> This is why I was moving them out of the way first, so that they would
> not overwrite each other.

Oh, my bad.

>> +  /* TODO: Use 'closefrom' where available.  */
>> +#if 0
>> +  /* Version 2.34 of the GNU libc provides this function.  */
>> +  posix_spawn_file_actions_addclosefrom_np (, 3);
>> +#else
>> +  if (in > 2)
>> +posix_spawn_file_actions_addclose (, in);
>> +  if (out > 2 && out != in)
>> +posix_spawn_file_actions_addclose (, out);
>> +  if (err > 2 && err != out && err != in)
>> +posix_spawn_file_actions_addclose (, err);
>
> Isn't this unneeded given we call close_inherited_fds below?

No, because of the FD_CLOEXEC selection.

Coming next is an updated patch series addressing this as proposed
above.  Let me know what y’all think!

I tested the ‘posix_spawn_file_actions_addclosefrom_np’ path by building in:

  guix time-machine --branch=core-updates -- shell -CP -D -f guix.scm

… which gives us glibc 2.35.

Ludo’.

bug#61095: possible misuse of posix_spawn API on non-linux OSes

[Ludovic Courtès]

Hi Omar,

Apologies for the late reply.

Omar Polo  skribis:

> I've noticed that test-system-cmds fails on OpenBSD-CURRENT while
> testing the update to guile 3.0.9:
>
> test-system-cmds: system* exit status was 127 rather than 42
> FAIL: test-system-cmds

We’re seeing the same failure on GNU/Hurd:

  https://issues.guix.gnu.org/61079

> Actually I can avoid the EBADF by checking that the fd is 'live' with
> something like fstat:
>
> [[[
>
> Index: libguile/posix.c
> --- libguile/posix.c.orig
> +++ libguile/posix.c
> @@ -1325,8 +1325,12 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
>  static void
>  close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
>  {
> -  while (--max_fd > 2)
> -posix_spawn_file_actions_addclose (actions, max_fd);
> +  struct stat sb;
> +  max_fd = getdtablecount();
> +  while (--max_fd > 2) {
> +if (fstat(max_fd, ) != -1)
> +  posix_spawn_file_actions_addclose (actions, max_fd);
> +  }
>  }

I came up with the following patch:

diff --git a/libguile/posix.c b/libguile/posix.c
index 3a8be94e4..cde199888 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1322,39 +1322,18 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
 #undef FUNC_NAME
 #endif /* HAVE_FORK */
 
-static void
-close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
-{
-  while (--max_fd > 2)
-posix_spawn_file_actions_addclose (actions, max_fd);
-}
-
 static void
 close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
 {
-  DIR *dirp;
-  struct dirent *d;
-  int fd;
-
-  /* Try to use the platform-specific list of open file descriptors, so
- we don't need to use the brute force approach. */
-  dirp = opendir ("/proc/self/fd");
-
-  if (dirp == NULL)
-return close_inherited_fds_slow (actions, max_fd);
-
-  while ((d = readdir (dirp)) != NULL)
+  while (--max_fd > 2)
 {
-  fd = atoi (d->d_name);
-
-  /* Skip "." and "..", garbage entries, stdin/stdout/stderr. */
-  if (fd <= 2)
-continue;
-
-  posix_spawn_file_actions_addclose (actions, fd);
+  /* Adding invalid file descriptors to an 'addclose' action leads
+ to 'posix_spawn' failures on some operating systems:
+ .  Hence the extra check.  */
+  int flags = fcntl (max_fd, F_GETFD, NULL);
+  if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0))
+posix_spawn_file_actions_addclose (actions, max_fd);
 }
-
-  closedir (dirp);
 }
 
 static pid_t
@@ -1366,6 +1345,26 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
   posix_spawn_file_actions_t actions;
   posix_spawnattr_t *attrp = NULL;
 
+  posix_spawn_file_actions_init ();
+
+  /* Duplicate IN, OUT, and ERR unconditionally to clear their
+ FD_CLOEXEC flag, if any.  */
+  posix_spawn_file_actions_adddup2 (, in, STDIN_FILENO);
+  posix_spawn_file_actions_adddup2 (, out, STDOUT_FILENO);
+  posix_spawn_file_actions_adddup2 (, err, STDERR_FILENO);
+
+  /* TODO: Use 'closefrom' where available.  */
+#if 0
+  /* Version 2.34 of the GNU libc provides this function.  */
+  posix_spawn_file_actions_addclosefrom_np (, 3);
+#else
+  if (in > 2)
+posix_spawn_file_actions_addclose (, in);
+  if (out > 2 && out != in)
+posix_spawn_file_actions_addclose (, out);
+  if (err > 2 && err != out && err != in)
+posix_spawn_file_actions_addclose (, err);
+
   int max_fd = 1024;
 
 #if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
@@ -1376,31 +1375,8 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
   }
 #endif
 
-  posix_spawn_file_actions_init ();
-
-  int free_fd_slots = 0;
-  int fd_slot[3];
-
-  for (int fdnum = 3; free_fd_slots < 3 && fdnum < max_fd; fdnum++)
-{
-  if (fdnum != in && fdnum != out && fdnum != err)
-{
-  fd_slot[free_fd_slots] = fdnum;
-  free_fd_slots++;
-}
-}
-
-  /* Move the fds out of the way, so that duplicate fds or fds equal
- to 0, 1, 2 don't trample each other */
-
-  posix_spawn_file_actions_adddup2 (, in, fd_slot[0]);
-  posix_spawn_file_actions_adddup2 (, out, fd_slot[1]);
-  posix_spawn_file_actions_adddup2 (, err, fd_slot[2]);
-  posix_spawn_file_actions_adddup2 (, fd_slot[0], 0);
-  posix_spawn_file_actions_adddup2 (, fd_slot[1], 1);
-  posix_spawn_file_actions_adddup2 (, fd_slot[2], 2);
-
   close_inherited_fds (, max_fd);
+#endif
 
   int res = -1;
   if (spawnp)

Could you confirm that it works on OpenBSD and that there’s no
performance regression?

Andrew: it removes the /proc/self/fd loop you added to fix
, but it reduces the number of ‘close’ calls
in the child.  Could you check whether that’s okay performance-wise?

Eventually I plan to use ‘posix_spawn_file_actions_addclosefrom_np’ on
glibc >= 2.34, but I have yet to test it.  That will be the best
solution.

Josselin: I simplified the ‘dup2’ logic somewhat.

Feedback welcome!

> The regress passes and while this workaround may be 

bug#62501: [3.0.9] ‘spawn’ test fails on GNU/Hurd, due to $LD_ORIGIN_PATH

[Ludovic Courtès]

On GNU/Hurd, the ‘exec’ server always sets ‘LD_ORIGIN_PATH’ in the
environment of programs it executes—see ‘do_exec’ in ‘exec/exec.c’.
Consequently, the ‘spawn’ test that checks environment variables fails:

--8<---cut here---start->8---
Running posix.test
UNRESOLVED: posix.test: affinity: getaffinity
UNRESOLVED: posix.test: affinity: setaffinity
FAIL: posix.test: spawn: env with #:environment and #:output - arguments: 
(expected-value "GNU=guile\n" actual-value 
"LD_ORIGIN_PATH=/gnu/store/9ln4hd7c1p7vn31wi0cf4pp8r9sy6pvw-coreutils-8.32/bin\nGNU=guile\n")
UNRESOLVED: posix.test: spawn: ls /proc/self/fd
--8<---cut here---end--->8---

(This is after applying a fix for .)

Ludo’.

bug#62469: ‘throw’ introduces a continuation barrier

[Ludovic Courtès]

‘throw’ introduces a continuation barrier as of Guile 3.0.9:

--8<---cut here---start->8---
$ cat ~/src/guile-debugging/suspendable-continuation.scm
(use-modules (ice-9 control))

(let ((tag (make-prompt-tag)))
  (call-with-prompt tag
(lambda ()
  (catch #t
(lambda ()
  (pk 'suspendable-from-catch? tag (suspendable-continuation? tag))
  (throw 'whatever))
(const #t)
(lambda args
  (pk 'suspendable-from-exn-handler? tag (suspendable-continuation? 
tag)
(const #t)))
$ guile ~/src/guile-debugging/suspendable-continuation.scm

;;; (suspendable-from-catch? ("prompt") #t)

;;; (suspendable-from-exn-handler? ("prompt") #f)
--8<---cut here---end--->8---

Apparently this is because that goes through ‘scm_throw’ via
‘intrinsics.c’.

A practical consequence is that a REPL running in Fibers on non-blocking
ports enters an endless “Attempt to suspend fiber within continuation
barrier” loop when it starts a recursive REPL due to an uncaught
exception.

Ludo’.

bug#62290: Error when handling invalid unicode with suspendable ports

[Ludovic Courtès]

Hello,

Christopher Baines  skribis:

> Based on the implementation in ports.c.  I don't understand what this
> code is really doing, but the suspendable ports implementation differs
> from the similar C code for a couple of inequalities.
>
> * module/ice-9/suspendable-ports.scm (decode-utf8, bad-utf8-len): Flip a
> couple of inequalities.
> * test-suite/tests/ports.test ("string ports"): Add additional invalid
> UTF-8 test case.

Pushed as cba2e7e3fec3c781230570f5d1ef070625eeeda8.

Thanks for documenting the problem and providing a perfect patch!

Ludo’.

bug#56413: [PATCH v3 1/1] scm_i_utf8_string_hash: compute u8 chars not bytes

[Ludovic Courtès]

Hi Rob,

Rob Browning  skribis:

> Noticed while investigating a migration to utf-8 strings.  After making
> changes that routed non-ascii symbol hashing through this function,
> encoding-iso88597.test began intermittently failing because it would
> traverse trailing garbage when u8_strnlen reported 8 chars instead of 4.
>
> Change the scm_i_str2symbol and scm_i_str2uninterned_symbol internal
> hash type to unsigned long to explicitly match the scm_i_string_hash
> result type.
>
> * libguile/hash.c (scm_i_utf8_string_hash): Call u8_mbsnlen not u8_strnlen.
> * libguile/symbols.c (scm_i_str2symbol, scm_i_str2uninterned_symbol):
> Use unsigned long for scm_i_string_hash result.
> * test-suite/standalone/.gitignore: Add test-hashing.
> * test-suite/standalone/Makefile.am: Add test-hashing.
> * test-suite/standalone/test-hashing.c: Add.

Still LGTM, please push!  :-)

Ludo’.

bug#56413: [PATCH v2 1/1] scm_i_utf8_string_hash: compute u8 chars not bytes

[Ludovic Courtès]

Hi,

Rob Browning  skribis:

> Noticed while investigating a migration to utf-8 strings.  After making
> changes that routed non-ascii symbol hashing through this function,
> encoding-iso88597.test began intermittently failing because it would
> traverse trailing garbage when u8_strnlen reported 8 chars instead of 4.
>
> Change the scm_i_str2symbol and scm_i_str2uninterned_symbol internal
> hash type to unsigned long to explicitly match the scm_i_string_hash
> result type.
>
> * libguile/hash.c (scm_i_utf8_string_hash): Call u8_mbsnlen not u8_strnlen.
> * libguile/symbols.c (scm_i_str2symbol, scm_i_str2uninterned_symbol):
> Use unsigned long for scm_i_string_hash result.
> * test-suite/standalone/.gitignore: Add test-hashing.
> * test-suite/standalone/Makefile.am: Add test-hashing.
> * test-suite/standalone/test-hashing.c: Add.

LGTM, thanks!

Please update ‘NEWS’ too, under a new “Bug fixes” heading.

Ludo’.

bug#61660: [feature request] optimization of case-lambda

[Ludovic Courtès]

Hi Daniel,

lloda  skribis:

> From 61ed612fb36108e395bdee4b1bbb46b49ef017b3 Mon Sep 17 00:00:00 2001
> From: Daniel Llorens 
> Date: Thu, 23 Feb 2023 17:38:10 +0100
> Subject: [PATCH] peval reduces some inlined case-lambda calls
>
> * module/language/tree-il/peval.scm (peval): Reduce multiple case lambda
>   in  trees according to the number of arguments. Do not try to
>   reduce case-lambda using keyword arguments.
> * test-suite/tests/peval.test: Tests.

[...]

> +++ b/module/language/tree-il/peval.scm
> @@ -1668,6 +1668,29 @@ top-level bindings from ENV and return the resulting 
> expression."
>  
>(log 'inline-end result exp)
>result)
> +   (($  src-proc meta orig-body)
> +;; If there are multiple cases and one matches nargs, omit all 
> the others.
> +(or (and
> + (lambda-case-alternate orig-body)
> + (let ((nargs (length orig-args)))
> +   (let loop ((body orig-body))
> + (match body
> +   (#f #f) ;; No matching case; an error.
> +   (($  src-case req opt rest kw inits 
> gensyms case-body alt)
> +(cond (kw
> +   ;; FIXME: Not handling keyword cases.
> +   #f)

Maybe s/FIXME/XXX/ since it’s at most a limitation, certainly not a bug.

It LGTM and Andy already approved it on IRC, so go ahead!

Ludo’.

bug#61086: [3.0.9] Wrong ‘AR’ value in ‘--enable-lto’ builds

[Ludovic Courtès]

In 3.0.9, ‘configure --enable-lto’ goes like this:

--8<---cut here---start->8---
checking for ar... ar
checking the archiver (ar) interface... ar
checking for ar... (cached) ar
checking for ranlib... ranlib
checking for gcc option to enable large file support... none needed
configure: autobuild project... GNU Guile
configure: autobuild revision... 3.0.9
./configure: line 8292: hostname: command not found
configure: autobuild timestamp... 20230126T203648Z
checking whether the compiler supports -flto... yes
checking for lto-specific prefix for ar, nm, objcopy, ranlib... gcc
checking for gcc-nm... gcc-nm
checking for gcc-objcopy... no
checking for objcopy... objcopy
checking compiler's C standard... c11
--8<---cut here---end--->8---

Notice that there’s no line for ‘gcc-ar’.  ‘configure.ac’ reads this:

--8<---cut here---start->8---
AC_MSG_CHECKING([for lto-specific prefix for ar, nm, objcopy, ranlib])
if test "$GCC" = yes; then
   TOOLCHAIN_PREFIX=gcc
else
   # Assuming LLVM if not GCC.  Probably won't hurt.
   TOOLCHAIN_PREFIX=llvm
fi
AC_MSG_RESULT([$TOOLCHAIN_PREFIX])
AC_CHECK_TOOLS([AR], [$TOOLCHAIN_PREFIX-ar ar])
AC_CHECK_TOOLS([NM], [$TOOLCHAIN_PREFIX-nm nm])
AC_CHECK_TOOLS([OBJCOPY], [$TOOLCHAIN_PREFIX-objcopy objcopy])
AC_CHECK_TOOLS([RANLIB], [$TOOLCHAIN_PREFIX-ranlib ranlib])
--8<---cut here---end--->8---

… but here the ‘AR’ and ‘RANLIB’ bits are omitted, because their value
were already computed earlier.

Contrast with 3.0.8, where the LTO tool search happened before:

--8<---cut here---start->8---
checking dependency style of gcc... gcc3
checking whether the compiler supports -flto... yes
checking for lto-specific prefix for ar, nm, objcopy, ranlib... gcc
checking for gcc-ar... gcc-ar
checking for gcc-nm... gcc-nm
checking for gcc-objcopy... no
checking for objcopy... objcopy
checking for gcc-ranlib... gcc-ranlib
checking how to enable C11 support... -std=gnu11

[...]

checking for Minix Amsterdam compiler... no
checking the archiver (gcc-ar) interface... ar
checking for ar... (cached) gcc-ar
checking for special C compiler options needed for large files... no
--8<---cut here---end--->8---

The effect is that 3.0.9 fails to build when doing a static build as
with (@ (gnu packages make-bootstrap) %guile-static-3.0) in Guix:

--8<---cut here---start->8---
  CCLD libguile-3.0.la
ar: libguile_3.0_la-alist.o: plugin needed to handle lto object
ranlib: .libs/libguile-3.0.a(libguile_3.0_la-alist.o): plugin needed to handle 
lto object
  CCLD guile
ld: /tmp/guix-build-guile-static-3.0.9.drv-0/ccQkracO.ltrans0.ltrans.o: in 
function `inner_main':
/tmp/guix-build-guile-static-3.0.9.drv-0/guile-3.0.9/libguile/guile.c:50: 
undefined reference to `scm_shell'
ld: /tmp/guix-build-guile-static-3.0.9.drv-0/ccQkracO.ltrans0.ltrans.o: in 
function `main':
/tmp/guix-build-guile-static-3.0.9.drv-0/guile-3.0.9/libguile/guile.c:94: 
undefined reference to `scm_boot_guile'
collect2: error: ld returned 1 exit status
--8<---cut here---end--->8---

(Notice the “plugin needed” message, due to the fact that we’re using
‘ranlib’ instead of ‘gcc-ranlib’.)

This may be a side effect of aeb22f486139f457ae7fc44c2d931312aaae52d8,
which moved ‘gl_EARLY’ earlier (not surprisingly).

Ludo’.

bug#61079: [3.0.9] ‘system*’ broken on GNU/Hurd

[Ludovic Courtès]

This test fails on GNU/Hurd:

--8<---cut here---start->8---
checking for library containing posix_spawn... none required
checking whether posix_spawn is declared... yes
checking for posix_spawn... yes
checking whether posix_spawn is declared... (cached) yes
checking whether posix_spawn works... yes
checking whether posix_spawn rejects scripts without shebang... yes
checking whether posix_spawnp rejects scripts without shebang... yes
checking whether posix_spawnattr_setschedpolicy is supported... yes
checking whether posix_spawnattr_setschedparam is supported... yes
checking for mbstate_t... yes
checking for ssize_t... yes
checking for sched.h... yes
checking for struct sched_param... yes
checking whether  is self-contained... yes
checking whether setenv is declared... yes
checking for search.h... yes
checking for tsearch... yes
checking for sigset_t... yes
checking for uid_t in sys/types.h... yes
checking for volatile sig_atomic_t... yes
checking for sighandler_t... yes
checking for posix_spawnattr_t... yes
checking for posix_spawn_file_actions_t... yes

[…]

make[5]: Entering directory 
'/tmp/guix-build-guile-3.0.9rc1.drv-0/guile-3.0.9rc1/test-suite/standalone'
In execvp of guile: Bad file descriptor
test-system-cmds: system* exit status was 127 rather than 42
FAIL: test-system-cmds
--8<---cut here---end--->8---

This looks like a bug in the new ‘posix_spawn’-based ‘system*’, or (more
likely) in the ‘posix_spawn’ implementation for the Hurd in glibc.

Ludo’.

bug#61073: ‘spawn’ crashes when passed a non-file port

[Ludovic Courtès]

Ludovic Courtès  skribis:

> Here’s an example:
>
> $ ./meta/guile
> GNU Guile 3.0.9
> Copyright (C) 1995-2023 Free Software Foundation, Inc.
>
> Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
> This program is free software, and you are welcome to redistribute it
> under certain conditions; type `,show c' for details.
>
> Enter `,help' for help.
> scheme@(guile-user)> (with-error-to-port (%make-void-port "w") (lambda () 
> (spawn "date" (list "date"
> Segmentation fault
>
> This is due to the careless use of ‘SCM_FPORT_FDES’ there.

Fixed in 5b42f8c154906584455a4989038406c88b723cb0.

Ludo’.

bug#61073: ‘spawn’ crashes when passed a non-file port

[Ludovic Courtès]

Here’s an example:

--8<---cut here---start->8---
$ ./meta/guile
GNU Guile 3.0.9
Copyright (C) 1995-2023 Free Software Foundation, Inc.

Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
This program is free software, and you are welcome to redistribute it
under certain conditions; type `,show c' for details.

Enter `,help' for help.
scheme@(guile-user)> (with-error-to-port (%make-void-port "w") (lambda () 
(spawn "date" (list "date"
Segmentation fault
--8<---cut here---end--->8---

This is due to the careless use of ‘SCM_FPORT_FDES’ there.

Ludo’.

bug#60971: build failure of v3.0.9rc1 on mac os 12.6

[Ludovic Courtès]

lloda  skribis:

> lgtm, no other issues on mac os.

Awesome, pushed as 9b20ca275dba758a194073936cde7c95311bd51e.

Ludo’.

bug#60234: Build failure on mac os 12.6 / gcc 12.2

[Ludovic Courtès]

Hi Daniel,

lloda  skribis:

> .../libguile/threads.h:194:43: error: 'scm_i_current_thread' is defined with 
> tls model global-dynamic
>   194 | SCM_INTERNAL SCM_THREAD_LOCAL scm_thread *scm_i_current_thread;
>   |   ^
> .../libguile/threads.c:357:30: note: previously defined here as local-dynamic
>   357 | SCM_THREAD_LOCAL scm_thread *scm_i_current_thread = NULL;
>
> Simply repeating SCM_INTERNAL in the .c fixes it...

The problem is that ‘SCM_INTERNAL’ is synonymous with ‘extern’, which
makes no sense for a definition (threads.c:357), so rightfully GCC
GNU/Linux rightfully complains:

--8<---cut here---start->8---
  CC   libguile_3.0_la-threads.lo
threads.c:358:43: warning: 'scm_i_current_thread' initialized and declared 
'extern'
  358 | SCM_INTERNAL SCM_THREAD_LOCAL scm_thread *scm_i_current_thread = NULL;
  |   ^~~~
--8<---cut here---end--->8---

It’s just a warning, but still not looking good.

Is there something else at play, such as a ‘-ftls-model’ flag being
passed to GCC somehow (info “(gcc) Code Gen Options")?

If not, should we have:

  #define SCM_THREAD_LOCAL \
__thread __attribute__ ((__tls_model__ ("global-dynamic")))

instead (info "(gcc) Common Variable Attributes")?

Would that work with Clang?

Ludo’.

bug#59647: [PATCH] Fix typos in docstrings.

[Ludovic Courtès]

jgart  skribis:

> * libguile/boolean.c
> (scm_not): Fix typo.
> (scm_boolean_p): Fix typo.
> ---
>  libguile/boolean.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libguile/boolean.c b/libguile/boolean.c
> index e8370331f..930001a89 100644
> --- a/libguile/boolean.c
> +++ b/libguile/boolean.c
> @@ -56,7 +56,7 @@ verify (SCM_BITS_DIFFER_IN_EXACTLY_TWO_BIT_POSITIONS
> \
>  
>  SCM_DEFINE (scm_not, "not", 1, 0, 0, 
>  (SCM x),
> -"Return @code{#t} iff @var{x} is false, else return @code{#f}.")
> +"Return @code{#t} if @var{x} is false, else return @code{#f}.")

Hi!  These are not typos: “iff” is short for “if and only if”.

Thanks,
Ludo’.

bug#60022: configure.ac fix for C99 compatibility

[Ludovic Courtès]

Hi Florian,

Florian Weimer  skribis:

> This patch is needed to improve C99 compatibility:
>
> diff --git a/configure.ac b/configure.ac
> index b3879df1f..cc865a028 100644
> --- a/configure.ac
> +++ b/configure.ac

Applied, thanks.

Ludo’.

bug#60487: string-ref segfaults with n < 0 on Guile 3.0.8

[Ludovic Courtès]

Hi,

fester...@posteo.net skribis:

> The following code results in a segmentation fault on Guile
> 3.0.8-deb+3.0.8-2 (obtained from the Debian repositories):
> (string-ref "my string" -3)

I can reproduce it with 3.0.8, where I get this backtrace:

--8<---cut here---start->8---
scheme@(guile-user)> (string-ref "my string" -3)

Thread 1 "guile" received signal SIGSEGV, Segmentation fault.
0x77f419d9 in scm_is_values (x=) at values.h:30
30  values.h: No such file or directory.
(gdb) bt
#0  0x77f419d9 in scm_is_values (x=) at values.h:30
#1  vm_debug_engine (thread=0x775c1d80) at vm-engine.c:974
#2  0x77f4c5d9 in scm_call_n (proc=, argv=, nargs=5)
at vm.c:1610
#3  0x77eb8571 in scm_apply_0 (proc=#, args=()) 
at eval.c:603
#4  0x77f3dc8d in scm_throw (key=out-of-range, 
args=0x72bb2c30)
at throw.c:262
#5  0x77f3dca9 in scm_ithrow (key=, args=, 
no_return=) at throw.c:457
#6  0x77eb5245 in scm_error_scm (key=key@entry=out-of-range, 
subr=, 
message=message@entry="Value out of range ~S to< ~S: ~S", 
args=args@entry=0x72bb2c70, data=data@entry=(4611686018427387901)) at error.c:90
#7  0x77eb52a0 in scm_error (key=out-of-range, subr=0x0, 
message=, 
args=0x72bb2c70, 
rest=(4611686018427387901)) at error.c:62
#8  0x77f02dd7 in range_error 
(bad_val=bad_val@entry=4611686018427387901, 
min=min@entry=0x0, 
max=#) at numbers.c:6611
#9  0x77f04dfb in scm_to_uint64 (arg=4611686018427387901) at 
integers.c:259
#10 0x77f42215 in vm_debug_engine (thread=0x775c1d80) at 
vm-engine.c:1533
#11 0x77f4c5d9 in scm_call_n (proc=, argv=, nargs=1)
at vm.c:1610
#12 0x77eb4457 in scm_primitive_eval (exp=, 
exp@entry=((@ (ice-9 control) %) (begin (load-user-init) ((@ (ice-9 
top-repl) top-repl)
at eval.c:671
#13 0x77eba4b6 in scm_eval (
exp=((@ (ice-9 control) %) (begin (load-user-init) ((@ (ice-9 top-repl) 
top-repl, 
module_or_state="#" = {...}) at eval.c:705
#14 0x77f1e3b6 in scm_shell (argc=1, argv=0x7fffd058) at 
script.c:357
--8<---cut here---end--->8---

Fortunately, this was fixed recently in
c0004442b7691f59a0e37869ef288eb26382ad9e.

Thanks!

Ludo’.

bug#60522: make-vector takes 100% cpu if called without argument in the REPL

[Ludovic Courtès]

Hi,

Sascha Ziemann  skribis:

> The following throws an error:
> guile -c '(make-vector)'
>
> But the evaluation of '(make-vector)' in the REPL generats just a warning:
>
> ;;; :1:0: warning: possibly wrong number of arguments to `make-vector'
>
> and seems to enter an endless loop afterwards.

The guts of the problem is an endless loop while reducing primitives:

--8<---cut here---start->8---
scheme@(language tree-il primitives)> (make-call #f (make-primitive-ref #f 
'make-vector) '())
$21 = #
scheme@(language tree-il primitives)> (resolve-primitives $21 (current-module))
$22 = #
scheme@(language tree-il primitives)> (expand-primcall $22)
$23 = #
--8<---cut here---end--->8---

This is fixed in 51152392ef04b053e3c7b2576473df2df9d08fe0:

--8<---cut here---start->8---
scheme@(language tree-il primitives)> (expand-primcall $22)
$32 = #
--8<---cut here---end--->8---

Thanks!

Ludo’.

bug#59321: ice-9's open-input-pipe is unexpectedly slow on some systems

[Ludovic Courtès]

Hello!

Andrew Whatson  skribis:

> commit c012d7b0d5248a99a3a92780687a676c5d420f5f
> Author: Andrew Whatson 
> Date:   Thu Dec 8 21:43:28 2022 +1000
>
> Reduce redundant close() calls when forking on some systems.
> 
> Some systems provide "/proc/self/fd" which is a directory containing an
> entry for each open file descriptor in the current process.  We use this
> to limit the number of close() calls needed to ensure file descriptors
> aren't leaked to the child process when forking.
> 
> * libguile/posix.c (close_inherited_fds_slow):
> (close_inherited_fds): New static helper functions.
> (scm_spawn_process): Attempt to close inherited file descriptors
> efficiently using 'close_inherited_fds', falling back to the brute-force
> approach in 'close_inherited_fds_slow'.

Finally applied on top of the ‘posix_spawn’ series as commit
9332b632407894c2e1951cce1bc678f19e1fa8e4.

Thanks!

Ludo’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Hi Andrew & Josselin,

Andrew Whatson  skribis:

> Thanks for your work on this, posix_spawn is a nice improvement!
>
> My comments on the changes in the wip-posix-spawn branch follow.

As discussed on IRC, I took your comments into account.  I also added a
‘system*’ test based on what Josselin provided at the beginning of this
bug report (let’s not forget we were initially fixing an actual bug :-))
and updated ‘NEWS’:

  527c257d6 Make 'system*' and 'piped-process' internally use 'spawn'.
  551929e4f Add 'spawn'.
  edfca3b7e Update gnulib to 0.1.5414-8204d and add posix_spawn, posix_spawnp.

Let me know if you notice anything fishy!

Thanks a lot, Josselin, for your hard work and for your patience.

Ludo’.

bug#60779: ‘SCM_F_BYTEVECTOR_IMMUTABLE’ is not honored by bytevector instructions

[Ludovic Courtès]

Bytevector literals are marked as ‘SCM_F_BYTEVECTOR_IMMUTABLE’, but VM
instructions that access bytevectors do not check that flag, which can
lead to segfaults:

--8<---cut here---start->8---
$ cat t.scm
(use-modules (rnrs bytevectors))

(define bv #vu8(1 2 3))
(bytevector-u8-set! bv 0 1)
$ guild compile -O2 t.scm
wrote `/home/ludo/.cache/guile/ccache/3.0-LE-8-4.6/data/src/guile-3.0/t.scm.go'
$ guile t.scm
Segmentation fault
--8<---cut here---end--->8---

Conversely, the subrs from bytevector.c do check the flag:

--8<---cut here---start->8---
$ guild compile -O0 t.scm
wrote `/home/ludo/.cache/guile/ccache/3.0-LE-8-4.6/data/src/guile-3.0/t.scm.go'
$ guile t.scm
Backtrace:
In ice-9/boot-9.scm:
  1752:10  6 (with-exception-handler _ _ #:unwind? _ # _)
In unknown file:
   5 (apply-smob/0 #)
In ice-9/boot-9.scm:
724:2  4 (call-with-prompt ("prompt") # …)
In ice-9/eval.scm:
619:8  3 (_ #(#(#)))
In ice-9/boot-9.scm:
   2836:4  2 (save-module-excursion #)
  4388:12  1 (_)
In unknown file:
   0 (bytevector-u8-set! #vu8(1 2 3) 0 1)

ERROR: In procedure bytevector-u8-set!:
In procedure bytevector-u8-set!: Wrong type argument in position 1 (expecting 
mutable bytevector): #vu8(1 2 3)
$ guile --version
guile (GNU Guile) 3.0.8
Copyright (C) 2021 Free Software Foundation, Inc.

License LGPLv3+: GNU LGPL 3 or later .
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
--8<---cut here---end--->8---

I suppose the immutability test would have to be done not at the level
of individual VM instructions but at a higher-level in generated code,
such that the compiler could lift the test outside loops that access
bytevectors, similar to what it does with ‘heap-object?’.

Ludo’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Hi Josselin, and a happy new year full of good hacks!  :-)

Josselin Poiret  skribis:

> I've done the above by using the C functions to bind keyword arguments,
> and added the #:use-path? keyword argument.  One annoying thing though
> is that since it uses keyword arguments, the first line of the
> documentation generated by SCM_DEFINE only mentions "spawn program
> . keyword_args".

That’s OK.

> I've opted to put it right below primitive-fork, and slightly rewrite
> the part about pipes in primitive-fork's description to also mention
> spawn.  Let me know if the documentation is not descriptive enough.

It looks good to me.

A few suggestions of mine got lost, so I pushed a new ‘wip-posix-spawn’
with additional commits taking those into account and adding tests.  The
changes in this v8.1 are:

  - make 'arguments' positional rather than keyword
  - rename keyword arguments to avoid abbreviations
  - add example in the manual
  - mark 'scm_spawn_process' as SCM_INTERNAL
  - add tests

Because this is now a core binding and no longer in (ice-9 spawn), I
hope ‘spawn’ is not clashing with someone else’s library, though the
worst that could happen is a run-time warning “module X overrides core
binding 'spawn'”.

I’m very pleased with the branch and could finally merge if you agree!
(I’ll put myself as co-author and take the blame for half of the bugs.
:-))

Ah yes, one thing that came to mind: we could change #:input #f to mean
“close file descriptor 0”, and so on.  Let’s first merge this, though.

Thoughts?

Ludo’.

bug#57948: [PATCH] Avoid 'frame-local-ref' errors when printing backtrace.

[Ludovic Courtès]

Hi,

Andrew Whatson  skribis:

> commit 164bdce6acf53796cb96ef1930a89c6caf84bc39
> Author: Andrew Whatson 
> Date:   Wed Jan 11 14:04:32 2023 +1000
>
> Test for 'frame-local-ref' errors when printing backtrace.
> 
> This test reproduces the error from , and
> passes with the workaround which was merged in commit
> c7fa78fc751eb336bcfafbb5ac59c460ee2c5d7a.
> 
> * test-suite/tests/eval.test ("avoid frame-local-ref out of range"): New
> test.

Applied, thanks!

Ludo’.

PS: Please use ‘git format-patch’ so the patch can be directly consumed
by ‘git am’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Josselin Poiret  skribis:

> * libguile/posix.c (scm_system_star, scm_piped_process): Use do_spawn.
> (start_child): Remove function.

LGTM, thanks!

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Josselin Poiret  skribis:

> * libguile/posix.c: Include spawn.h from Gnulib.
> (do_spawn, scm_spawn_process): New functions.
> * module/ice-9/spawn.scm: New file
> (spawn): New procedure.
> ---
>  libguile/posix.c   | 82 ++
>  libguile/posix.h   |  2 ++
>  module/ice-9/spawn.scm | 54 
>  3 files changed, 138 insertions(+)

The new module should be added to ‘am/bootstrap.am’.

> +SCM_API SCM scm_spawn_process (SCM prog, SCM args, SCM env,
> +   SCM in, SCM out, SCM err);

Let’s keep it ‘SCM_INTERNAL’.

> +(define* (spawn exec-file
> +#:optional (args (list exec-file))
> +#:key  (env (environ))
> +   (in (current-input-port))
> +   (out (current-output-port))
> +   (err (current-error-port)))

s/exec-file/program/
s/args/arguments/
s/env/environment/

s/in/standard-input/
s/out/standard-output/
s/err/standard-error/

Maybe we could allow these two be either ports or file descriptors?

> +  "Spawns a new process running the program @var{exec} with arguments
> +@var{args}, in the environment specified by the list of environment
> +variable strings @var{env}, and with standard input, output and error
> +set to the ports specified by @var{in}, @var{out}, @var{err}.  Note that
> +the last part only works with fd-backed ports."
> +  (let* ((in (port-with-defaults in "r"))
> + (out (port-with-defaults out "w"))
> + (err (port-with-defaults err "w"))
> + ;; Increment port revealed counts while to prevent ports GC'ing and
> + ;; closing the associated fds while we spawn the process.
> + (result (spawn* exec-file
> + args
> + env
> + (port->fdes in)
> + (port->fdes out)
> + (port->fdes err

I believe ‘spawn*’ is unbound here because it’s defined by
‘scm_init_popen’, which is called within the (ice-9 popen) module.

Ludo’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Hi Josselin,

Josselin Poiret  skribis:

> I've added a convenience module (ice-9 spawn) with a `spawn` procedure
> in it, which takes an optional argument list which defaults to just the
> executable, and optional environment variables as well as in, out and
> err ports.  I also think everything in (ice-9 popen) should be migrated
> on the next major release, as well as being re-implemented in terms of
> `spawn` purely, so that the change is immediately noticeable.

OK.

> We're reaching the bike-shedding time now, but IMHO having such a
> `spawn*` exposed to the user seems fine, it's a pretty simple "raw"
> interface with fdes, and there is a convenience `spawn` function that is
> nicer for users.

‘spawn*’ is expressive enough, but a procedure with 6 positional
arguments cannot be exposed as-is IMO.

One tiny thing that’s missing is a boolean to choose between
‘posix_spawn’ and ‘posix_spawnp’.

So we need either ‘spawn’ defined in Scheme as you did, or directly use
‘scm_c_bind_keyword_arguments’ right in ‘scm_spawn_process’ (info
"(guile) Keyword Procedures"), to avoid ending up with two procedures.
I’m fine either way.

> Do we need to add a documentation page as well?

Yes, please.  :-)  I realize I’m asking a lot of things, so let me know
if you’d like to share the workload somehow.

Doc could go in the “Processes” section, or possibly in a new section
we’d add right after it, “Spawning Programs”.  We should probably add a
“@quotation Note” in the doc of ‘primitive-fork’ that briefly explains
why people should probably use ‘spawn’ rather than fork + exec.

Some cosmetic suggestions follow…

Thanks,
Ludo’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Hi Josselin,

Josselin Poiret  skribis:

> Here is hopefully the last reroll of this patchset.  First of all, I did not
> include the gnulib patch again because it still applies cleanly and it is
> extremely large, but it should be applied before those 3 patches.

Yay!

> The first two patches should be applied on the current major release, while 
> the
> third one should be applied on the next major release to finish the migration 
> to
> spawn.

I pushed it to ‘wip-posix-spawn’ along with fixups I’m proposing, mostly
along the lines of what I suggested in
:

  • Avoiding the extra ‘fork’ in ‘system*’ upon error;

  • Keep ‘scm_spawn_process’ internal.

I also added Andrew Whatson’s patch from
.

If that’s fine with you, I can squash the “fixup!” commits and merge the
branch.  Let me know!

Earlier we agreed it’d be nice to expose ‘spawn*’/‘primitive-spawn’.  I
still think it’s a good idea, but the interface would need some work IMO
to be more generally useful.  In essence, we could provide something
similar to ‘fork+exec-command’ in the Shepherd, where one can pass
environment variables, stdin/stdout/stderr, etc., all that with keyword
arguments and reasonable defaults.

It’s a bit of extra work though so we can discuss that later,
separately.

Thank you!

Ludo’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Hello,

Josselin Poiret  skribis:

>> I would call this one ‘primitive-spawn’ rather than ‘spawn*’ and keep it
>> private to (ice-9 popen).
>
> Is there any reason we would want this to not be accessible to the user?
> There are already a bunch of functions that manipulate raw fdes, and
> people might want to directly use this to not have to care about ports.

Yeah, on second thought I think you’re right: it be can be useful to
have it exposed to users.

In fact, I think we should provide interfaces that make ‘primitive-fork’
unnecessary for most use cases.  Exposing that procedure is a step in
that direction.

>> We could even avoid allocating a port when we’re going to use /dev/null
>> (and thus go with ‘open-fdes’ directly), but that’s a micro-optimization
>> we can keep for later.
>
> Right.  I chose to keep the code simple for now, it's too much trouble
> having to choose between using ports and fdes.  Note that I did a small
> benchmark and system* with PATCH v5 is 3x faster than on 3.0.8.  vfork
> is working wonders.

Nice!

>>> +++ b/test-suite/tests/posix.test
>>> @@ -236,24 +236,24 @@
>>>  
>>>  (with-test-prefix "system*"
>>>  
>>> -  (pass-if "http://bugs.gnu.org/13166;
>>> -;; With Guile up to 2.0.7 included, the child process launched by
>>> -;; `system*' would remain alive after an `execvp' failure.
>>> -(let ((me (getpid)))
>>> -  (and (not (zero? (system* "something-that-does-not-exist")))
>>> -   (= me (getpid)
>>
>> I’d keep this one, I guess it doesn’t hurt?
>
> As is, it doesn't work since system* would throw a system exception
> because spawn is able to catch that error.  Previously, the child would
> fail its execvp and die with exit code 127, which system* would return.
>
>>> -  (pass-if-equal "exit code for nonexistent file"
>>> -  127 ;aka. EX_NOTFOUND
>>> -(status:exit-val (system* "something-that-does-not-exist")))
>>
>> It’s good that we have better error reporting thanks to ‘posix_spawn’.
>>
>> However, I don’t think we can change that in 3.0.  What about, for 3.0,
>> adding a layer around ‘spawn’ so that ‘system*’ still returns 127 when
>> ‘spawn’ throws to ‘system-error’?
>
> So I've been working on something that would do this, but I noticed that
> we have no way to be strictly backwards-compatible: if there's an error
> like ENOFILE, we can't get a pid from posix_spawn, and so piped-process
> won't have anything to return, whereas before it would return the pid of
> the failing child.  I'm not sure there's a way to emulate that, unless
> we just fork a child that instantly returns 127.  Doesn't seem great
> though.

Right now ‘system*’ does:

  pid = scm_spawn_process (prog, args, in, out, err);
  SCM_SYSCALL (wait_result = waitpid (scm_to_int (pid), , 0));
  if (wait_result == -1)
SCM_SYSERROR;

How about introducing decomposing ‘scm_spawn_process’ so that we have a
lower-level function we could use (‘spawn_process’ below), roughly like
so:

  ret = spawn_process (proc, args, in, out, err, );
  if (ret != 0)
{
  if (ret == ENOMEM)
{
  errno = ENOMEM;
  SCM_SYSERROR;
}
  else
/* Emulate old-style failure.  TODO: In 3.2, turn that into an
   exception */
status = 127 << 8;
}
  else
SCM_SYSCALL (wait_result = waitpid (scm_to_int (pid), , 0));

Does that make sense?

It’s a bit of work to emulate that suboptimal behavior, but I think it’s
important not to change that in 3.0.

Thanks for your feedback!

Ludo’.

bug#59540: Calling length on a very long improper list is disastrous.

[Ludovic Courtès]

Hi,

lloda  skribis:

> I think excessive output is a more serious problem, because it should be 
> possible to go to a backtrace frame and look at objects directly. On the 
> other hand, it should also be possible to C-c when guile starts to flood the 
> terminal. But neither of these workarounds is reliable :-/ Ultimately this 
> printing should be configurable.
>
> We already have the repl-option system (repl-option-set! repl 'print ...). 
> This system doesn't apply to exception messages nor backtraces. I think if it 
> did, that would mostly solve the problem.

Yes.  Right now it’s just $COLUMNS; we should have a documented SRFI-39
parameter for that.

> I also think that, besides options to truncate or not, we should have a pager 
> (display at most a page, give options to next/stop/all). That would be the 
> best default.

Yes!

Ludo’.

bug#59540: Calling length on a very long improper list is disastrous.

[Ludovic Courtès]

Hi Jeremy,

Jeremy Phelps  skribis:

> I lost my Emacs session today because I accidentally called the length
> function on an extremely long improper list.
>
> But Guile prints the entire thing when it reports the error that happens
> when the length function gets to the improper part of the list. It tried to
> print a few million elements just to tell me which list wasn't a proper
> list.
>
> To reproduce the error should be easy:
>
> (length
>
>(let loop ((result 'x)
>
>   (n 5000))
>
>  (if (= n 0)
>
>  result
>
>  (loop (cons n result) (- n 1)
>
>
> Emacs is more sensitive to the problem because it doesn't throw away any of
> the output. It's also really bad over SSH. The above test is enough to dump
> output to a local terminal for a minute or two, or to uninterruptibly tie
> up an SSH session for several minutes.

I think there are several issues here:

  1. ‘length’ takes time linear to the size of the list (for
 non-circular lists).  That’s how it’s specified in the Scheme
 reports.

  2. Guile doesn’t truncate arguments that come after the “Wrong type
 argument” error message.

  3. Emacs poorly handles very long lines, to put it mildly.

Of these only #2 is something we could work on.  However, truncation has
proven to be a hindrance sometimes (in backtraces, objects are
automatically), so I’m not sure we want to enable it by default on
wrong-type-arg error messages.

Thoughts?

Ludo’.

bug#59321: ice-9's open-input-pipe is unexpectedly slow on some systems

[Ludovic Courtès]

Hi,

Andrew Whatson  skribis:

> Playing with the wip-posix-spawn branch, it has the same slowdown
> (actually a bit worse).  I've updated the "/proc/self/fd" fast-path
> patch for posix_spawn, please find attached.

Ooh that’s perfect, thank you.

I’ll apply it once we’ve sorted out remaining issues on the branch.

Ludo’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Hi Josselin,

Sorry for taking so long to come back to you.  I think this is great
work!  I pushed it as ‘wip-posix-spawn’ so others can give it a try.

Josselin Poiret  skribis:

> * libguile/posix.c (renumber_file_descriptor, start_child,
> scm_piped_process): Remove functions.
> (scm_port_to_fd_with_default): New helper function.
> (scm_system_star): Rewrite using scm_spawn_process.
> (scm_init_popen): Remove the definition of piped-process.
> (scm_init_posix): Now make popen available unconditionally.
>
> * module/ice-9/popen.scm (port-with-defaults): New helper procedure.
> (spawn): New procedure.
> (open-process): Rewrite using spawn.
> (pipeline): Rewrite using spawn*.
>
> * test-suite/tests/popen.test ("piped-process", "piped-process:
> with-output"): Removed tests.
> ("spawn", "spawn: with output"): Added tests.
> * test-suite/tests/posix.test ("http://bugs.gnu.org/13166;, "exit code
> for nonexistent file", "https://bugs.gnu.org/55596;): Remove obsolete
> tests.
> ("exception for nonexistent file"): Add test.
> ---
>  libguile/posix.c| 218 +++-
>  module/ice-9/popen.scm  |  83 ++
>  test-suite/tests/popen.test |  14 +--
>  test-suite/tests/posix.test |  36 +++---
>  4 files changed, 102 insertions(+), 249 deletions(-)

More deletions than insertions. 

That scary-looking Gnulib update seems to have worked well.

I have mostly cosmetic/polishing comments and one issue with ‘system*’.
I can actually do that on your behalf if you’re unavailable these days;
let me know what you prefer.

>  static SCM
>  scm_spawn_process (SCM prog, SCM args, SCM scm_in, SCM scm_out, SCM scm_err)
>  #define FUNC_NAME "spawn*"

For top-level functions, please add a comment above explaining what it
does.

I would call this one ‘primitive-spawn’ rather than ‘spawn*’ and keep it
private to (ice-9 popen).

> +(define* (spawn exec-file argv #:key
> + (in (current-input-port))
> + (out (current-output-port))
> + (err (current-error-port)))

Please add a docstring.  It may also be worth documenting it in the
manual given that it’s public.

> +  (let* ((in (port-with-defaults in "r"))
> + (out (port-with-defaults out "w"))
> + (err (port-with-defaults err "w"))

I’d make it “r0” and “w0” since we’re doing to throw the ports away
right after.

We could even avoid allocating a port when we’re going to use /dev/null
(and thus go with ‘open-fdes’ directly), but that’s a micro-optimization
we can keep for later.

> +++ b/test-suite/tests/posix.test
> @@ -236,24 +236,24 @@
>  
>  (with-test-prefix "system*"
>  
> -  (pass-if "http://bugs.gnu.org/13166;
> -;; With Guile up to 2.0.7 included, the child process launched by
> -;; `system*' would remain alive after an `execvp' failure.
> -(let ((me (getpid)))
> -  (and (not (zero? (system* "something-that-does-not-exist")))
> -   (= me (getpid)

I’d keep this one, I guess it doesn’t hurt?

> -  (pass-if-equal "exit code for nonexistent file"
> -  127 ;aka. EX_NOTFOUND
> -(status:exit-val (system* "something-that-does-not-exist")))

It’s good that we have better error reporting thanks to ‘posix_spawn’.

However, I don’t think we can change that in 3.0.  What about, for 3.0,
adding a layer around ‘spawn’ so that ‘system*’ still returns 127 when
‘spawn’ throws to ‘system-error’?

> -  (pass-if-equal "https://bugs.gnu.org/55596;
> -  127
> -;; The parameterization below used to cause 'start_child' to close
> -;; fd 2 in the child process, which in turn would cause it to
> -;; segfault, leading to a wrong exit code.
> -(parameterize ((current-output-port (current-error-port)))
> -  (status:exit-val (system* "something-that-does-not-exist")

Likewise we should keep this one.

> +  (pass-if-equal "exception for nonexistent file"
> +  2 ; ENOENT
> +  (call-with-prompt 'escape
> +(lambda ()
> +  (with-exception-handler
> +  (lambda (exn)
> +(let* ((kind (exception-kind exn))
> +   (errno (and (eq? kind 'system-error)
> +   (car (car
> + (cdr (cdr (cdr (exception-args
> +exn)
> +  (abort-to-prompt 'escape errno)))
> +(lambda ()
> +  (status:exit-val (system*
> +"something-that-does-not-exist")))
> +#:unwind? #t))
> +(lambda (k arg)
> +  arg

We’ll have to leave this change for the next major series of Guile.

Thanks!

Ludo’.

bug#59321: ice-9's open-input-pipe is unexpectedly slow on some systems

[Ludovic Courtès]

Hi Andrew,

Andrew Whatson  skribis:

> Ludovic Courtès  wrote:

[...]

>> Libguile opens all its own file descriptors at O_CLOEXEC (one omission
>> was recently fixed in 0aa1a9976fc3c6af4d1087e59d728cb8fe7d369a) so it
>> may be possible to remove that FD-closing loop.  There’s still the
>> possibility that application bug unwillingly leaks FDs, but we could
>> consider it’s none of our business.
>>
>> Thoughts?
>
> I agree with this approach in principle, but from what Tomas is saying
> it seems like it's not currently possible for applications to do the
> right thing in all cases.

OK.

[...]

> We also need equivalent functionality around SOCK_CLOEXEC.  It seems
> this is implemented for ‘accept’, but not ‘socket’ or ‘socketpair’.

It’s possible to use SOCK_CLOEXEC with ‘socket’ and ‘socketpair’
already, as in:

  (socket AF_INET (logior SOCK_CLOEXEC SOCK_STREAM) 0)

With commit 1d313bf5f0d296d766bd3a0e6d030df37c71711b, ‘pipe’ is also
covered.

So I think we have pretty much everything we need, at least starting
with 3.0.9.

> Python's PEP 433 contains a good explanation of the issues which can
> arise from leaked file descriptors:
> https://peps.python.org/pep-0433/#inherited-file-descriptors-issues
>
> Given the risks, I'm convinced that Guile's conservative approach is
> actually quite sensible.  It seems like the best path forward would be
> to implement platform-specific optimizations where possible.
>
> I've attached a draft patch which implements a fast-path on systems
> where "/proc/self/fd" is available.

The patch LGTM; it’s certainly an improvement on systems configured with
a high per-process FD limit.

Now, I believe use of ‘posix_spawn’ as proposed in
<https://issues.guix.gnu.org/52835> makes that unnecessary.  Let’s take
a closer look at that other patch and so we can see…

Thanks,
Ludo’.

bug#59021: Unbounded heap growth when combining dynamic states & delimited continuation

[Ludovic Courtès]

Hi,

Ludovic Courtès  skribis:

> Ludovic Courtès  skribis:
>
>> Consider this code:
>>
>> ;; https://issues.guix.gnu.org/58631
>> ;; https://github.com/wingo/fibers/issues/65
>>
>> (define loss
>>   (make-vector 100))
>>
>> (let ((tag (make-prompt-tag "my prompt")))
>>   (define handler
>> (lambda (k i)
>>   (when (zero? (modulo i 200))
>> (pk 'heap-size (assoc-ref (gc-stats) 'heap-size)))
>>
>>   (call-with-prompt tag
>> (lambda ()
>>   (k (modulo (+ 1 i) 1000)))
>> handler)))
>>
>>   (call-with-prompt tag
>> (let ((state (current-dynamic-state)))
>>   (lambda ()
>> ;; (define (with-dynamic-state state thunk)
>> ;;   (let ((previous #f))
>> ;; (dynamic-wind
>> ;;   (lambda () (set! previous (set-current-dynamic-state 
>> state)))
>> ;;   thunk
>> ;;   (lambda () (set-current-dynamic-state previous)
>> (with-dynamic-state state
>> (lambda ()
>>   (let loop ((i 0))
>> (loop (abort-to-prompt tag i)))
>> handler))
>>
>> On Guile 3.0.8, this program exhibits seemingly unbounded heap growth.
>
> This is fixed by the patch below (tested against the test case above and
> the Fibers and Shepherd test cases mentioned before):

Pushed as e47a153317c046ea5d335940412999e7dc604c33.

> Using a simple heap profiler (more on that later), I noticed that the
> stacks allocated at ‘p->stack_bottom’ would be partly retained,
> explaining the heap growth.
>
> I couldn’t pinpoint what exactly is keeping a pointer to the stack, but
> what I can tell is that the trick above makes that impossible (because
> we disable interior pointer tracing), hence the difference.
>
> Also, why changing the SCM_DYNSTACK_TYPE_DYNAMIC_STATE entry to an
> SCM_DYNSTACK_TYPE_UNWINDER entry would make a difference remains a
> mystery to me.
>
> I’m interested in theories that would explain all this in more detail!
> I’ll go ahead with the fix above if there are no objections.

I still am.  :-)

Ludo’.

bug#59321: ice-9's open-input-pipe is unexpectedly slow on some systems

[Ludovic Courtès]

Hi,

Andrew Whatson  skribis:

> Forcibly closing file descriptors like this shouldn't be necessary if
> the application has properly opened descriptors with the FD_CLOEXEC
> flag.  It would be good to get input from some more experienced Guile
> hackers on the potential consequences of this change.

Libguile opens all its own file descriptors at O_CLOEXEC (one omission
was recently fixed in 0aa1a9976fc3c6af4d1087e59d728cb8fe7d369a) so it
may be possible to remove that FD-closing loop.  There’s still the
possibility that application bug unwillingly leaks FDs, but we could
consider it’s none of our business.

Thoughts?

Similarly, with commit a356ceebee000efe91a2a16dbcaa64d6c6a3a922, it’s
possible to pass ‘open-file’ a flag that corresponds to O_CLOEXEC, which
wasn’t possible before.  I’ve also been thinking that files opened with
‘call-with-*’ should be O_CLOEXEC.  That’d be an incompatible change
though, so maybe not something for 3.0.x.

Ludo’.

bug#59055: [PATCH] Fix possible deadlock.

[Ludovic Courtès]

Hi,

Olivier Dion  skribis:

> If we got interrupted while waiting on our condition variable, we unlock
> the kernel mutex momentarily while executing asynchronous operations
> before putting us back into the waiting queue.
>
> However, we have to retry acquiring the mutex before getting back into
> the queue, otherwise it's possible that we wait indefinitely since
> nobody could be the owner for a while.
>
> * libguile/threads.c (lock_mutex): Try acquring the mutex after signal
> interruption.

Looks reasonable to me; applied.

Did you try to come up with a reproducer?  That would be awesome but I
guess it’s hard because you need to trigger EINTR at the right point.

Thanks,
Ludo’.

bug#56413: [PATCH 1/1] scm_i_utf8_string_hash: compute u8 chars not bytes

[Ludovic Courtès]

Hi,

Rob Browning  skribis:

> Oh, and unless I'm missing something, I remembered why we may need to
> keep the standalone C test program -- there's no straightforward way to
> call scm_from_utf8_symbol() from scheme?

Ah yes, you’re probably right!

Ludo’.

bug#56413: [PATCH 1/1] scm_i_utf8_string_hash: compute u8 chars not bytes

[Ludovic Courtès]

Rob Browning  skribis:

> So this change *could* alter results, but only for non-ASCII strings,
> and those results would have been wrong (i.e. relying on uninitialized
> memory).

OK, that was my understanding too.

> That leaves the size_t -> long change in scm_i_str2symbol(), and I don't
> think that has anything to do with UTF-8, but it could cause mangling of
> the value on any platform where the data types differ sufficiently, and
> then of course if we're not using the same type consistently, then we
> could give different answers for the same symbol in different contexts
> (for different code paths).

Right.  This one looks safe to me.

> And indeed, looks like I missed another case; just below in
> scm_i_str2uninterned_symbol() we also use size_t.  For now, I suspect we
> should change both or neither, and definitely change them all to match
> "eventually".

Sure.

Thanks!

Ludo’.

bug#56413: [PATCH 1/1] scm_i_utf8_string_hash: compute u8 chars not bytes

[Ludovic Courtès]

Rob Browning  skribis:

>> Is this a documented example of Jenkins?  Or did you use a reference
>> implementation?
>
> Jenkins?

That’s the name of the hash function in question.

If not, where did you get that example from?  :-)

Thanks,
Ludo’.

bug#56413: [PATCH 1/1] scm_i_utf8_string_hash: compute u8 chars not bytes

[Ludovic Courtès]

Hi,

Rob Browning  skribis:

> Noticed while investigating a migration to utf-8 strings.  After making
> changes that routed non-ascii symbol hashing through this function,
> encoding-iso88597.test began intermittently failing because it would
> traverse trailing garbage when u8_strnlen reported 8 chars instead of 4.
>
> Change the scm_i_str2symbol internal hash type to unsigned long to
> explicitly match the hashing result type.

Oh, good catch.

For the final patch please add a ChangeLog-style entry.

> +  // Make sure a utf-8 symbol has the expected hash.  In addition to
> +  // catching algorithmic regressions, this would have caught a
> +  // long-standing buffer overflow.
> +
> +  // περί
> +  char about_u8[] = {0xce, 0xa0, 0xce, 0xb5, 0xcf, 0x81, 0xce, 0xaf, 0};
> +  SCM sym = scm_from_utf8_symbol (about_u8);
> +
> +  const unsigned long expect = 4029223418961680680;
> +  const unsigned long actual = scm_to_ulong (scm_symbol_hash (sym));

Is this a documented example of Jenkins?  Or did you use a reference
implementation?

> Hmm.  I suppose the current test could be handled on the scheme side
> instead.  (I'd started off attempting some more direct, elaborate tests
> that didn't pan out.)  Happy to rework that if desired.

Yes, it may be nicer to have it in ‘test-suite/tests/hash.test’.

AFAICS this will only change the hash of UTF-8 symbols and won’t have
any effect on the output of ‘string-hash’, right?  If not that would be
an incompatibility.

Thanks and sorry for the delay!

Ludo’.

bug#59021: Unbounded heap growth when combining dynamic states & delimited continuation

[Ludovic Courtès]

Ludovic Courtès  skribis:

> Consider this code:
>
> ;; https://issues.guix.gnu.org/58631
> ;; https://github.com/wingo/fibers/issues/65
>
> (define loss
>   (make-vector 100))
>
> (let ((tag (make-prompt-tag "my prompt")))
>   (define handler
> (lambda (k i)
>   (when (zero? (modulo i 200))
> (pk 'heap-size (assoc-ref (gc-stats) 'heap-size)))
>
>   (call-with-prompt tag
> (lambda ()
>   (k (modulo (+ 1 i) 1000)))
> handler)))
>
>   (call-with-prompt tag
> (let ((state (current-dynamic-state)))
>   (lambda ()
> ;; (define (with-dynamic-state state thunk)
> ;;   (let ((previous #f))
> ;; (dynamic-wind
> ;;   (lambda () (set! previous (set-current-dynamic-state state)))
> ;;   thunk
> ;;   (lambda () (set-current-dynamic-state previous)
> (with-dynamic-state state
> (lambda ()
>   (let loop ((i 0))
> (loop (abort-to-prompt tag i)))
> handler))
>
> On Guile 3.0.8, this program exhibits seemingly unbounded heap growth.

This is fixed by the patch below (tested against the test case above and
the Fibers and Shepherd test cases mentioned before):

diff --git a/libguile/vm.c b/libguile/vm.c
index 6fd5c554f..516bae773 100644
--- a/libguile/vm.c
+++ b/libguile/vm.c
@@ -165,11 +165,13 @@ capture_stack (union scm_vm_stack_element *stack_top,
scm_t_dynstack *dynstack, uint32_t flags)
 {
   struct scm_vm_cont *p;
+  size_t stack_size;
 
-  p = scm_gc_malloc (sizeof (*p), "capture_vm_cont");
-  p->stack_size = stack_top - sp;
-  p->stack_bottom = scm_gc_malloc (p->stack_size * sizeof (*p->stack_bottom),
-   "capture_vm_cont");
+  stack_size = stack_top - sp;
+  p = scm_gc_malloc (sizeof (*p) + stack_size * sizeof (*p->stack_bottom),
+ "capture_vm_cont");
+  p->stack_size = stack_size;
+  p->stack_bottom = (void *) ((char *) p + sizeof (*p));
   p->vra = vra;
   p->mra = mra;
   p->fp_offset = stack_top - fp;

Using a simple heap profiler (more on that later), I noticed that the
stacks allocated at ‘p->stack_bottom’ would be partly retained,
explaining the heap growth.

I couldn’t pinpoint what exactly is keeping a pointer to the stack, but
what I can tell is that the trick above makes that impossible (because
we disable interior pointer tracing), hence the difference.

Also, why changing the SCM_DYNSTACK_TYPE_DYNAMIC_STATE entry to an
SCM_DYNSTACK_TYPE_UNWINDER entry would make a difference remains a
mystery to me.

I’m interested in theories that would explain all this in more detail!
I’ll go ahead with the fix above if there are no objections.

It’s not fully satisfying but still it’s a relief.

Ludo’.

bug#59021: Unbounded heap growth when combining dynamic states & delimited continuation

[Ludovic Courtès]

(This is a followup to ,
itself a followup to .)

Consider this code:

--8<---cut here---start->8---
;; https://issues.guix.gnu.org/58631
;; https://github.com/wingo/fibers/issues/65

(define loss
  (make-vector 100))

(let ((tag (make-prompt-tag "my prompt")))
  (define handler
(lambda (k i)
  (when (zero? (modulo i 200))
(pk 'heap-size (assoc-ref (gc-stats) 'heap-size)))

  (call-with-prompt tag
(lambda ()
  (k (modulo (+ 1 i) 1000)))
handler)))

  (call-with-prompt tag
(let ((state (current-dynamic-state)))
  (lambda ()
;; (define (with-dynamic-state state thunk)
;;   (let ((previous #f))
;; (dynamic-wind
;;   (lambda () (set! previous (set-current-dynamic-state state)))
;;   thunk
;;   (lambda () (set-current-dynamic-state previous)
(with-dynamic-state state
(lambda ()
  (let loop ((i 0))
(loop (abort-to-prompt tag i)))
handler))
--8<---cut here---end--->8---

On Guile 3.0.8, this program exhibits seemingly unbounded heap growth.
Uncommenting the local ‘with-dynamic-state’ definition fixes the
problem.

Ludo’.

bug#58905: Vector initialization loop remains even when allocation is elided

[Ludovic Courtès]

Spot the issue in the code below?

--8<---cut here---start->8---
scheme@(guile-user)> (version)
$28 = "3.0.8"
scheme@(guile-user)> ,c (let () (make-vector 12345 #\x) 42)
Disassembly of  at #xe8:

   0(instrument-entry 42) at (unknown 
file):5009:35
   2(assert-nargs-ee/locals 1 1);; 2 slots (0 args)
   3(load-u64 1 0 12346)  at (unknown 
file):5009:11
   6(load-u64 0 0 1)
   9(uadd/immediate 0 0 1)  
  10(u64 L2
L1:
  12(instrument-loop 30)
  14(handle-interrupts) 
  15(uadd/immediate 0 0 1)  
  16(u64 L1
L2:
  18(make-immediate 1 170)  ;; 42 at (unknown 
file):5009:35
  19(reset-frame 1) ;; 1 slot
  20(handle-interrupts) 
  21(return-values) 

--8<---cut here---end--->8---

Vector allocation is elided (no ‘allocate-words’ instruction),
rightfully so (assuming we neglect the possibility of an out-of-memory
effect), but we still have the skeleton of the vector initialization
loop with its 12345 iterations.

Vector allocation is removed by the ‘eliminate-dead-code’ pass, which
happens on the VM instruction stream, so it explains why the loop
remains.

Still kinda surprising!

Also weird is the fact that ‘make-vector’ is kept by ‘peval’, on the
grounds that it might throw, but down the road it’s removed anyway.

Thoughts?

Ludo’.

bug#57948: [PATCH] Avoid 'frame-local-ref' errors when printing backtrace.

[Ludovic Courtès]

Hi,

Andrew Whatson  skribis:

> Ludovic Courtès  wrote:
>>
>> It would be great if you could add a simple test case though, so that
>> the bug doesn’t eventually come back to haunt us.
>>
>> Could you send an updated patch?
>
> Ah yes, getting this covered in a test is on my list.  I had trouble
> writing a reproducer previously, I think the bug might only occur in a
> nested compilation context (top-level error compiling a dependency
> module), but I'll have another go.

Awesome.

>> PS: BTW, it’ll be great to have more patches from you!  :-) To that end,
>> please check out the new Guile copyright policy and let us know what
>> option you’d like to choose:
>> <https://lists.gnu.org/archive/html/guile-devel/2022-10/msg8.html>.
>
> I have already assigned copyright of my work on Guile to the FSF,
> happy for that to remain.

Oh sorry I had overlooked that, perfect!

Thanks,
Ludo’.

bug#58109: simple-format vs (ice-9 format) bug in 3.0.7?

[Ludovic Courtès]

Hi,

Jean Abou Samra  skribis:

> OK, understood. How about adding comments and documentation?

That’s a good idea.  Applied with minor tweaks and a commit log.

Thanks!

Ludo’.

bug#57948: [PATCH] Avoid 'frame-local-ref' errors when printing backtrace.

[Ludovic Courtès]

Hi Andrew,

Andrew Whatson  skribis:

> Workaround for .
>
> * module/system/vm/frame.scm (frame-call-representation): Treat a
> binding as "unspecified" if its slot exceeds 'frame-num-locals'.

Yay, great to see that fixed (or almost)!

It would be great if you could add a simple test case though, so that
the bug doesn’t eventually come back to haunt us.

Could you send an updated patch?

Thanks,
Ludo’.

PS: BTW, it’ll be great to have more patches from you!  :-) To that end,
please check out the new Guile copyright policy and let us know what
option you’d like to choose:
.

bug#58217: tree-il->bytecode compilation of (not (list …)) fails

[Ludovic Courtès]

Ludovic Courtès  skribis:

> I believe this is fixed by changing ‘predicate?’ to:
>
> (define (predicate? name)
>   (and=> (lookup-primitive name) primitive-predicate?))

Done in e2797f529b8934b0a11b9f6aebbf937b183ece77!

Ludo’.

bug#57377: [PATCH] api-coverage.texi: fix example, remove mentions of unimplemented features

[Ludovic Courtès]

Hi Antoine,

Antoine Kalmbach  skribis:

> * doc/ref/api-coverage.texi (Code Coverage): #:modules is not
>   supported... never seems to have been supported?
>   (example): close port, not file.

In commit 4456245753ff925cafd3e72d130761b6f1c2c419, Jessica Talon added
support for #:modules.

I applied the fix for the typo you found in the example, though.

Thank you!

Ludo’.

bug#57440: Guile manual has incorrect history on the name

[Ludovic Courtès]

Hi Arne and Lee,

"Dr. Arne Babenhauserheide"  skribis:

> From 8bfc607ffbc433b7dde50787cf813bd455726daa Mon Sep 17 00:00:00 2001
> From: Arne Babenhauserheide 
> Date: Sat, 27 Aug 2022 01:57:57 +0200
> Subject: [PATCH] doc: Lee Thomas suggested the name change.

Thanks for the historical research work, please push, Arne!

And of course, thank you Lee for this significant contribution to the
project, and thanks for letting us know.

Cheers,
Ludo’.

bug#57776: [PATCH] * doc/ref/compiler.texi (CPS Soup) : fix small typo in doc (closing paren out of @code{} tag).

[Ludovic Courtès]

Applied, thanks!

bug#57778: [PATCH] * doc/ref/sxml.texi (7.21.4 Transforming SXML): adds a mention to the module to be imported for the procedures introduced in this section of the documentation.

[Ludovic Courtès]

Hi,

Fulbert  skribis:

> Small change proposal in attached patch to give some direction on module
> to use for functions discussed in the (7.21.4 Transforming SXML) section
> of the documentation.

A welcome improvement!

> +The pre-post-order function, in (sxml transform) module, visits the
  ^
I added a missing “the” and wrap the procedure name and module name in @code.

Applied, thanks!

Ludo’.

bug#58109: simple-format vs (ice-9 format) bug in 3.0.7?

[Ludovic Courtès]

Hi,

Jean Abou Samra  skribis:

> Uh, at the end of module/ice-9/format.scm, there is
>
> ;; Thanks to Shuji Narazaki
> (module-set! the-root-module 'format format)
>
> which dates back to
>
> commit 14469b7c69feb0f2c5b8a093f19fe2a548b31c5b
> Author: Greg J. Badros 
> Date:   Thu Jan 20 20:58:30 2000 +

[...]

> This probably predates #:replace and could be removed now, right?

Yes, it could be removed, but probably not before the 4.0 series.

The ‘-Wformat’ warning introduced sometime in the 2.0 or 2.2 series
prepared for that removal by warning about simple-format/format
mismatches, but there’s probably still code out there that assumes
‘format’ is the full-blown ‘format’, even when (ice-9 format) is not
explicitly imported.

Thanks,
Ludo’.

bug#49941: [PATCH] module/system/base/target.scm: support riscv32

[Ludovic Courtès]

Hi,

Fabrice Fontaine  skribis:

> Fix the following build failure on riscv32:
>
> system/base/target.scm:132:16: In procedure triplet-pointer-size:
> unknown CPU word size "riscv32"
>
> Fixes:
>  - 
> http://autobuild.buildroot.org/results/6705630c1484239ec8b73d57ebc2e2570fbfc8f8
>
> Signed-off-by: Fabrice Fontaine 

Pushed a while back as ffb33fd66bba30f6a3e554be644fcaf269d39f05, thanks!

Ludo'.

bug#58217: tree-il->bytecode compilation of (not (list …)) fails

[Ludovic Courtès]

Hi,

I stumbled upon this bug of the baseline compiler:

--8<---cut here---start->8---
scheme@(guile-user)> (compile '(not (list 1 2)) #:optimization-level 2)
$3 = #f
scheme@(guile-user)> (compile '(not (list 1 2)) #:optimization-level 1)
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure struct-vtable: Wrong type argument in position 1 (expecting 
struct): #f

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(guile-user) [1]> ,bt
In system/base/compile.scm:
   352:28  5 (compile _ #:from _ #:to _ #:env _ #:optimization-level _ 
#:warning-level _ #:opts _)
   265:44  4 (_ _ _)
   261:27  3 (_ _ _)
In language/tree-il/compile-bytecode.scm:
  1386:14  2 (compile-bytecode # # …)
   412:30  1 (_ _)
In ice-9/boot-9.scm:
  1685:16  0 (raise-exception _ #:continuable? _)
scheme@(guile-user) [1]> (version)
$4 = "3.0.8"
--8<---cut here---end--->8---

The exception is raised in the ‘predicate?’ call below, where
(lookup-primitive 'list) returns #f:

  (define (finish-conditional exp)
(define (true? x) (match x (($  _ val) val) (_ #f)))
(define (false? x) (match x (($  _ val) (not val)) (_ #f)))
(define (predicate? name) (primitive-predicate? (lookup-primitive name)))
(match exp
  (($  src ($  _ test (? true?) (? false?))
  consequent alternate)
   (finish-conditional (make-conditional src test consequent alternate)))
  (($  src ($  _ test (? false?) (? true?))
  consequent alternate)
   (finish-conditional (make-conditional src test alternate consequent)))
  (($  src ($  _ (? predicate?)))
   exp)
  (($  src test consequent alternate)
   (make-conditional src (make-primcall src 'false? (list test))
 alternate consequent

I believe this is fixed by changing ‘predicate?’ to:

(define (predicate? name)
  (and=> (lookup-primitive name) primitive-predicate?))

Thoughts?

Ludo’.

bug#57567: ‘primitive-load’ should open files with O_CLOEXEC

[Ludovic Courtès]

Ludovic Courtès  skribis:

> Currently ‘primitive-load’ opens files without O_CLOEXEC:
>
> port = scm_open_file_with_encoding (filename,
> scm_from_latin1_string ("r"),
> SCM_BOOL_T, /* guess_encoding */
> scm_from_latin1_string ("UTF-8"));

[...]

> For a proper fix, one possibility would be to first augment
> ‘scm_i_mode_to_open_flags’ to recognize some letter for O_CLOEXEC.  In
> glibc, fopen(3) uses “e” for that.

I went ahead and did that:

  0aa1a9976 'primitive-load' opens files with O_CLOEXEC.
  a356ceebe Add support for "e" flag (O_CLOEXEC) to 'open-file'.

Ludo’.

bug#57507: Regular expression matching depends on locale encoding

[Ludovic Courtès]

Hi,

Jean Abou Samra  skribis:

> Le 05/09/2022 à 09:48, Ludovic Courtès a écrit :
>> Hi Jean,
>>
>> Jean Abou Samra  skribis:
>>
>>> Regular expressions do funky things with Unicode if a non-Unicode-aware
>>> locale is set. Yet, they're purely string operations, so I don't think
>>> it's expected that they depend on the locale encoding.
>> This is the expected behavior: first because (ice-9 regex) is
>> implemented in terms of the libc regex functions, as Dale put (but that
>> could be thought as an implementation detail), and second because things
>> such as character classes are necessarily locale-dependent (this has
>> bitten us in the past, for instance with <https://bugs.gnu.org/35785>).
>>
>> I hope that makes sense.
>
>
>
> OK, thanks, but in this case, it should be clearly stated as a limitation
> in the (ice-9 regex) documentation IMHO. If you don't know what constraints
> there are on the implementation, there is no reason to expect this. Would it
> help if I submitted a patch for that?

Yes, that’d be welcome.  I would not call it a constraint or limitation;
for example, that ‘w’ is not a letter in Swedish is the kind of thing
you’d generally want to take into account.  Now, it’d be nice if one
could easily specify the locale to operate under, with an API similar to
that of (ice-9 i18n) and its first-class locale objects.

Thanks,
Ludo’.

bug#57507: Regular expression matching depends on locale encoding

[Ludovic Courtès]

Hi Jean,

Jean Abou Samra  skribis:

> Regular expressions do funky things with Unicode if a non-Unicode-aware
> locale is set. Yet, they're purely string operations, so I don't think
> it's expected that they depend on the locale encoding.

This is the expected behavior: first because (ice-9 regex) is
implemented in terms of the libc regex functions, as Dale put (but that
could be thought as an implementation detail), and second because things
such as character classes are necessarily locale-dependent (this has
bitten us in the past, for instance with ).

I hope that makes sense.

Thanks,
Ludo’.

bug#57567: ‘primitive-load’ should open files with O_CLOEXEC

[Ludovic Courtès]

Hi,

Currently ‘primitive-load’ opens files without O_CLOEXEC:

port = scm_open_file_with_encoding (filename,
scm_from_latin1_string ("r"),
SCM_BOOL_T, /* guess_encoding */
scm_from_latin1_string ("UTF-8"));

We should fix that; here’s an example where it shows:

--8<---cut here---start->8---
$ guix shell bash -- sh -c 'ls -l /proc/$$/fd'
total 0
lrwx-- 1 ludo users 64 Sep  3 21:20 0 -> /dev/pts/0
lrwx-- 1 ludo users 64 Sep  3 21:20 1 -> /dev/pts/0
lrwx-- 1 ludo users 64 Sep  3 21:20 2 -> /dev/pts/0
lr-x-- 1 ludo users 64 Sep  3 21:20 3 -> /proc/9563/fd
lr-x-- 1 ludo users 64 Sep  3 21:20 5 -> 
/gnu/store/4qbqaa4dgr2fwjjs9i2naqrd0djrcnw3-guix-command
$ head -1 $(type -P guix)
#!/gnu/store/9z95jms1r801z1kxpiq5xw594cxaw5jx-guile-wrapper/bin/guile 
--no-auto-compile
--8<---cut here---end--->8---

For a proper fix, one possibility would be to first augment
‘scm_i_mode_to_open_flags’ to recognize some letter for O_CLOEXEC.  In
glibc, fopen(3) uses “e” for that.

Thoughts?

Ludo’.

bug#53928: [PATCH] Allow null bytes in UNIX sockets.

[Ludovic Courtès]

Hi,

Liliana Marie Prikler  skribis:

> The current socket address constructors all assume, that there are no
> null bytes in the socket path.  This assumption does not hold in Linux,
> which uses an initial null byte to demarcate abstract sockets and
> ignores all further null bytes [1].
>
> [1] https://www.man7.org/linux/man-pages/man7/unix.7.html
>
> * libguile/sockets.c (scm_fill_sockaddr)[HAVE_UNIX_DOMAIN_SOCKETS]:
> Use scm_to_locale_stringn to construct c_address.
> Use memcpy instead of strcpy and calculate size directly instead of
> using SUN_LEN.
> (_scm_from_sockaddr): Copy the entire path up to the limits imposed by
> addr_size.
> * test-suite/tests/00-socket.test: ("make-socket-address"): Add case for
> abstract unix sockets.
> ("AF_UNIX/SOCK_STREAM"): Add abstract socket versions of bind, listen,
> connect and accept.

Finally applied, thanks!

Ludo’.

bug#53125: Missing abstract unix socket support

[Ludovic Courtès]

Hi,

Zhu Zihao  skribis:

> Currently, It's not valid to create abstract unix socket in Guile via
> `connect` procedure, because it rejects string with leading NUL.

Fixed in 01b686b701dc06f6623f0cc75acd4583c0296333 with Liliana’s patch:

  https://issues.guix.gnu.org/53928

Thanks,
Ludo’.

bug#54911: Missing modules argument for coverage-data->lcov

[Ludovic Courtès]

Hi Jessica,

Jessica Tallon  skribis:

>>From 828daf200539d3a642fcf8210df7b58aa0d5fede Mon Sep 17 00:00:00 2001
> From: Jessica Tallon 
> Date: Wed, 13 Apr 2022 15:57:24 +0200
> Subject: [PATCH] Fix missing modules argument for coverage-data->lcov
>
> The code coverage function `coverage-data->lcov` has a documented
> `modules` argument, however that was missing from the source. I have
> added it so when supplied it only converts the coverage data for the
> supplied modules. If not supplied it defaults the old behavour of
> including all the modules currently loaded.

I added a ChangeLog-style entry and committed it.

Thanks for fixing this issue!

Ludo’.

bug#54622: [PATCH] Find unidata_to_charset.awk in $(srcdir)

[Ludovic Courtès]

Hi Andreas,

Andreas Schwab  skribis:

> * libguile/Makefile.am (srfi-14.i.c): Prepend $(srcdir).

A similar fix was committed in the meantime as
cc455976813ab94de121395982435430874cbf58.

Thanks,
Ludo’.

bug#54915: [PATCH] Guile rejects empty vendor in GNU triplets, as used by NetBSD

[Ludovic Courtès]

Hi,

Taylor R Campbell  skribis:

>>From 12440a85559c3de5e6bced9c9377f3d5d7f5948e Mon Sep 17 00:00:00 2001
> From: Taylor R Campbell 
> Date: Wed, 13 Apr 2022 09:51:08 +
> Subject: [PATCH] Allow empty vendor string in GNU target triplets.
>
> NetBSD and pkgsrc have been using an empty vendor string since the
> mid-'90s, such as x86_64--netbsd.  pkgsrc has been carrying around a
> workaround just the guile build for a long time.  (Before that,
> NetBSD omitted the vendor altogether, so if x86_64 existed then it
> might have been `x86_64-netbsd', but that caused more problems.)
> This change makes Guile accept an empty vendor string so workarounds
> are no longer necessary.
>
> * module/system/base/target.scm (validate-target): Allow empty vendor
> string in GNU target triplets.

I added tests in cross-compilation.test and committed.

Thanks!

Ludo’.

bug#55934: [PATCH] (library ...) form in cond-expand inside R7RS define-library

[Ludovic Courtès]

Hi,

Mihail Iosilevich  skribis:

> Guile (3.0.8) reports a compilation error when cond-expand tries to
> check existence of a missing library:
>
> scheme@(guile-user)> (define-library (test)
>(cond-expand
> ((library (scheme sort))
>  (import (scheme sort)
> While compiling expression:
> no code for module (scheme sort)
>
> It looks like bug #40252 was not fully eliminated.
>
> Also, (library ...) cannot handle module names like (srfi 1), though
> (import (srfi 1)) works fine. For example, this code fails:
>
> scheme@(guile-user)> (define-library (test)
>(cond-expand
> ((library (srfi 1))
>  (import (srfi 1)
> While compiling expression:
> In procedure symbol->string: Wrong type argument in position 1
> (expecting symbol): 1
>
> There are probably other cases when (library ...) and (import ...) does
> not work identically: (library ...) uses resolve-interface while
> (import ...) uses resolve-r6rs-interface.
>
> This patch fixes both issues.

That looks reasonable to me.

I added a ChangeLog-style entry and committed it, thanks!

Ludo’.

bug#54538: (ice-9 suspendable-ports) doesn’t implement the 'escape conversion strategy

[Ludovic Courtès]

‘peek-char’ in (ice-9 suspendable-ports) checks for the 'substitute port
conversion strategy, but it doesn’t check for 'escape:

(if (eq? (port-conversion-strategy port) 'substitute)
(values #\xFFFD buf (+ cur len))
(decoding-error "peek-char" port))

and:

 ((eq? (port-conversion-strategy port) 'substitute)
  (values #\xFFFD buf (+ cur prev-input-size)))

This is in 3.0.8.

Ludo’.

bug#54171: [PATCH] web: default to INADDR_ANY instead of INADDR_LOOPBACK

[Ludovic Courtès]

Bon dia!

Aleix Conchillo Flaqué  skribis:

> Using INADDR_ANY instead of INADDR_LOOPBACK makes it convenient when
> starting the web server inside containers without the need to having to
> specify INADDR_ANY all the time. This is the default in most libraries
> and languages.
>
> This doesn't break backwards compatibility since INADDR_LOOPBACK is also
> included in INADDR_ANY.

A potential problem with changing the default is that people using the
defaults would all of a sudden have their servers accessible from the
outside, which could be a real problem.

Also, defaulting to INADDR_LOOPBACK is a conservative choice, with the
understanding that you have to explicitly say so if you want your server
to be directly accessible from the outside.  (In most cases, one would
run Guile web servers behind a proxy such as nginx.)

So I have a preference for the status quo.

WDYT?

Ludo’.

bug#54198: Guile 3.0.8 cross-compiled to i586-pc-gnu crashes

[Ludovic Courtès]

Hi,

Ludovic Courtès  skribis:

> #11 0x010b68dc in range_error (bad_val=bad_val@entry=0x6, min=0x2, 
> max=max@entry=0xfffe) at numbers.c:6611
> #12 0x010b6b8f in inum_in_range (max=-1, min=0, x=0x6) at numbers.c:6630
> #13 scm_to_uint32 (arg=0x6) at numbers.c:6787

Turns out we had SCM_SIZEOF_LONG == 8 (instead of 4, since i586-pc-gnu
is a 32-bit platform), as Andy hinted on IRC yesterday.

Fixed in Guile commit 24b30130ca75653bdbacea84ce0443608379d630.

Ludo’.