bug#27476: libguile/memoize.c is not thread safe, so syntax parameter expansion is not thread-safe

[Ludovic Courtès]

Hello Andy!

Since guix-core.drv is the best reproducer I have so far for this syntax
parameter crash, I modified (guix self) to print the name of the files
it’s compiling, and here’s the crash I got (on a 24-core machine):

--8<---------------cut here---------------start------------->8---
building /gnu/store/hf324mhj5607hh2izb01dzhwakmn8am8-guix-core.drv...
[ 39/ 78] loading...    100.0% of 39 filesbuilding "guix/config.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix/monad-repl.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix/store.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix/utils.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix/memoization.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix/profiling.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix/build/utils.scm"
[ 39/ 78] compiling...    0.0% of 39 filesbuilding "guix/build/syscalls.scm"
[ 40/ 78] compiling...    2.6% of 39 filesbuilding "guix/deprecation.scm"
[ 41/ 78] compiling...    5.1% of 39 filesbuilding "guix/i18n.scm"
[ 42/ 78] compiling...    7.7% of 39 filesbuilding "guix/serialization.scm"
[ 43/ 78] compiling...   10.3% of 39 filesbuilding "guix/combinators.scm"
[ 44/ 78] compiling...   12.8% of 39 filesbuilding "guix/monads.scm"
[ 45/ 78] compiling...   15.4% of 39 filesbuilding "guix/records.scm"
[ 46/ 78] compiling...   17.9% of 39 filesIn ice-9/psyntax.scm:
  2338:44 19 (expand-let _ _ _ ((line . 447) (column . 6) (filename . 
"./guix/monads.scm")) _ #<procedure build-let (src ids vars val-exps body-exp)> 
_ _ _)
  1679:45 18 (parse _ _ _ _ _ _ _)
In ice-9/boot-9.scm:
   222:17 17 (map1 (((("placeholder" placeholder) ("l-10a3c941d34314a1-4889" 
lexical . failure-10a3c941d34314a1-488a) ("placeholder" placeholder) 
("placeholder" placeholder) ("l-10a3c9?" . #) ?) ?)))
In ice-9/psyntax.scm:
  1409:12 16 (_ _ _ #<syntax (#<syntax failure>)>)
  2338:44 15 (expand-let _ _ _ ((line . 447) (column . 6) (filename . 
"./guix/monads.scm")) (hygiene guix monads) #<procedure build-let (src ids vars 
val-exps body-exp)> _ _ ((#<syntax match-o?> ?)))
  1679:45 14 (parse _ _ _ _ _ _ _)
In ice-9/boot-9.scm:
   222:17 13 (map1 (((("l-10a3c941d34314a1-4894" macro . #<procedure 460ab80 at 
ice-9/eval.scm:333:13 (a)>) ("placeholder" placeholder) 
("l-10a3c941d34314a1-488f" lexical . #) ("l-10?" . #) ?) . #)))
In ice-9/psyntax.scm:
  2338:44 12 (expand-let _ _ _ ((line . 447) (column . 6) (filename . 
"./guix/monads.scm")) (hygiene guix monads) #<procedure build-let (src ids vars 
val-exps body-exp)> _ _ ((#<syntax match-o?> ?)))
  1679:45 11 (parse _ _ _ _ _ _ _)
In ice-9/boot-9.scm:
   222:17 10 (map1 (((("l-10a3c941d34314a1-48b0" macro . #<procedure 40f9b40 at 
ice-9/eval.scm:333:13 (a)>) ("placeholder" placeholder) 
("l-10a3c941d34314a1-48ac" lexical . #) ("l-10?" . #) ?) . #)))
In ice-9/psyntax.scm:
  2338:44  9 (expand-let _ _ _ ((line . 447) (column . 6) (filename . 
"./guix/monads.scm")) (hygiene guix monads) #<procedure build-let (src ids vars 
val-exps body-exp)> _ _ ((#<syntax match-d?> ?)))
  1612:33  8 (parse (((("placeholder" placeholder) ("l-10a3c941d34314a1-48c8" 
lexical . tail-10a3c941d34314a1-48c9) ("l-10a3c941d34314a1-48b0" macro . 
#<procedure 40f9b40 at ice-9/eval.?>) ?) . #)) ?)
  1348:32  7 (syntax-type (>>= (mproc head result) (lambda (result) (loop tail 
result))) (("placeholder" placeholder) ("l-10a3c941d34314a1-48c8" lexical . 
tail-10a3c941d34314a1-48c9) ("l-?" . #) ?) ?)
  1559:32  6 (expand-macro #<procedure 17a5ba0 at ice-9/eval.scm:333:13 (a)> _ 
_ _ _ _ _)
In ice-9/boot-9.scm:
   752:25  5 (dispatch-exception _ _ _)
   751:25  4 (dispatch-exception 1 syntax-error (>>= ">>= (bind) used outside 
of 'with-monad'" ((line . 451) (column . 9) (filename . "./guix/monads.scm")) 
(>>= (mproc head result) (lambda # ?)) #))
In guix/build/compile.scm:
    122:6  3 (_ _ . _)
In ice-9/boot-9.scm:
    829:9  2 (catch #t #<procedure 40f93e0 at guix/build/compile.scm:122:6 ()> 
#<procedure 7fffefd02888 at guix/build/compile.scm:122:6 args> _)
In guix/build/compile.scm:
   125:21  1 (_)
In unknown file:
           0 (make-stack #t)
guix/build/compile.scm:125:21: Syntax error:
./guix/monads.scm:452:9: >>=: >>= (bind) used outside of 'with-monad' in form 
(>>= (mproc head result) (lambda (result) (loop tail result)))
builder for `/gnu/store/hf324mhj5607hh2izb01dzhwakmn8am8-guix-core.drv' failed 
with exit code 1
--8<---------------cut here---------------end--------------->8---

Here (guix monads) was already loaded before, but it’s only when
compiling (guix monads), so after it had been loaded, that we get the
error.  The syntax parameter in question is defined in (guix monads)
itself.

I drew the conclusion that our syntax parameter is redefined when we
compile or when we load (guix monads), so there’s a chance that we get
to see the wrong value when we expand (guix monads) (I’m not entirely
sure about the exact sequence of events.)

So I came up with ‘define-syntax-parameter-once’, which is like
‘define-once’ but for syntax parameters (note that we can’t use
‘define-once’ in ‘define-syntax-parameter-once’ because it expands to a
reference to NAME, which doesn’t work for a macro):

diff --git a/guix/monads.scm b/guix/monads.scm
index 6ae616aca9..1bbf79c8ba 100644
--- a/guix/monads.scm
+++ b/guix/monads.scm
@@ -274,12 +274,20 @@ more optimizations."
                    (_
                     #'generic-name))))))))))
 
-(define-syntax-parameter >>=
+(define-syntax-rule (define-syntax-parameter-once name proc)
+  (eval-when (load eval expand compile)
+    (define name
+      (if (module-locally-bound? (current-module) 'name)
+          (module-ref (current-module) 'name)
+          (make-syntax-transformer 'name 'syntax-parameter
+                                   (list proc))))))
+
+(define-syntax-parameter-once >>=
   ;; The name 'bind' is already taken, so we choose this (obscure) symbol.
   (lambda (s)
     (syntax-violation '>>= ">>= (bind) used outside of 'with-monad'" s)))
 
-(define-syntax-parameter return
+(define-syntax-parameter-once return
   (lambda (s)
     (syntax-violation 'return "return used outside of 'with-monad'" s)))
 

I’ve done a number of rebuilds of guix-core.drv on that 24-core machine
and AFAICS that fixes the issue!

We’ll also have to use it in (guix gexp), which I’m pretty sure will fix
<https://issues.guix.info/issue/28144>.

I’ll push this workaround if there are no objections.

On the Guile side, we could maybe arrange to always have ‘define-once’
semantics for those bindings introduced at expansion time, as shown
below (untested):

--- a/module/ice-9/psyntax.scm
+++ b/module/ice-9/psyntax.scm
@@ -296,9 +296,10 @@
 
       (define put-global-definition-hook
         (lambda (symbol type val)
-          (module-define! (current-module)
-                          symbol
-                          (make-syntax-transformer symbol type val))))
+          (unless (module-locally-bound? (current-module) symbol)
+            (module-define! (current-module)
+                            symbol
+                            (make-syntax-transformer symbol type val)))))
     
       (define get-global-definition-hook
         (lambda (symbol module)

WDYT, Andy?

The discussion we had at FOSDEM turned out to be very helpful, thanks
a lot!

Ludo’.