fix: duplicate global constants

[ymardoukhi]

See issue #2706

This PR should resolve the issue when global constants with private linkage get external linkage if the LLVM IR is called via ccall interface. See the same issue above for detailed examination.

[github-actions[bot]]

Your PR requires formatting changes to meet the project's style guidelines. Please consider running Runic (git runic main) to apply these changes.

Click here to view the suggested changes.

diff --git a/test/global_constants.jl b/test/global_constants.jl
index 04c22700..64003d42 100644
--- a/test/global_constants.jl
+++ b/test/global_constants.jl
@@ -27,7 +27,7 @@ tmp_so_file = joinpath(tmp_dir, "func.so")
 run(
     pipeline(
         `clang -x ir - -Xclang -no-opaque-pointers -O3 -fPIC -fembed-bitcode -shared -o $(tmp_so_file)`;
-        stdin=IOBuffer(LLVM_IR)
+        stdin = IOBuffer(LLVM_IR)
     )
 );
 lib = Libdl.dlopen(tmp_so_file);
@@ -36,8 +36,9 @@ const fptr = Libdl.dlsym(lib, :func);
 
 function func_llvm(x::Float64, y::Float64, n::Int)
     n >= 0 && n <= 2 || throw("0 ≤ n ≤ 2")
-    Base.llvmcall((LLVM_IR, "func"), Cdouble,
-        Tuple{Cdouble,Cdouble,Clong},
+    return Base.llvmcall(
+        (LLVM_IR, "func"), Cdouble,
+        Tuple{Cdouble, Cdouble, Clong},
         x, y, n
     )
 end;
@@ -45,7 +46,8 @@ end;
 
 function func_ccall(x::Float64, y::Float64, n::Int)
     n >= 0 && n <= 2 || throw("0 ≤ n ≤ 2")
-    ccall(fptr, Cdouble,
+    return ccall(
+        fptr, Cdouble,
         (Cdouble, Cdouble, Clong),
         x, y, n
     )
@@ -59,9 +61,8 @@ end;
     A = [1.0, 2.0, 3.0]
 
     @test func_llvm(x, y, n) == func_ccall(x, y, n)
-    @test func_llvm(x, y, n) == x * A[n+1] + y
-    @test func_ccall(x, y, n) == x * A[n+1] + y
-
+    @test func_llvm(x, y, n) == x * A[n + 1] + y
+    @test func_ccall(x, y, n) == x * A[n + 1] + y
 
 
     @test gradient(Reverse, func_llvm, Const(x), y, Const(n)) == (nothing, 1.0, nothing)

[vchuravy]

So one challenge here is that we are hitting different language semantics. Julia code doesn't use LLVM globals as much, except for compilation unit local operations.

In C/C++ that is not the case and I have to answer the question if a global variable with the same name ought to be unifed or not. This is AD independent and is normally handled by the dynamic linker, here we are consuming LLVM IR from before the dynamic linker phase and I think we must met the semantics.

[ymardoukhi]

So one challenge here is that we are hitting different language semantics. Julia code doesn't use LLVM globals as much, except for compilation unit local operations.

In C/C++ that is not the case and I have to answer the question if a global variable with the same name ought to be unifed or not. This is AD independent and is normally handled by the dynamic linker, here we are consuming LLVM IR from before the dynamic linker phase and I think we must met the semantics.

That clarifies the intent. Thanks!

But what confused me earlier was that no matter how I defined the linkage in my LLVM IR, it was overwritten when the llvmbc was loaded from the shared object. I think it must be the user's sole responsibility to make sure how the globals are handled? If I used Base.llvmcall, I could see that the LLVM IR remained unaltered.

[wsmoses]

So one challenge here is that we are hitting different language semantics. Julia code doesn't use LLVM globals as much, except for compilation unit local operations. In C/C++ that is not the case and I have to answer the question if a global variable with the same name ought to be unifed or not. This is AD independent and is normally handled by the dynamic linker, here we are consuming LLVM IR from before the dynamic linker phase and I think we must met the semantics.

That clarifies the intent. Thanks!

But what confused me earlier was that no matter how I defined the linkage in my LLVM IR, it was overwritten when the llvmbc was loaded from the shared object. I think it must be the user's sole responsibility to make sure how the globals are handled? If I used Base.llvmcall, I could see that the LLVM IR remained unaltered.

yeah so I think probably the thing to do is to mark it as external, but remove initialization assuming it will be initialized by the actual library itself [therefore preserving the one definiton, and not overwriting]. alternatively we can link it weakly/etc

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 50.87%. Comparing base (986bbf4) to head (8a14560).

:exclamation: There is a different number of reports uploaded between BASE (986bbf4) and HEAD (8a14560). Click for more details.

HEAD has 13 uploads less than BASE

Flag	BASE (986bbf4)	HEAD (8a14560)
	18	5

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2735       +/-   ##
===========================================
- Coverage   68.91%   50.87%   -18.05%     
===========================================
  Files          58       13       -45     
  Lines       19861     1256    -18605     
===========================================
- Hits        13688      639    -13049     
+ Misses       6173      617     -5556     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.