-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Link binary executables for the Fortran backend using clang #2805
Conversation
integration_tests/CMakeLists.txt
Outdated
@@ -206,7 +206,7 @@ RUN(NAME include_03 LABELS gfortran llvm wasm c INCLUDE_PATH include_03 fortran) | |||
RUN(NAME cond_01 LABELS gfortran llvm cpp x86 wasm c fortran) | |||
RUN(NAME cond_02 LABELS gfortran llvm wasm c fortran) | |||
|
|||
RUN(NAME expr_01 FAIL LABELS gfortran llvm cpp x86 wasm c fortran) | |||
RUN(NAME expr_01 FAIL LABELS gfortran llvm cpp x86 wasm c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's problematic to remove this, since we need the tests to fail. Can you explain what the failure is? Let's fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we link with the clang, gfortran doesn't generate the stack trace for the failure(It hangs), so I remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a stacktrace. But we need it to reliably fail.
So we have to find a solution to this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I think we can fix this using -fno-stacktrace
.
31302b8
to
e3198dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this change now on its own seems good, as it simplified the code a bit, and no regressions in tests.
I have several questions:
- Do we test these Fortran-backend tests on both linux and macOS at the CI?
- What exact issue is this PR fixing? Obviously all tests pass in master as well as in this PR, and no new test was added. However, it would be good to know what exact test fails in master, but now passes, and either add it as a test, or if too complicated, at least describe it and we can test by hand.
The last point is important to know to understand each PR change in isolation and what problem it fixes and verify that using code/test (automatic as part of the PR or manual outside of the PR). Because in general we do not want to be doing changes without understanding what they actually do or fix.
Nope, we only test on Linux. I had the same thought of testing Fortran backend on both Linux and MacOs. I will submit a PR for that.
This PR is fixing the linking of the intrinsic c function, .i.e., bindc functions. I will push the required changes soon.
I agree with that! |
How do you test it by hand? I assume there will be some linkage commands. |
Do you mean the following command? $ gfortran -fno-backtrace -o expr2.out.tmp.o -c expr2.out.tmp.o.tmp.f90
$ clang -o expr2.out expr2.out.tmp.o -L"/Users/thirumalai/Open_Source/lfortran/src/bin/../runtime" -Wl,-rpath,"/Users/thirumalai/Open_Source/lfortran/src/bin/../runtime" -llfortran_runtime -lm -L"$CONDA_PREFIX/lib" -Wl,-rpath,"$CONDA_PREFIX/lib/" -lgfortran The following command uses the above commands to compile the code using fortran backend $ lfortran examples/expr2.f90 --backend=fortran |
Ah I see --- this PR enables #2747 to work, without it it doesn't work? In this case it's ok. That's what I was looking for. |
Yup. Thanks for the review |
There seems to be some issue with the GLibc:
clock_gettime@GLIBC_2.17
.So, we need to switch to clang linking for Fortran backend.