Check for conflicting @ccallable name before JIT registration

[topolarity]

Fixes #54878. For pkgimages with conflicting @ccallable definitions:

$ cat Foo/src/Foo.jl
module Foo
Base.@ccallable function foo()::Cint
    return 0
end
end
$ cat Bar/src/Bar.jl
module Bar
Base.@ccallable function foo()::Cint
    return 0
end
end

This turns the existing segfault into a warning, as on 1.9:

julia> using Foo, Bar
WARNING: @ccallable was already defined for this method name

[vchuravy]

Should we instead turn it into a proper error? Especially since we are deleting an assert?

[topolarity]

Should we instead turn it into a proper error?

I think we should do that, but also make @ccallable register w/ the JIT much less eagerly.

These functions aren't actually available to dlsym etc., so I'm not sure why/whether we even need to do this JIT registration unless you're actually in imaging mode (and also trying to build a monolithic sysimage?)

Especially since we are deleting an assert?

The assert was just wrong - it assumed there is only one (pkg/sys)image loaded system-wide and therefore naming conflicts cannot occur.

[vchuravy]

These functions aren't actually available to

I think so that

julia/test/llvmcall.jl

Lines 154 to 177 in c3af4fc

	@ccallable Cvoid function jl_the_callback()
	global didcall
	didcall = true
	nothing
	end
	@test_throws(ErrorException("@ccallable was already defined for this method name"),
	@eval @ccallable Cvoid jl_the_callback(not_the_method::Int) = "other")
	# Make sure everything up until here gets compiled
	@test jl_the_callback() === nothing
	@test jl_the_callback(1) == "other"
	didcall = false
	function do_the_call()
	llvmcall(
	("""declare void @jl_the_callback()
	define void @entry() #0 {
	0:
	call void @jl_the_callback()
	ret void
	}
	attributes #0 = { alwaysinline }
	""", "entry"),Cvoid,Tuple{})
	end
	do_the_call()
	@test didcall

work. Which you can use for other fun shenanigans.

[topolarity]

Hmm yeah - feels like that behavior ought to be opt-in though

[vchuravy]

julia> Base.@ccallable Cvoid function jl_the_callback()
                global didcall
                didcall = true
                nothing
            end

julia> function call()
           ccall("extern jl_the_callback", llvmcall, Nothing, ())
       end
call (generic function with 1 methods)

julia> call()

julia> didcall
true

Another variant how this is used (since I did encounter this myself again)

julia> Base.@ccallable Cvoid function jl_the_callback()
                global didcall
                didcall = true
                nothing
            end

julia> Base.@ccallable Cvoid function jl_the_callback()
                global didcall
                didcall = true
                nothing
            end
ERROR: @ccallable was already defined for this method name
Stacktrace:
 [1] _ccallable(rt::Type, sigt::Type)
   @ Base ./c.jl:513
 [2] macro expansion
   @ c.jl:544 [inlined]
 [3] top-level scope
   @ REPL[2]:1

Really, that's the same error we want to give here, and that the error you would get in a systemimage.
Of course with pkgimages this is annoying since we mostly are done loading.

[topolarity]

Of course with pkgimages this is annoying since we mostly are done loading.

Throwing the error isn't really the annoying part imo - it's that it's not always avoiding a real problem:

1. If you just wanted a ccallable for PackageCompiler workflows, then having a warning / error is annoying because your code works just fine without the ccallable (but you have no way to turn it off)
2. If you wanted a ccallable for llvmcall then you're forced to fight over a single namespace, which is only composable with a lot of user discipline to include unique prefixes.

Which is why I think it might be better to separate these two use cases

[topolarity]

For my use case, I wanted to test side-by-side two versions of a package for use with PackageCompiler.

I ended up writing a terrible wrapper to save all the ccallable registrations, and then defer their registration until an explicit point in time that I can activate in the PackageCompiler build

Otherwise it was impossible to import both modules without warnings / errors

[vchuravy]

If you wanted a ccallable for llvmcall then you're forced to fight over a single namespace, which is only composable with a lot of user discipline to include unique prefixes.

Yeah, but that is the universe we live in. Even if I want those packages for use in PackageCompiler I need to use prefixes to ensure that my names are unique.

The question is "could we come up with a better namespace strategy than the user"? Maybe, we could automatically add the package (module?, FQN?) as a prefix. Even for your use-case you likely had to do some shenaigans like changing the UUID and renaming a package to MyPackagev1 for both version to be loaded into the same Julia process at the same time.

[topolarity]

Even if I want those packages for use in PackageCompiler I need to use prefixes to ensure that my names are unique.

Yeah, for PackageCompiler you're specifically exporting for the linker, so we're forced to accept the realities of C namespacing

But only when you're actually building the sysimage - so for my use case, I can load vA and vB just fine in an interactive session

The question is "could we come up with a better namespace strategy than the user"? Maybe, we could automatically add the package (module?, FQN?) as a prefix.

I think it's pretty much equivalent to the gensym problem - but I have to admit I don't know how much we try to make gensym unique across pkgimages