Re: [Chicken-hackers] PATCH: allow signal handlers to be called from any thread.

[Jörg F . Wittenberger]

Sorry for replying to my own message.

Am 14.01.2016 um 22:18 schrieb Jörg F. Wittenberger:
> Great that this made it so far.
> 
> Am 14.01.2016 um 20:20 schrieb Peter Bex:
>> On Fri, Jan 15, 2016 at 12:59:28AM +1300, Evan Hanson wrote:
>>> Hi all,
>>>
>>> Attached is a signed-off copy of this patch, with some of the info in
>>> the comments moved into the commit message.
> 
> Actually I'm slightly worried that those comments are stashed away
> somewhere in the commit message.  The issue is somewhat thorny (as you
> can see from the number of failed attempts I made on it).

It took much of my sleep tonight.  So I'd rather like to take my wording
back and escalate to being "seriously worried".

After all: I have a tendency to be too terse on comments.  But reading
the code now I could easily be tempted to make changes causing another
regression.  I would not be surprised if an innocent reader would read
the code with the comment missing and conclude that we save the local
variable `stack_limit` and simply assign C_stack_limit to
`saved_stack_limit`, which would bring the bug back.

((BTW: I've been hunting this one for years.  First time the real reason
escaped me thus I just worked around the symptom by limiting the
mutation stack when it grew beyond bonds dues to missing gc.  See
C_mutation_stack_limit here:
http://lists.nongnu.org/archive/html/chicken-hackers/2012-06/msg00051.html
))

Apparently my comments where still too terse: even an experienced
chicken hacker proof-reading the code did conclude:

>>> It won't change anything in the normal, single-threaded case

The old code did:

      interrupt_time = C_cpu_milliseconds();
      pending_interrupts[ pending_interrupts_count++ ] = reason;

However - please correct me if I'm wrong at that - C_cpu_milliseconds
will result in yet another system call.  Thus another signal may be
dispatched at this point.  And since `pending_interrupts_count` is still
zero the first branch of the if statement would be executed again.

This would not be as bad as in the multi threaded case, since it would
not overwrite the stack limit with a value derived from another threads
stack pointer, but it would still reduce the available stack size by
another 1000 words.  Thus after a long while the stack may shrink to
zero.  That however is exacerbated with the fix.  Now this case would
immediately set the saved_stack_size to stack_bottom and tie chicken
into gc forever.

That's why I swapped the assignments.


> [....]
> 
> Thus we probably need a memory barrier.  Which one?
> __sync_synchronize ? atomic_signal_fence ? __memory_barrier ?

I'll probably aim at this the other day.

Still I'd like to learn which memory barrier I should prefer.

Any suggestions?

Thanks

/Jörg