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

math.pi causes linker errors #2129

Closed
certik opened this issue Jul 9, 2023 · 13 comments
Closed

math.pi causes linker errors #2129

certik opened this issue Jul 9, 2023 · 13 comments
Labels
bug Something isn't working

Comments

@certik
Copy link
Contributor

certik commented Jul 9, 2023

This global variables in math.py:

from lpython import i8, i16, i32, f32, i64, f64, ccall, overload
                                     
          
pi: f64 = 3.141592653589793238462643383279502884197
e: f64 = 2.718281828459045235360287471352662497757
tau: f64 = 6.283185307179586        

Cause the following linker errors using the C backend if compiling and linking more than one file:

duplicate symbol '_pi_64' in:
    CMakeFiles/a.dir/a.c.o
    b/b.mod.a(b.c.o)
duplicate symbol '_pi_32' in:
    CMakeFiles/a.dir/a.c.o
    b/b.mod.a(b.c.o)
ld: 2 duplicate symbols for architecture arm64
@certik certik added bug Something isn't working BLOCKER workaround not found yet labels Jul 9, 2023
@certik
Copy link
Contributor Author

certik commented Jul 9, 2023

This was caused by the (correct) fix not to use C macros but global variables. But they now clash.

@certik
Copy link
Contributor Author

certik commented Jul 9, 2023

I think we need to make the names unique to a translation unit, similarly to what we did in LFortran recently. We need to port the solution from here: lfortran/lfortran#1898 and use it for these global variables. Update: implemented in #2138.

@certik
Copy link
Contributor Author

certik commented Jul 10, 2023

I think in the C backend we need to use a global map (using a unique hash) where we save the actual (unique) name, and look it up. Similarly how we handle similar things in the LLVM backend.

@Smit-create
Copy link
Collaborator

Can you please add a small reproducible example? Is this something related to #2076?

@Shaikh-Ubaid
Copy link
Collaborator

I am probably able to reproduce it:

$ cat a.py 
from lpython import f64
from math import pi

def area_of_circle(r: f64) -> f64:
    return pi * r * r
$ cat b.py
from lpython import f64
from math import pi

def main0():
    print("hi")

main0()
$ lpython --show-c a.py --disable-main > main1.c
$ lpython --show-c b.py  > main2.c              
$ gcc -c main1.c -I src/libasr/runtime                         
$ gcc -c main2.c -I src/libasr/runtime                         
$ gcc main1.o main2.o -o my_app -Lsrc/runtime -llpython_runtime
duplicate symbol '_tau' in:
    main1.o
    main2.o
duplicate symbol '_pi' in:
    main1.o
    main2.o
duplicate symbol '_e' in:
    main1.o
    main2.o
ld: 3 duplicate symbols for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@certik
Copy link
Contributor Author

certik commented Jul 10, 2023

@Shaikh-Ubaid yes, exactly. I was going to post the same thing. We use this to compile a larger project, file by file, to C, then we link everything at the end.

The solution is to make these global variables have a unique name, using lcompilers_unique_ID from #2138.

LFortran does this for some of the generated functions, but only when --generate-object-code is used. We could also hide this behind an option, such as --separate-compilation or something like that, if needed.

@Smit-create
Copy link
Collaborator

This would also fail for functions and subroutines:

% cat a.py 
from lpython import f64

def f1(r: f64) -> f64:
    return abs(r-1.0)
% cat b.py 
from lpython import f64

def main0():
    x: f64 = 3.0
    y: f64 = abs(x-6.0)
    print("hi")

main0()

@certik
Copy link
Contributor Author

certik commented Jul 10, 2023

I think we need a --separate-compilation flag, which will make all these symbols unique for each translation unit. Otherwise they are not unique.

@certik
Copy link
Contributor Author

certik commented Jul 10, 2023

A quick workaround:

diff --git a/src/runtime/lpython_intrinsic_numpy.py b/src/runtime/lpython_intrinsic_numpy.py
index 17243dd99..5d5aebcbd 100644
--- a/src/runtime/lpython_intrinsic_numpy.py
+++ b/src/runtime/lpython_intrinsic_numpy.py
@@ -1,7 +1,5 @@
 from lpython import i32, i64, f64, f32, ccall, vectorize, overload
 
-pi_64: f64 = f64(3.141592653589793238462643383279502884197)
-pi_32: f32 = f32(3.141592653589793238462643383279502884197)
 
 ########## sin ##########
 
@@ -271,27 +269,7 @@ def arctan(x: f32) -> f32:
 
 ########## degrees ##########
 
-@overload
-@vectorize
-def degrees(x: f64) -> f64:
-    return x*180.0/pi_64
-
-@overload
-@vectorize
-def degrees(x: f32) -> f32:
-    return x*f32(f32(180)/pi_32)
-
-########## radians ##########
 
-@overload
-@vectorize
-def radians(x: f64) -> f64:
-    return x*pi_64/180.0
-
-@overload
-@vectorize
-def radians(x: f32) -> f32:
-    return x*f32(pi_32/f32(180))
 
 ########## arcsinh ##########
 

This works.

certik added a commit to certik/lpython that referenced this issue Jul 10, 2023
Due to lcompilers#2129, the global variables do not always work well. Until that
issue is fixed, a simple workaround is to move these global variables
into local variables.

Reference tests updated.
@certik certik removed the BLOCKER workaround not found yet label Jul 10, 2023
@certik
Copy link
Contributor Author

certik commented Jul 10, 2023

So the workaround in #2143 makes this issue not a blocker anymore.

However, it can become again at any time, until we have a proper fix.

To fix this, let's implement a nice ASR pass, as discussed in #2142.

@certik
Copy link
Contributor Author

certik commented Jul 17, 2023

@Smit-create how do I use the PR #2149 here?

Here is an example:

from math import pi

def f():
    print(pi)

And I tried:

lpython --separate-compilation --global-mangling --show-c a.py
lpython --separate-compilation  --show-c a.py
lpython --module-mangling  --show-c a.py

But it doesn't seem to mangle the symbols, so things still fail at link time when compiling separately two files like the above.

@Smit-create
Copy link
Collaborator

This works for me:

% lpython --separate-compilation --all-mangling --show-c b.py > d.c

The reason that global-mangling doesn't work is that I think we wrap all the global symbols in a module named _global_symbols(#2149 (comment)). I checked the ASR passed to unique_symbols pass and it has the following modules:

__main__
lpython_builtin
math

@certik
Copy link
Contributor Author

certik commented Jul 18, 2023

@Smit-create awesome, I think it works!! Here is what I tried:

$ cat a.py 
from math import pi

def f():
    print(pi)
$ cat b.py 
from math import pi

def f():
    print(pi)

f()
$ lpython --separate-compilation --all-mangling --show-c --disable-main a.py > a.c  
$ lpython --separate-compilation --all-mangling --show-c b.py > b.c                 
$ clang -I$(lpython --get-rtl-header-dir) b.c a.c -Wl,-rpath -Wl,$(lpython --get-rtl-dir) -L$(lpython --get-rtl-dir) -llpython_runtime
$ ./a.out 
3.141593

Thank you for this. I think this is good enough and we can improve it later. Great job!

@certik certik closed this as completed Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants