Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

staticdata: remove reinit_ccallable #56987

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Jan 8, 2025

This commit removes jl_reinit_ccallable whose purpose is unclear.
Specifically, the function re-adds C-callable functions to the execution
engine during the loading of sysimages/pkgimages, but the consensus is
that the function is largely unnecessary because ccall can find
symbols via dlsym. The function's only apparent use case is a
contrived llvmcall example, only because we currently don't add the
sysimage symbols to the JIT, but we could do anyway. llvmcall has
always been experimental, and if it is truly needed, the functionality
for finding the symbols should be properly implemented later.

@aviatesk aviatesk requested review from vtjnash and timholy and removed request for vtjnash January 8, 2025 04:36
@vchuravy
Copy link
Member

vchuravy commented Jan 8, 2025

We might be missing test coverage, but I think PkgEval found this back then ...

Generally speaking a package that has a @ccallable and then a user who uses ccall to call it by name.

@gbaraldi
Copy link
Member

gbaraldi commented Jan 8, 2025

I believe @topolarity touched this last, (I might be imagining it). But still we need this functionality. Otherwise what restores the ccallable pointer?

@vchuravy
Copy link
Member

vchuravy commented Jan 8, 2025

We have a test here

Base.@ccallable Cint f35014(x::Cint) = x+Cint(1)
&
@eval begin function ccallable_test()

And CI does complain: "JIT session error: Symbols not found: [ f35014 ]"

Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

This code (or equivalent) is still needed as seen on CI.

@topolarity
Copy link
Member

I believe @topolarity touched this last

Yeah, I was fixing a bug in this space in #55813 but I don't think it affects this PR much

@vtjnash
Copy link
Member

vtjnash commented Jan 8, 2025

Generally speaking a package that has a @ccallable and then a user who uses ccall to call it by name.

@vtjnash
Copy link
Member

vtjnash commented Jan 8, 2025

The test is for llvmcall, which is all experimental and can therefore just be deleted

@Keno
Copy link
Member

Keno commented Jan 8, 2025

Can you elaborate of what the difficulty of #56499 with jl_reinit_ccallable is? If we have a test for it that does raise the likelyhood that people are depending on it.

@vtjnash
Copy link
Member

vtjnash commented Jan 8, 2025

I fixed the test for CI, so that it uses the stable ccall interface instead of the unstable llvmcall one

@aviatesk aviatesk force-pushed the avi/rm-reinit_ccallable branch from 01bb043 to 5040e48 Compare January 13, 2025 09:18
@aviatesk
Copy link
Member Author

@vchuravy We had a local discussion and reached a consensus that this functionality can be excluded without any issues. For detailed reasoning, please refer to the commit message. If there are no objections, I'd appreciate if you could remove the block. Thanks!

@vchuravy
Copy link
Member

This breaks a feature I at least have used several times in research projects. I have used this to write self-hosted runtimes for Tapir and RMA based RPCs, which both had LLVM based interface and this feature allowed me to write those runtimes in Julia instead of C.

A related case is in GPUCompiler where we use a @ccallable + ccall("extern fn", llvmcall, ...) https://github.com/JuliaGPU/GPUCompiler.jl/blob/a9b5a8263e9bf1051395840b63b0c10fd503d8de/src/driver.jl#L137
This variant is also used by Enzyme.

So my objection stands.

@gbaraldi
Copy link
Member

Should we use ORCs library handling to do this registering instead? I believe we can filter as needed i.e something like this

julia/src/jitlayers.cpp

Lines 2010 to 2022 in a23a6de

if (libatomic) {
static void *atomic_hdl = jl_load_dynamic_library(libatomic, JL_RTLD_LOCAL, 0);
if (atomic_hdl != NULL) {
GlobalJD.addGenerator(
cantFail(orc::DynamicLibrarySearchGenerator::Load(
libatomic,
DL.getGlobalPrefix(),
[&](const orc::SymbolStringPtr &S) {
const char *const atomic_prefix = "__atomic_";
return (*S).starts_with(atomic_prefix);
})));
}
}
?

This commit removes `jl_reinit_ccallable` whose purpose is unclear.
Specifically, the function re-adds C-callable functions to the execution
engine during the loading of sysimages/pkgimages, but the consensus is
that the function is largely unnecessary because `ccall` can find
symbols via `dlsym`. The function's only apparent use case is a
contrived `llvmcall` example, only because we currently don't add the
sysimage symbols to the JIT, but we could do anyway. `llvmcall` has
always been experimental, and if it is truly needed, the functionality
for finding the symbols should be properly implemented later.
@aviatesk aviatesk force-pushed the avi/rm-reinit_ccallable branch from 5040e48 to 9a5ac56 Compare January 15, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants