Only add symbols that are explicit entrypoints when building juliac image #56681
Conversation
| for (size_t i = 0; i < jl_entrypoint_mis->len ; i++) { | ||
| mi = (jl_method_instance_t*)jl_entrypoint_mis->items[i]; |
There was a problem hiding this comment.
One worry I have here is that there is the potential to break user-code that depends on this behavior.
That code may be rare, but I have seen it in the wild.
There was a problem hiding this comment.
This just affects trimmed binaries, so I'm slightly less worried.
There was a problem hiding this comment.
I do think it's worth considering a policy change and making the flag --ignore-ccallable, rather than ignoring @ccallable by default unless you pass --compile-ccallable
But that's orthogonal versus this fix, which at least properly implements the current policy correctly
There was a problem hiding this comment.
I think we should stick to default-exclude for everything with trimming; it will keep it easier to use.
| jl_method_instance_t* mi = (jl_method_instance_t*)jl_entrypoint_mis->items[i]; | ||
| assert(jl_is_method_instance(mi)); | ||
| jl_method_t *ccallable = mi->def.method; | ||
| if (ccallable == m) { |
There was a problem hiding this comment.
There's a corner case here where the MI != the @ccallable signature, which would still lead to a crash I think.
Can we check for that?
There was a problem hiding this comment.
I think we already assert that.
https://github.com/JuliaLang/julia/blob/fb02e641066fe423c34ee8e4fec24b965662ce85/src/precompile_utils.c#L370-L394
There was a problem hiding this comment.
I can add a comment that mentions that though
There was a problem hiding this comment.
I don't see where that's asserted there - Can you spell it out for me?
There was a problem hiding this comment.
static void *jl_precompile_trimmed(size_t world)
{
// array of MethodInstances and ccallable aliases to include in the output
jl_array_t *m = jl_alloc_vec_any(0);
jl_value_t *ccallable = NULL;
JL_GC_PUSH2(&m, &ccallable);
jl_method_instance_t *mi;
for (size_t i = 0; i < jl_entrypoint_mis->len ; i++) {
mi = (jl_method_instance_t*)jl_entrypoint_mis->items[i];
if (mi == NULL)
break;
assert(jl_is_method_instance(mi));
jl_array_ptr_1d_push(m, (jl_value_t*)mi);
ccallable = (jl_value_t *)mi->def.method->ccallable; // If the method passed into jl_entrypoint has ccallable we always compile it.
if (ccallable)
jl_array_ptr_1d_push(m, ccallable);
}
jl_cgparams_t params = jl_default_cgparams;
params.trim = jl_options.trim;
void *native_code = jl_create_native(m, NULL, ¶ms, 0, /* imaging */ 1, 0,
world, NULL);
JL_GC_POP();
return native_code;
}There was a problem hiding this comment.
Where does that assert that the entrypoint MI == the ccallable signature?
There was a problem hiding this comment.
It means if we are compiling the entrypoint and if the method of that entrypoint has a ccallable defined on it, we will add both the entrypoint MI and the ccallable signature (it's an svec). We can maybe compare ccallables instead of methods.
There was a problem hiding this comment.
That's fine, but we shouldn't be imprecise about checking the entrypoint just because we happen to enqueue the ccallables in that way right now
It makes it buggy if we ever try to add entrypoints without the corresponding ccallable in the future
This currently just does a linear search over the items. We might want to change this datastructure to a hash table at some point, but given that I don't think libraries have too too many entrypoints this linear search should be fast enough.