Hi Peter, This makes sense. I didn't know that was ever done, but after a search of the rest of the runtime it doesn't *look* like this temporary stack trick is used anywhere else. Nice find! I haven't been able to reproduce a crash since applying these fixes on a handful of platforms.
Just two minor things: (1) it looks like one too many words is allocated for the C_apply_values argvector, and (2) as a small performance tweak, we can avoid inspecting all the pair headers a second time during C_apply* by reusing the list's length when copying it into the argvector (since we already know its length (and that it's a regular list) by that point). See the attached patches. It's also a bit silly to shift, then immediately unshift the C_u_i_length results, but that probably doesn't matter much overall. If you're happy with those two changes, I'll push the whole lot. Then we can see a beautiful, error-free salmonella run tonight. :) Evan
>From 705b28563f317d27ba09776b37f68233bac9b4a1 Mon Sep 17 00:00:00 2001 From: Evan Hanson <[email protected]> Date: Sun, 4 Oct 2015 20:09:19 +1300 Subject: [PATCH 1/2] Fix (harmless) off-by-one in C_apply_values --- runtime.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/runtime.c b/runtime.c index c981963..102fa42 100644 --- a/runtime.c +++ b/runtime.c @@ -6107,8 +6107,7 @@ void C_ccall C_apply_values(C_word c, C_word *av) C_word /* closure = av[ 0 ] */ k = av[ 1 ], - lst, - n; + lst, len, n; if(c != 3) C_bad_argc(c, 3); @@ -6117,16 +6116,17 @@ void C_ccall C_apply_values(C_word c, C_word *av) if(lst != C_SCHEME_END_OF_LIST && (C_immediatep(lst) || C_block_header(lst) != C_PAIR_TAG)) barf(C_BAD_ARGUMENT_TYPE_ERROR, "apply", lst); - /* Check continuation wether it receives multiple values: */ + /* Check whether continuation receives multiple values: */ if(C_block_item(k, 0) == (C_word)values_continuation) { C_word *av2, *ptr; - n = C_unfix(C_u_i_length(lst)) + 1; + len = C_unfix(C_u_i_length(lst)); + n = len + 1; if(!C_demand(n)) C_save_and_reclaim((void *)C_apply_values, c, av); - av2 = C_alloc(n + 1); + av2 = C_alloc(n); av2[ 0 ] = k; ptr = av2 + 1; while(!C_immediatep(lst) && C_block_header(lst) == C_PAIR_TAG) { -- 2.5.3
>From 52412d6a14d779dc34012db1993f71376ea5b873 Mon Sep 17 00:00:00 2001 From: Evan Hanson <[email protected]> Date: Sun, 4 Oct 2015 20:25:56 +1300 Subject: [PATCH 2/2] Loop to known list length when copying args into av during C_apply* This avoids some unnecessary bit-twiddling, since we already know the length of the list to copy. --- runtime.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/runtime.c b/runtime.c index 102fa42..10d70f7 100644 --- a/runtime.c +++ b/runtime.c @@ -5965,7 +5965,7 @@ void C_ccall C_apply(C_word c, C_word *av) fn = av[ 2 ]; int av2_size, i, n = c - 3; int non_list_args = n - 1; - C_word lst, *ptr, *av2; + C_word lst, len, *ptr, *av2; if(c < 4) C_bad_min_argc(c, 4); @@ -5976,7 +5976,8 @@ void C_ccall C_apply(C_word c, C_word *av) if(lst != C_SCHEME_END_OF_LIST && (C_immediatep(lst) || C_block_header(lst) != C_PAIR_TAG)) barf(C_BAD_ARGUMENT_TYPE_ERROR, "apply", lst); - av2_size = 2 + non_list_args + C_unfix(C_u_i_length(lst)); + len = C_unfix(C_u_i_length(lst)); + av2_size = 2 + non_list_args + len; if(!C_demand(av2_size)) C_save_and_reclaim((void *)C_apply, c, av); @@ -5990,7 +5991,7 @@ void C_ccall C_apply(C_word c, C_word *av) ptr += non_list_args; } - while(!C_immediatep(lst) && C_block_header(lst) == C_PAIR_TAG) { + while(len--) { *(ptr++) = C_u_i_car(lst); lst = C_u_i_cdr(lst); } @@ -6129,7 +6130,7 @@ void C_ccall C_apply_values(C_word c, C_word *av) av2 = C_alloc(n); av2[ 0 ] = k; ptr = av2 + 1; - while(!C_immediatep(lst) && C_block_header(lst) == C_PAIR_TAG) { + while(len--) { *(ptr++) = C_u_i_car(lst); lst = C_u_i_cdr(lst); } -- 2.5.3
_______________________________________________ Chicken-hackers mailing list [email protected] https://lists.nongnu.org/mailman/listinfo/chicken-hackers
