Copy global variable linkages when moving functions

[jonathanvdc]

Hi! I'd like to propose a one-line change to the GlobalVariable copying logic that is invoked when llvmcall is passed a pointer to an LLVM function. Currently, any global variables that are used by such a function are copied to the module that performs the llvmcall. However, the current global variable copying logic discards linkage information by making all copied globals external.

It'd be really nice if linkage information didn't get discarded—the current semantics are not what llvmcall users seem to expect, judging from, e.g., this line of code in CUDAnative. The change I'm proposing is to copy global variable linkages when copying global variables. Is that okay?

[Keno]

This seems fine to me, though I have no particular frame of reference. I guess we should try Cxx.jl and see if that still works, since the way this code was originally written was to try that and see if it works.

[StefanKarpinski]

I guess we should try Cxx.jl and see if that still works

By "we" I assume you mean "you" since I doubt anyone else is able to do that right now 😁

[Keno]

Actually, I haven't tried Cxx.jl in quite some time - there's some other folks actively using it though, but yes, I could try it, though I'm unlikely to get to it for another couple days.

[vchuravy]

So I gave it a whirl and Cxx passes on master, but fails on this branch with:

julia: /home/vchuravy/src/julia/src/jitlayers.cpp:323: void JuliaOJIT::DebugObjectRegistrar::registerObject(RTDyldObjHandleT, const ObjT&, const LoadResult&) [with ObjT = llvm::object::ObjectFile*; LoadResult = const llvm::RuntimeDyld::LoadedObjectInfo*; RTDyldObjHandleT = std::_List_iterator<std::unique_ptr<llvm::orc::RTDyldObjectLinkingLayerBase::LinkedObject> >]: Assertion `Sym' failed.

signal (6): Aborted
in expression starting at /home/vchuravy/.julia/dev/Cxx/test/misc.jl:206
gsignal at /usr/lib/libc.so.6 (unknown line)
abort at /usr/lib/libc.so.6 (unknown line)
__assert_fail_base.cold.0 at /usr/lib/libc.so.6 (unknown line)
__assert_fail at /usr/lib/libc.so.6 (unknown line)
registerObject<llvm::object::ObjectFile*, const llvm::RuntimeDyld::LoadedObjectInfo*> at /home/vchuravy/src/julia/src/jitlayers.cpp:323
_M_invoke at /home/vchuravy/src/julia/src/jitlayers.cpp:335
operator() at /usr/include/c++/8.2.1/bits/std_function.h:687 [inlined]
operator() at /home/vchuravy/builds/julia/usr/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h:272 [inlined]
finalize at /home/vchuravy/builds/julia/usr/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h:144
emitAndFinalize at /home/vchuravy/builds/julia/usr/include/llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h:343 [inlined]
emitAndFinalize at /home/vchuravy/builds/julia/usr/include/llvm/ExecutionEngine/Orc/IRCompileLayer.h:91 [inlined]
addModule at /home/vchuravy/src/julia/src/jitlayers.cpp:465
jl_add_to_ee at /home/vchuravy/src/julia/src/jitlayers.cpp:687 [inlined]
jl_finalize_function at /home/vchuravy/src/julia/src/jitlayers.cpp:695
getAddressForFunction at /home/vchuravy/src/julia/src/codegen.cpp:1283
jl_generate_fptr at /home/vchuravy/src/julia/src/codegen.cpp:1390
jl_fptr_trampoline at /home/vchuravy/src/julia/src/gf.c:1894
jl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2250
do_call at /home/vchuravy/src/julia/src/interpreter.c:323
eval_value at /home/vchuravy/src/julia/src/interpreter.c:411
eval_stmt_value at /home/vchuravy/src/julia/src/interpreter.c:362 [inlined]
eval_body at /home/vchuravy/src/julia/src/interpreter.c:754
eval_body at /home/vchuravy/src/julia/src/interpreter.c:699
jl_interpret_toplevel_thunk_callback at /home/vchuravy/src/julia/src/interpreter.c:884
unknown function (ip: 0xfffffffffffffffe)
unknown function (ip: 0x7f04e2f6f90f)
unknown function (ip: 0x2)
jl_interpret_toplevel_thunk at /home/vchuravy/src/julia/src/interpreter.c:893
jl_toplevel_eval_flex at /home/vchuravy/src/julia/src/toplevel.c:797
jl_parse_eval_all at /home/vchuravy/src/julia/src/ast.c:873
jl_load at /home/vchuravy/src/julia/src/toplevel.c:859
include at ./boot.jl:325 [inlined]
include_relative at ./loading.jl:1041
include at ./Base.jl:29
jl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2250
include at ./client.jl:443
jl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2250
do_call at /home/vchuravy/src/julia/src/interpreter.c:323
eval_value at /home/vchuravy/src/julia/src/interpreter.c:411
eval_stmt_value at /home/vchuravy/src/julia/src/interpreter.c:362 [inlined]
eval_body at /home/vchuravy/src/julia/src/interpreter.c:754
jl_interpret_toplevel_thunk_callback at /home/vchuravy/src/julia/src/interpreter.c:884
unknown function (ip: 0xfffffffffffffffe)
unknown function (ip: 0x7f04e2dca18f)
unknown function (ip: 0xffffffffffffffff)
jl_interpret_toplevel_thunk at /home/vchuravy/src/julia/src/interpreter.c:893
jl_toplevel_eval_flex at /home/vchuravy/src/julia/src/toplevel.c:797
jl_parse_eval_all at /home/vchuravy/src/julia/src/ast.c:873
jl_load at /home/vchuravy/src/julia/src/toplevel.c:859
include at ./boot.jl:325 [inlined]
include_relative at ./loading.jl:1041
include at ./Base.jl:29
jl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2250
exec_options at ./client.jl:307
_start at ./client.jl:476
jl_apply_generic at /home/vchuravy/src/julia/src/gf.c:2250
jl_apply at /home/vchuravy/src/julia/ui/../src/julia.h:1594 [inlined]
true_main at /home/vchuravy/src/julia/ui/repl.c:96
main at /home/vchuravy/src/julia/ui/repl.c:217
__libc_start_main at /usr/lib/libc.so.6 (unknown line)
_start at /home/vchuravy/builds/julia/julia (unknown line)
Allocations: 31600808 (Pool: 31594801; Big: 6007); GC: 74

So Cxx.jl will need some adjustment (sadly I don't have time to play around with it),l I found this old thing, but I didn't try it out https://github.com/Keno/Cxx.jl/blob/c62cb578ebde43d040e3f175ea2365b292c6f487/src/bootstrap.cpp#L1729

[jonathanvdc]

Thanks! I restored those commented-out lines and sent a PR to Cxx.jl. I'm still waiting for the CI builds to come back, but the changes in that PR seem to be sufficient to make Cxx.jl's tests pass on my machine, even when run by a version of Julia compiled from this PR.

[jonathanvdc]

Hi everyone. It's been a while since I sent this PR and the accompanying PR for Cxx.jl. The latter works locally for me but the CI builds are broken by what I think is bitrot: routine steps of the CI build process are failing. The Linux CI builds fail because a download 404s; the macOS CI builds try to delete folders without the appropriate permissions.

I'm not sure how to proceed here. I think the change I'm proposing in this PR—to not overwrite global linkages—is reasonable. I'm working on a fork of CUDAnative that includes a garbage collector and said fork depends on this PR, among other things. I'd like to upstream my changes eventually, but I can't realistically expect for that to happen until my fork runs on Julia's master branch.

Is there anything I can do to get this PR merged in?

[vchuravy]

This change sounds reasonable for me. Can you rebase (so that we get current CI) and maybe add a test (even though I don't quite know how to test this.)

[jonathanvdc]

Sure! I rebased and added a test. I'm still waiting for the CI results to come back for the commit where I added the test, but the CI results for my previous commit seem promising.

[jonathanvdc]

Thanks for the approval!

The buildbot CI builds seem to succeed, but the Travis and AppVeyor builds are failing due to unsatisfiable package requirements in the Pkg tests. I reckon those are unrelated to my PR because the same errors also appear in the CI builds for the latest master branch commit. Is there anything I can do to fix the failing AppVeyor and Travis CI builds for this PR? Should I rebase again?

[vchuravy]

@Keno any objections to merge this?