Skip to content

Commit

Permalink
fix #9883, #10085
Browse files Browse the repository at this point in the history 
apparently some compilers don't like my previous attempts to generate conditional alloca gcframes
  • Loading branch information
@vtjnash
vtjnash committed Feb 8, 2015
1 parent 470cacb commit f769e41
Showing 1 changed file with 4 additions and 5 deletions.
9 changes: 4 additions & 5 deletions src/builtins.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,15 +432,14 @@ JL_CALLABLE(jl_f_apply)
}
}
jl_value_t **newargs;
if (n > jl_page_size/sizeof(jl_value_t*)) {
int onstack = (n < jl_page_size/sizeof(jl_value_t*));
JL_GC_PUSHARGS(newargs, onstack ? n : 1);
if (!onstack) {
// put arguments on the heap if there are too many
jl_value_t *argarr = (jl_value_t*)jl_alloc_cell_1d(n);
JL_GC_PUSH1(&argarr);
newargs[0] = argarr;
newargs = jl_cell_data(argarr);
}
else {
JL_GC_PUSHARGS(newargs, n);
}
n = 0;
for(i=1; i < nargs; i++) {
if (jl_is_tuple(args[i])) {
Expand Down

3 comments on commit f769e41

@tkelman
Copy link
Contributor

@tkelman tkelman commented on f769e41 Feb 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On release-0.3, this looks like

    if (n > jl_page_size/sizeof(jl_value_t*)) {
        // put arguments on the heap if there are too many
        argarr = (jl_value_t*)jl_alloc_cell_1d(n);
        newargs = jl_cell_data(argarr);
    }
    else {
        newargs = (jl_value_t**)alloca(n * sizeof(jl_value_t*));
    }

Should completely replacing the above with the new

    jl_value_t **newargs;
    int onstack = (n < jl_page_size/sizeof(jl_value_t*));
    JL_GC_PUSHARGS(newargs, onstack ? n : 1);
    if (!onstack) {
        // put arguments on the heap if there are too many
        jl_value_t *argarr = (jl_value_t*)jl_alloc_cell_1d(n);
        newargs[0] = argarr;
        newargs = jl_cell_data(argarr);
    }

work correctly?

@vtjnash
Copy link
Member Author

@vtjnash vtjnash commented on f769e41 Feb 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not quite, there's another JL_GC_PUSH1(&t); above that needs to move, likely by cherry-picking 69b84e9 first

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another big grab-bag commit that doesn't backport cleanly. I don't know the right way to resolve any of these conflicts. Can you make PR's within the next few days for anything you think should be backported for 0.3.6 but hasn't been yet?

Please sign in to comment.