Re: [Chicken-hackers] Release blocker? Stack checks completely FUBAR? Or am I just paranoid?

[Jörg F . Wittenberger]

Hi folks,

if you really consider anything to be done to the argvector handling
before the next release, please have a look at the patch I posted
recently here
http://lists.nongnu.org/archive/html/chicken-hackers/2016-02/msg00082.html
not for the minor performance gain, read it for the re-structuring it does.

Do not yet apply.  It's outdated already due to conflicting changes in
cthulu (actually a minor change easy to resolve manually).  I'm happy to
produce a new one incorporating the lessons learned discussing this
issue here.

The main advantage of the patch in this situation is that it already
pulls all the argvector related allocations into two macros.  Easy to be
change to try what works best.  Since the first patch #defines them back
to C_alloc as before it leave you with binary compatible chicken.

If activated, the argvector is allocated afresh after garbage collection
and basically always re-used once it grew to the maximum size (the only
exception are where the compiler actually produces code using
C_kontinue) plus ...see below or look for comments containing "no
av-reuse" in the patch.  However I have not yet seen a case where the
c-backend could not be changed to call the C_kontinue_av passing the old
argvector too.

This reduces the odds of an argvector allocation to overflow the stack
to those cases where a large vector is allocated rarely by the
application (otherwise it would have had a high chance to have allocate
a large argvector shortly after gc, which did fit the stack and is still
reused).  So this is saying: I'd guess it mitigates the problem.

Am 26.02.2016 um 16:25 schrieb Peter Bex:
> On Fri, Feb 26, 2016 at 04:03:12PM +0100, address@hidden wrote:
>>> These two probably need to be fixed before we can do a release.
>>> The second was always broken but it might not have been such a big
>>> deal because we never allocated as much on the stack(?).  The first
>>> is new with argvectors.
>>
>> Unless one operates with native threads, the stack can be considered
>> "large enough" (IIRC, it even grows on demand on modern systems,
>> up to certain limits, but that may be wrong)
> 
> Yeah, but the C_stack_check will raise an exception if it grows beyond
> our arbitrarily defined soft limit.  See the Salmonella link in my mail.
> 
>>> Like I said, I'm a bit ill so this may all be just a fever dream, but
>>> the proper solution eludes me.  A fix for 2) might involve collecting
>>> all the possible function calls a generated function will perform and
>>> then pre-allocating the av2 (but only when needed), and then checking
>>> whether that would cause the stack to overflow.
>>
>> Do you mean 1) here? Since a CPS-procedure will call only a limited
>> amount of CPS functions (usually 1), this should be doable.

In the long run, I'd prefer this approach.  However I'd guess adding
another stack check would not do too much harm.  Especially not when
usually re-using the argvector.

It occurs to me that it would only be a minor change to
C_force_allocate_fresh_argvector
which is only called once the re-use tests already failed.

Instead of allocating with C_alloc and caching the value as default, it
would have to check the stack and if it does not fit flush the default
vector and return room from the temporary stack.

> Yeah, I probably meant 1).  Alternatively, we may make the allocation of
> the vector return a pointer into the C stack *or* into the temporary stack
> depending on whether there's still room in the C stack.

Something like this is already happening. Tagged "no av-reuse" in the diff.


> Then, fill the argvector with arguments and either call the function or
> call into the GC with the CPS closure just like we would do at the start
> of the CPS function.  The disadvantage is that we'd be doing one more
> stack check, the advantage is that when we call more CPS functions, we
> don't need to be pessimistic by using the maximum of the argument counts.

I don't understand this.

>>> A fix for 1), if I'm not mistaken, would be to swap the way the stack
>>> checks are done: the C_demand and C_stack_probe should observe the
>>> reserve, while C_stack_check1() should only look at the hard limit.
>>
>> (2?) I'm not sure whether I'm following completely, but the reserve should
>> probably respected in any case.
> 
> Yeah, but it should be consistently applied in both cases, I think.
> Currently the stack probe/demand at the start of a CPS function doesn't
> respect the reserve while the C_stack_check does respect it, causing
> these spurious "stack overflow" errors.

In the (very) long run, I'd wonder if it would be possible to define
that stack checks are done either on entry or at the call site, but not
both.

However this should not delay a release.

>>> Besides, I think C_stack_check1 looks too complicated; why does it
>>> create the temporary *_sp variable?  Is that to avoid a pointer
>>> overflow/underflow?  Couldn't it just do a check like this:
>>>
>>> if ((C_byte *)C_stack_pointer < ((C_byte *)C_stack_limit - 
>>> C_STACK_RESERVE)) err;
>>
>> I think this doesn't make much of a difference in terms of compilation.
>> The C compiler should take care of this.
> 
> The code is a bit hairy to look at, especially since it's a multi-line
> macro.  This suggestion was purely aesthetical.

If I was asked, I'd vote for your taste.


So please have a look at the diff.  I'll simply add a check and test it
in the meantime.  But I'd guess right now would be a good time to send
me suggestions regarding things you'd not like eventually.

Best

/Jörg