bug#59055: [PATCH] Fix possible deadlock.
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.
---
libguile/threads.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/libguile/threads.c b/libguile/threads.c
index 280d306bf..0f5cf2ed5 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -1022,14 +1022,7 @@ lock_mutex (enum scm_mutex_kind kind, struct scm_mutex
*m,
if (err == 0)
{
-if (scm_is_eq (m->owner, SCM_BOOL_F))
- {
-m->owner = current_thread->handle;
-scm_i_pthread_mutex_unlock (&m->lock);
-return SCM_BOOL_T;
- }
-else
- continue;
+goto maybe_acquire;
}
else if (err == ETIMEDOUT)
{
@@ -1041,7 +1034,7 @@ lock_mutex (enum scm_mutex_kind kind, struct scm_mutex *m,
scm_i_pthread_mutex_unlock (&m->lock);
scm_async_tick ();
scm_i_scm_pthread_mutex_lock (&m->lock);
-continue;
+goto maybe_acquire;
}
else
{
@@ -1050,6 +1043,14 @@ lock_mutex (enum scm_mutex_kind kind, struct scm_mutex
*m,
errno = err;
SCM_SYSERROR;
}
+
+ maybe_acquire:
+if (scm_is_eq (m->owner, SCM_BOOL_F))
+ {
+m->owner = current_thread->handle;
+scm_i_pthread_mutex_unlock (&m->lock);
+return SCM_BOOL_T;
+ }
}
}
#undef FUNC_NAME
--
2.38.0
bug#59021: Unbounded heap growth when combining dynamic states & delimited continuation
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#56413: [PATCH 1/1] scm_i_utf8_string_hash: compute u8 chars not bytes
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’.
