bug#67255: define-library does not support 'rename' directives
Hey, Maxim Cournoyer writes: > Timothy Sample writes: > >> Fixes <https://bugs.gnu.org/67255>. >> Reported by Maxim Cournoyer . > > Nitpick: at least 'Reported-by' is a common git trailer, and these > must appear at the bottom of the git commit. That’s a fair point. I’m following what seems to be (from the commit log) Guile’s convention here. See $ git log --grep='^Report' Whether it’s a good convention is probably out of scope here! :) > It at least works for my use case (SRFI 128), so it's for sure an > improvement :-). You can see it in action in the series I've sent today. Hooray! -- Tim
bug#67255: define-library does not support 'rename' directives
Timothy Sample writes: > Maxim Cournoyer writes: > >> Our R7RS define-library syntax, from (ice-9 r7rs-library) does not >> support renaming bindings to export, via 'rename' directives. > > I appreciate your R7RS debugging effort. Thanks! Actions speak louder than words, so here’s a patch! The ‘define-library’ syntax uses the R6RS ‘library’ syntax under the hood. TIL that R6RS and R7RS have different syntax for 'rename'. In R6RS, you write: (export (rename (internal external))) while in R7RS, it’s: (export (rename internal external)) My patch adds a conversion step to deal with this difference. >From b87bf8910ac8e75dc0ec63cb7385ddf199fd400c Mon Sep 17 00:00:00 2001 From: Timothy Sample Date: Mon, 20 Nov 2023 11:01:08 -0600 Subject: [PATCH] Use R7RS 'rename' syntax for exports. Fixes <https://bugs.gnu.org/67255>. Reported by Maxim Cournoyer . * module/ice-9/r7rs-libraries.scm (define-library): Convert R7RS exports to R6RS exports before passing them on to 'library'. --- module/ice-9/r7rs-libraries.scm | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/module/ice-9/r7rs-libraries.scm b/module/ice-9/r7rs-libraries.scm index 63a300a26..f8b6b4c59 100644 --- a/module/ice-9/r7rs-libraries.scm +++ b/module/ice-9/r7rs-libraries.scm @@ -1,5 +1,5 @@ ;; R7RS library support -;; Copyright (C) 2020, 2021 Free Software Foundation, Inc. +;; Copyright (C) 2020, 2021, 2023 Free Software Foundation, Inc. ;; ;; This library is free software; you can redistribute it and/or ;; modify it under the terms of the GNU Lesser General Public @@ -97,12 +97,17 @@ ((decl ...) (partition-decls #'(decl ... . decls) exports imports code)) +(define (r7rs-export->r6rs-export export) + (syntax-case export (rename) +((rename internal external) #'(rename (internal external))) +(_ export))) + (syntax-case stx () ((_ name decl ...) (call-with-values (lambda () (partition-decls #'(decl ...) '() '() '())) (lambda (exports imports code) #`(library name - (export . #,exports) + (export . #,(map r7rs-export->r6rs-export exports)) (import . #,imports) . #,code))) -- 2.41.0
bug#67255: define-library does not support 'rename' directives
Hi Maxim, Maxim Cournoyer writes: > Our R7RS define-library syntax, from (ice-9 r7rs-library) does not > support renaming bindings to export, via 'rename' directives. I appreciate your R7RS debugging effort. Thanks! > Our define-module syntax does not have such a feature (of renaming > *exported* bindings), so this would seem to require new development on > that side first. I believe you’re mistaken. At least, the manual says: -- syntax: export variable ... Add all VARIABLEs (which must be symbols or pairs of symbols) to the list of exported bindings of the current module. If VARIABLE is a pair, its ‘car’ gives the name of the variable as seen by the current module and its ‘cdr’ specifies a name for the binding in the current module’s public interface. Using pairs in Guile’s ‘export’ (or ‘#:export’ in ‘define-module’) should be the same as ‘rename’ from R7RS. HTH! -- Tim
bug#66046: Relative includes in R7RS define-library seem broken
Hi Daphne, Daphne Preston-Kendal writes: > A standard layout for R7RS libraries is to have an .sld file > containing the library import and export declarations with a parallel > .scm file with the same name in the same directory, which the .sld > file (include ...)s. > > [...] > > Guile supports looking for .sld files before .scm files if started in > --r7rs mode. However, in this case, it will not find the .scm file if > it’s included from the .sld file. This is currently causing me problems, too, so I will look into writing and submitting a patch. We are technically following R7RS, which says the lookup strategy is “implementation-specific”. However, it goes on to say: “implementations are encouraged to search for files in the directory which contains the including file [...].” This is perfectly reasonable, and like you say, part of an established pattern for portable code. > Changing the path in the include declaration to be absolute fixes the > problem, but then it no longer works on other people’s machines. FWIW, I’ve settled on this (annoying) pattern for now: (cond-expand (guile (import (only (guile) include-from-path)) (begin (include-from-path "relative/from/load/path/foo.scm"))) (else (include "foo.scm"))) I wouldn’t bother with it if I weren’t committed to Guile, though! -- Tim
bug#65132: [PATCH] fix cond1 macro
Hi all, Ekaitz Zarraga writes: > From ab6e4980ea301b5d1e14a98464e5de3e726984f1 Mon Sep 17 00:00:00 2001 > From: Ekaitz Zarraga > Date: Mon, 7 Aug 2023 20:23:42 +0200 > Subject: [PATCH] fix cond1 macro Just adding some context, as we talked about this on IRC. If you take the docs example as is, (define-syntax cond1 (syntax-rules (=> else) ((cond1 test => fun) (let ((exp test)) (if exp (fun exp) #f))) ((cond1 test exp exp* ...) (if test (begin exp exp* ...))) ((cond1 else exp exp* ...) (begin exp exp* ... and invoke it as (cond1 else #t) you get Syntax error: unknown location: else: bad use of 'else' syntactic keyword in subform else of else This is because ‘else’ matches the ‘(cond1 test exp exp* ...)’ pattern and gets inserted as the test: (if else (begin #t)) This patch puts the more specific ‘else’ pattern before the more general ‘test’ pattern so it has a chance to be matched. -- Tim
bug#62691: Calling system* in the module body hangs Guile, while calling open-pipe* does not
Timothy Sample writes:
> It turns out that Guile cannot start its signal handling thread during
> module resolution.
I should add that merely using 'call-with-new-thread' in a module being
imported is enough to trigger deadlock.
> A simple fix would be to use the underlying ‘setter’ generic directly:
>
> (setter thread-join-data thread (cons cv mutex))
>
> It’s hard to read (IMO), but maybe with a comment it’s good enough.
Here’s the patch.
I’m not super familiar with Guile’s threading code, so maybe this quick
fix is a little too quick. That is, maybe it would be better to avoid
object properties altogether, or maybe we could make other changes to
the way that the new thread signals the calling thread. We could also
think through the usage of the ‘autoload’ mutex. The benefit of this
patch is that it is (almost) exactly the same as the old code, so it’s
unlikely to result in nasty surprises.
>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 <https://bugs.gnu.org/62691>.
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.
---
module/ice-9/threads.scm | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/module/ice-9/threads.scm b/module/ice-9/threads.scm
index 5a13cec1d..048d8b085 100644
--- a/module/ice-9/threads.scm
+++ b/module/ice-9/threads.scm
@@ -151,7 +151,15 @@ Once @var{thunk} or @var{handler} returns, the return value is made the
(lambda ()
(lock-mutex mutex)
(set! thread (current-thread))
- (set! (thread-join-data thread) (cons cv mutex))
+ ;; Rather than use the 'set!' syntax here, we use the
+ ;; underlying 'setter' generic function to set the
+ ;; 'thread-join-data' property on 'thread'. This is
+ ;; because 'set!' will try to resolve 'setter' in the
+ ;; '(guile)' module, which means acquiring the
+ ;; 'autoload' mutex. If the calling thread is
+ ;; already holding that mutex, this will result in
+ ;; deadlock. See <https://bugs.gnu.org/62691>.
+ ((setter thread-join-data) thread (cons cv mutex))
(signal-condition-variable cv)
(unlock-mutex mutex)
(call-with-unblocked-asyncs
--
2.39.2
bug#62691: Calling system* in the module body hangs Guile, while calling open-pipe* does not
Hi! Михаил Бахтерев writes: > Greetings! I've hit the following issue. > > [...] > > 4. When loading (i am not sure about the stage) module which contains > in the body system* call Guile hangs on futex operation. The code to > reproduce the behavior. > > $ cat a.scm > (add-to-load-path ".") > (import (b)) > (display "hello world from SCM!") > (newline) > > $ cat b.scm > (define-module (b)) > (system* "echo" "hello world from SYS!") > > $ guile a.scm > HANGS HERE! It hangs for me, too. It turns out that Guile cannot start its signal handling thread during module resolution. A slightly simpler reproducer can be obtained by replacing the call to ‘system*’ with: (sigaction SIGINT SIG_DFL) It doesn’t matter which signal or how it’s handled, just that calling ‘sigaction’ forces Guile to start the signal handling thread. (The reason ‘system*’ hangs is that it calls ‘sigaction’ under the hood.) If I’m reading everything right, this is because Guile uses ‘call-with-new-thread’ to start the signal handling thread. That procedure creates a new thread, and among other things, sets an object property on the thread [1]: (set! (thread-join-data thread) (cons cv mutex)) However, setting the object property requires module resolution, since the above expands to: (((@@ (guile) setter) thread-join-data) thread (cons cv mutex)) ;; ^ Module resolution. At this point, we have two threads. The first one is holding the “autoload” lock (acquired during module resolution) and is trying to start the signal handling thread. It ends up waiting for the signal thread to indicate that it started [1]: (wait-condition-variable cv mutex) The second thread is trying to acquire the “autoload” lock so that it can set its ‘thread-join-data’ property (by resolving ‘setter’). It won’t signal that it has started until it does so. The two threads are deadlocked. A simple fix would be to use the underlying ‘setter’ generic directly: (setter thread-join-data thread (cons cv mutex)) It’s hard to read (IMO), but maybe with a comment it’s good enough. For now it’s best to simply avoid calling ‘system*’ at the top level of a module. -- Tim [1] See ‘call-with-new-thread’ in “module/ice-9/threads.scm”.
bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
Hey Josselin, Thanks for finding this bug! I have one concern about your patches: Josselin Poiret writes: > The second patch removes renumber_file_descriptor, as it is no longer > used. One thing that ‘renumber_file_descriptor’ does that we seem to be losing here is error checking. To my eye, the old code will try and warn the user if they run out of file descriptors, but the new code will not. The other thing that I like is how ‘renumber_file_descriptor’ checks the return value of ‘dup’ in addition to checking ‘errno’. (I realize that the old code skips that check for ‘dup2’ – I’m kinda just stating a preference here.) A quick check of POSIX turns up the following: “the value of ‘errno’ should only be examined when it is indicated to be valid by a function’s return value” [1]. -- Tim [1] https://pubs.opengroup.org/onlinepubs/9699919799/
bug#51276: Problems with format and scaling floats
Hi Guilers, It turns out there’s a little blunder in ‘format’ (from ‘ice-9’). Look at what happens when using the SCALE argument to format a fixed-point float (this is Guile from the Git repo at the time of writing): GNU Guile 3.0.7.6-22120 Copyright (C) 1995-2021 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)> (format #t "~,,3f~%" 0.00123) 0.23 $3 = #t scheme@(guile-user)> (format #t "~,,1f~%" 0.00123) ice-9/boot-9.scm:1685:16: In procedure raise-exception: Value out of range 0 to 400: -1 Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. The first example gives the wrong result. Scaling 0.00123 by 3 should yield 1.23, not 0.23. For the second example, instead of 0.0123, we get an error! What’s going on here? Well, our ‘format’ code comes from SLIB and was written in 1998, so it’s not easy to explain. There’s so much mutation even a C programmer would blush! ;) The issue happens in the ‘format:parse-float’ procedure (which is defined inside of ‘format’). It normalizes the string representation of a number, and applies the scale argument when needed. It does this by keeping a string of digits and the location of the decimal point. Another thing it keeps track of the leading zeros in a variable called ‘left-zeros’. Here’s the code that does the final shifting and places the decimal point: (if (> left-zeros 0) (if (<= left-zeros shift) ; shift always > 0 here (format:fn-shiftleft shift) ; shift out 0s (begin (format:fn-shiftleft left-zeros) (set! format:fn-dot (- shift left-zeros (set! format:fn-dot (+ format:fn-dot shift))) The issue is that the cases in the inner ‘if’ form are reversed. That is, if there are MORE leading zeros than we need to shift, we can just shift. Otherwise (if there are FEWER leading zeros), we need to shift out the zeros and then move the decimal point (‘format:fn-dot’). AFAICS, this bug was in the original SLIB implementation (1998) and has not been fixed since then. It’s been in Guile since 1999. Anyway, that’s more than anyone cares to know Here’s a patch with tests! :) >From c31d1f5d44343da1201ea1be86bc6b2ac8af6c8d Mon Sep 17 00:00:00 2001 From: Timothy Sample Date: Mon, 18 Oct 2021 17:07:41 -0400 Subject: [PATCH] ice-9 format: Fix scaling floats with leading zeros --- module/ice-9/format.scm | 4 ++-- test-suite/tests/format.test | 10 -- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/module/ice-9/format.scm b/module/ice-9/format.scm index 48d9c0c84..ee7cba910 100644 --- a/module/ice-9/format.scm +++ b/module/ice-9/format.scm @@ -1359,10 +1359,10 @@ (else (if (> left-zeros 0) (if (<= left-zeros shift) ; shift always > 0 here - (format:fn-shiftleft shift) ; shift out 0s (begin (format:fn-shiftleft left-zeros) - (set! format:fn-dot (- shift left-zeros + (set! format:fn-dot (- shift left-zeros))) + (format:fn-shiftleft shift)) ; shift out 0s (set! format:fn-dot (+ format:fn-dot shift (let ((negexp ; expon format m.nnnEee diff --git a/test-suite/tests/format.test b/test-suite/tests/format.test index b9aa7a854..d5111f1c6 100644 --- a/test-suite/tests/format.test +++ b/test-suite/tests/format.test @@ -2,7 +2,7 @@ Matthias Koeppe --- June 2001 Copyright (C) 2001, 2003, 2004, 2006, 2010, 2011, 2012, - 2014, 2017 Free Software Foundation, Inc. + 2014, 2017, 2021 Free Software Foundation, Inc. This library is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public @@ -121,7 +121,13 @@ ;; in guile prior to 1.6.9 and 1.8.1, leading zeros were incorrectly ;; stripped, moving the decimal point and giving "25.0" here (pass-if "string 02.5" -(string=? "2.5" (format #f "~f" "02.5" +(string=? "2.5" (format #f "~f" "02.5"))) + + (pass-if "scale with few leading zeros" +(string=? "1.23" (format #f "~,,3f" "0.00123"))) + + (pass-if "scale with many leading zeros" +(string=? "0.0123" (format #f "~,,1f" "0.00123" ;;; ;;; ~h -- 2.33.0
