[flang-rt] Pass the whole path of libflang_rt.runtime.a to linker on AIX and LoP #131041
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-powerpc @llvm/pr-subscribers-clang-driver Author: Daniel Chen (DanielCChen) ChangesOn AIX, we want to pass the whole path (e.g. Full diff: https://github.com/llvm/llvm-project/pull/131041.diff 1 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index b43472a52038b..21f934cdba468 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1345,7 +1345,16 @@ void tools::addFortranRuntimeLibs(const ToolChain &TC, const ArgList &Args,
if (AsNeeded)
addAsNeededOption(TC, Args, CmdArgs, /*as_needed=*/false);
}
- CmdArgs.push_back("-lflang_rt.runtime");
+ if (TC.getTriple().isOSAIX()) {
+ // On AIX, pass the whole path of flang_rt.runtime.a to be consistent
+ // with clang.
+ std::string CRTBasename = "libflang_rt.runtime.a";
+ SmallString<128> Path(TC.getCompilerRTPath());
+ llvm::sys::path::append(Path, CRTBasename);
+ if (TC.getVFS().exists(Path))
+ CmdArgs.push_back(Args.MakeArgString(std::string(Path)));
+ } else
+ CmdArgs.push_back("-lflang_rt.runtime");
addArchSpecificRPath(TC, Args, CmdArgs);
// needs libexecinfo for backtrace functions
|
There was a problem hiding this comment.
This is what I would like to do more generally, not just for AIX. This PR however will break using a shared library libflang_rt.runtime.so. Ideally, we would synchronize with the compiler-rt implementation which already has many supporting functions such as buildCompilerRTBasename.
| if (TC.getVFS().exists(Path)) | ||
| CmdArgs.push_back(Args.MakeArgString(std::string(Path))); |
There was a problem hiding this comment.
IMHO just doing nothing if the file does exist is a very confusing behavior.
There was a problem hiding this comment.
IMHO just doing nothing if the file does exist is a very confusing behavior.
There are some similar usages in the Driver code.
It adds the path to the linker arg list only if the path has been created. Otherwise, not adding it (i.e. do nothing).
For example, if I didn't have -DLLVM_ENABLE_RUNTIMES="flang-rt" specified, I wouldn't have build/lib/clang/21/lib/aix/libflang_rt.runtime.a, so this code will check that and not adding that to the linker arg list.
That being said, I totally understand your point. Just I don't have a good answer for it.
There was a problem hiding this comment.
There are some similar usages in the Driver code.
And its source of one of my biggest grievences. See #122152 and all the discussion in #87866.
If the file does not exist, I want an error message about the file not existing. With the runtime not being added, one will get an error about a symbol not resolved. Someone will have to debug why flang-rt does not define the symbol only to discover that flang-rt is not added to the linker line. Then they have to find out why and the only way I can think of is to find this line in the source that tells them that the flang-rt it not present or just in the wrong path. The result of getCompilerRTPath() varies by some parameters, or the triple is sligthly different (x86_64-unknown-linux-gnu vs x86_64-linux-gnu), or ... . I know because I went through this rabbit hole for #122152. Let's please just not do this.
If the absolute path cannot be resolved, at least add -lflang_rt.runtime so the linker can either resolve the library itself, or display an appropriate error.
There was a problem hiding this comment.
Fair enough. I will add -lflang_rt.runtime if the path does not exist.
Right. I tried with As for Would you prefer to have a new additional set of functions for flang-rt? Something like: |
With this PR applied? It hardcodes
flang-rt also has components, currently there are
I don't find the compiler-rt ones very logical as they have grown over time, especially with the Also: multilib. |
Sorry, I just realized on AIX, both static and shared library are named |
|
I am working on a patch that re-uses |
|
@Meinersbur and other reviewers, In the attempt to make building the path of flant-rt more general as well as customizable in response to the review comment, I made the following changes in this latest commit (sorry about a few more commits that fixed some typos).
Please let me know if this approach is acceptable. |
DanielCChen
force-pushed
the
daniel_driver
branch
from
aa19eda to
310bcff
Compare
There was a problem hiding this comment.
Approach look OK to me.
Could you add test(s) along the lines of aix-rlib.c. If we make further changes, knowing what is expected on AIX/PPCLinux would be very helpful.
A review from someone involved with the Clang driver side would be helpful. @MaskRay ?
| Path = getCompilerRTArgString(Args, "runtime", ToolChain::FT_Static, | ||
| getDriver().IsFlangMode())))) |
There was a problem hiding this comment.
Aren't we always in IsFlangMode when executing addFortranRuntimeLibs?
There was a problem hiding this comment.
Yes, we are.
The IsFlangMode() is indeed checked before we call addFortranRuntimeLibs.
Change both AIX.cpp and PPCLinux.cpp to true.
Thanks for the review again. Future improvement:
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thanks for the comments! I updated the description to reflect the latest changes. |
DanielCChen
force-pushed
the
daniel_driver
branch
from
644510a to
19bdc52
Compare
|
The intention is to make this PR for flang-rt only. As discussed with @daltenty, I remove the NFC-change of adding an overriding |
DanielCChen
force-pushed
the
daniel_driver
branch
2 times, most recently
from
f812a8a to
b63b099
Compare
|
@Meinersbur @MaskRay and all other reviewers, PR #132821 enables The change in this PR is intact because I re-used the same code to handle the option either ON or OFF. I updated the description of this PR to reflect that. I will undo PR #130875 and add a LIT test to this PR as soon as PR #132821 lands. Sorry about the change or direction. |
DanielCChen
force-pushed
the
daniel_driver
branch
from
b63b099 to
64d4efc
Compare
…R=ON, LoP should still be able to handle both to be consistent with clang-rt.
…_target_runttime_dir and add LIT tests.
DanielCChen
force-pushed
the
daniel_driver
branch
from
de41c60 to
a999a3e
Compare
DanielCChen
changed the title
Thanks for the suggestion! I will investigate that and post a separate PR. |
|
I think that this patch broke our x86-64 builds with a test failure in Driver/linker-flags.f90. |
|
Thanks! I will take a look. |
|
@klausler It seems it passes for me on LoP after I pull the latest source. Could you please try again and see if you still run into the failure? |
|
Still fails in my x86-64 environment. |
|
Still fail at the same |
|
After llvm#131041, the F128 libraries are not linked for PPC targets even when the driver is built with FLANG_RUNTIME_F128_MATH_LIB.
|
Seems PR #134320 is fixing this. |
After #131041, the F128 libraries are not linked for PPC targets even when the driver is built with FLANG_RUNTIME_F128_MATH_LIB.
…e.a. (#134362) The PR is to generalize the re-use of the `compilerRT` code of adding the path of `libflang_rt.runtime.a (so)` from AIX and LoP only to all platforms via a new function `addFlangRTLibPath`. It also added `-static-libflangrt` and `-shared-libflangrt` compiler options to allow users choosing which `flang-rt` to link to. It defaults to shared `flang-rt`, which is consistent with the linker behavior, except on AIX, it defaults to static. Also, PR #134320 exposed an issue in PR #131041 that the the overriding `addFortranRuntimeLibs` is missing the link to `libquadmath`. This PR also fixed that and restored the test case that PR #131041 broke.
…e.a. (llvm#134362) The PR is to generalize the re-use of the `compilerRT` code of adding the path of `libflang_rt.runtime.a (so)` from AIX and LoP only to all platforms via a new function `addFlangRTLibPath`. It also added `-static-libflangrt` and `-shared-libflangrt` compiler options to allow users choosing which `flang-rt` to link to. It defaults to shared `flang-rt`, which is consistent with the linker behavior, except on AIX, it defaults to static. Also, PR llvm#134320 exposed an issue in PR llvm#131041 that the the overriding `addFortranRuntimeLibs` is missing the link to `libquadmath`. This PR also fixed that and restored the test case that PR llvm#131041 broke.
This PR is to improve the driver code to build
flang-rtpath by re-using the logic and code ofcompiler-rt.Moved
addFortranRuntimeLibraryPathandaddFortranRuntimeLibstoToolChain.hand made them virtual so that they can be overridden if customization is needed. The current implementation of those two procedures is moved toToolChain.cppas the base implementation to default to.Both AIX and PPCLinux now override
addFortranRuntimeLibs.The overriding function of
addFortranRuntimeLibsfor both AIX and PPCLinux callsgetCompilerRTArgString=>getCompilerRT=>buildCompilerRTBasenameto get the path toflang-rt. This code handlesLLVM_ENABLE_PER_TARGET_RUNTIME_DIRsetting. As shown inPPCLinux.cpp,FT_staticis the default. If not found, it will search and build forFT_shared. To differentiateflang-rtfromclang-rt, a boolean flagIsFortranis passed to the chain of functions in order to reachbuildCompilerRTBasename.