[Chicken-hackers] [PATCH] [PRERELEASE] Fix #1283

[Peter Bex]

Good news everyone!

I managed to pinpoint the cause of #1283, and it's so simple I'm kind
of surprised this hasn't been found earlier.

When a process receives a POSIX signal, C_raise_interrupt will
modify C_stack_limit to be identical to stack_bottom, which means
the very next stack check in C_demand() will fail, causing a GC
to be triggered which then will reset the stack and pick the pending
interrupts from the list.

This is really a bit of an abuse/overloading of the C_stack_limit
variable, and it is biting us in the behind pretty badly now:
The C_stack_check1() macro also uses C_stack_limit.  As you know, this
macro is used in equal? to "detect" cyclic or very deeply nested
structures, and in C_stack_overflow_check(), which is inserted by the
compiler to allow programs to recover from detected runaway recursions
by raising a condition.

This means that if a signal arrives in between a C_demand() and a
C_stack_overflow_check() call, the latter will trigger the aforementioned
condition signaling.

As far as I understand it, this problem has always lurked in the code
base.  I guess we don't run into this very often because a typical
process won't receive that many signals, and it's a big coincidence if
it will happen before C_stack_check1() instead of C_demand() (the latter
occurs much more often).  This is also the reason it got triggered in
the scsh-process tests: it will receive SIGCHLD when a child is finished,
and it does spawn quite a few child processes.

The attached patch is a simple one, it simply replaces all C_stack_limit
by C_stack_hard_limit, which indicates the actual limit.  C_stack_limit
is initialised to C_stack_hard_limit, so it will have the same meaning as
before.  The signal handler and C_demand code still use C_stack_limit, so
we can keep relying on this trick to force a GC.  One very nice advantage
of this is that we can get rid of the saved_stack_limit: we can simply
set C_stack_limit to C_stack_hard_limit when we need to restore it.

I'm not super-happy with the fact that we are mutating C_stack_limit like
we do to enforce a GC, but this has been done for performance reasons I
think: only one variable needs to be checked in C_demand.  Of course
C_demand is used EVERYWHERE, so it really needs to be fast.  For this
reason I decided to keep the existing mechanism, and also to keep the
patch as simple and understandable as possible, so it doesn't involve
a complete code overhaul just before the second release candidate.

The attached patches should go into prerelease, master and chicken-5.

Cheers,
Peter

0001-Avoid-triggering-stack-overflows-in-signal-handler.chicken-5.patch
Description: Text Data

0001-Avoid-triggering-stack-overflows-in-signal-handler.master-prerelease.patch
Description: Text Data

signature.asc
Description: Digital signature