-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[driver] Generalize the code that adds the path of libflang_rt.runtime.a. #134362
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
Conversation
@llvm/pr-subscribers-flang-driver @llvm/pr-subscribers-clang-driver Author: Daniel Chen (DanielCChen) ChangesThe PR is to generalize the re-use of the Also, PR #134320 exposed an issue in PR #131041 that the the overriding Full diff: https://github.com/llvm/llvm-project/pull/134362.diff 7 Files Affected:
diff --git a/clang/include/clang/Driver/ToolChain.h b/clang/include/clang/Driver/ToolChain.h
index 076e4296c3090..d0059673d6a67 100644
--- a/clang/include/clang/Driver/ToolChain.h
+++ b/clang/include/clang/Driver/ToolChain.h
@@ -521,6 +521,10 @@ class ToolChain {
addFortranRuntimeLibraryPath(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs) const;
+ /// Add the path for libflang_rt.runtime.a
+ void addFlangRTLibPath(const llvm::opt::ArgList &Args,
+ llvm::opt::ArgStringList &CmdArgs) const;
+
const char *getCompilerRTArgString(const llvm::opt::ArgList &Args,
StringRef Component,
FileType Type = ToolChain::FT_Static,
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 36d0ae34dec86..054618a44d7bc 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -816,8 +816,7 @@ void ToolChain::addFortranRuntimeLibs(const ArgList &Args,
if (AsNeeded)
addAsNeededOption(*this, Args, CmdArgs, /*as_needed=*/false);
}
- CmdArgs.push_back("-lflang_rt.runtime");
- addArchSpecificRPath(*this, Args, CmdArgs);
+ addFlangRTLibPath(Args, CmdArgs);
// needs libexecinfo for backtrace functions
if (getTriple().isOSFreeBSD() || getTriple().isOSNetBSD() ||
@@ -850,6 +849,23 @@ void ToolChain::addFortranRuntimeLibraryPath(const llvm::opt::ArgList &Args,
CmdArgs.push_back(Args.MakeArgString("-L" + DefaultLibPath));
}
+void ToolChain::addFlangRTLibPath(const ArgList &Args,
+ llvm::opt::ArgStringList &CmdArgs) const {
+ // Link static flang_rt.runtime.a or shared flang_rt.runtime.so
+ const char *Path;
+ if (getVFS().exists(Twine(Path = getCompilerRTArgString(
+ Args, "runtime", ToolChain::FT_Static, true))))
+ CmdArgs.push_back(Path);
+ else if (getVFS().exists(
+ Twine(Path = getCompilerRTArgString(
+ Args, "runtime", ToolChain::FT_Shared, true))))
+ CmdArgs.push_back(Path);
+ else {
+ CmdArgs.push_back("-lflang_rt.runtime");
+ addArchSpecificRPath(*this, Args, CmdArgs);
+ }
+}
+
// Android target triples contain a target version. If we don't have libraries
// for the exact target version, we should fall back to the next newest version
// or a versionless path, if any.
diff --git a/clang/lib/Driver/ToolChains/AIX.cpp b/clang/lib/Driver/ToolChains/AIX.cpp
index 26b9d4c772be6..5dc80bc5a3d25 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -608,14 +608,6 @@ void AIX::addProfileRTLibs(const llvm::opt::ArgList &Args,
ToolChain::addProfileRTLibs(Args, CmdArgs);
}
-void AIX::addFortranRuntimeLibs(const ArgList &Args,
- llvm::opt::ArgStringList &CmdArgs) const {
- // Link flang_rt.runtime.a. On AIX, the static and shared library are all
- // named .a
- CmdArgs.push_back(
- getCompilerRTArgString(Args, "runtime", ToolChain::FT_Static, true));
-}
-
ToolChain::CXXStdlibType AIX::GetDefaultCXXStdlibType() const {
return ToolChain::CST_Libcxx;
}
diff --git a/clang/lib/Driver/ToolChains/AIX.h b/clang/lib/Driver/ToolChains/AIX.h
index 17e8370cd1218..8f130f6b54547 100644
--- a/clang/lib/Driver/ToolChains/AIX.h
+++ b/clang/lib/Driver/ToolChains/AIX.h
@@ -87,9 +87,6 @@ class LLVM_LIBRARY_VISIBILITY AIX : public ToolChain {
void addProfileRTLibs(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs) const override;
- void addFortranRuntimeLibs(const llvm::opt::ArgList &Args,
- llvm::opt::ArgStringList &CmdArgs) const override;
-
CXXStdlibType GetDefaultCXXStdlibType() const override;
RuntimeLibType GetDefaultRuntimeLibType() const override;
diff --git a/clang/lib/Driver/ToolChains/PPCLinux.cpp b/clang/lib/Driver/ToolChains/PPCLinux.cpp
index 575e88c6ab124..0ed0f91ad166c 100644
--- a/clang/lib/Driver/ToolChains/PPCLinux.cpp
+++ b/clang/lib/Driver/ToolChains/PPCLinux.cpp
@@ -12,7 +12,6 @@
#include "clang/Driver/Options.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
-#include "llvm/Support/VirtualFileSystem.h"
using namespace clang::driver;
using namespace clang::driver::toolchains;
@@ -102,18 +101,3 @@ bool PPCLinuxToolChain::SupportIEEEFloat128(
return GlibcSupportsFloat128((Twine(D.DyldPrefix) + Linker).str()) &&
!(D.CCCIsCXX() && HasUnsupportedCXXLib);
}
-
-void PPCLinuxToolChain::addFortranRuntimeLibs(
- const ArgList &Args, llvm::opt::ArgStringList &CmdArgs) const {
- // Link static flang_rt.runtime.a or shared flang_rt.runtime.so
- const char *Path;
- if (getVFS().exists(Twine(Path = getCompilerRTArgString(
- Args, "runtime", ToolChain::FT_Static, true))))
- CmdArgs.push_back(Path);
- else if (getVFS().exists(
- Twine(Path = getCompilerRTArgString(
- Args, "runtime", ToolChain::FT_Shared, true))))
- CmdArgs.push_back(Path);
- else
- CmdArgs.push_back("-lflang_rt.runtime");
-}
diff --git a/clang/lib/Driver/ToolChains/PPCLinux.h b/clang/lib/Driver/ToolChains/PPCLinux.h
index 910df3d16e6a5..63adaff6be9c2 100644
--- a/clang/lib/Driver/ToolChains/PPCLinux.h
+++ b/clang/lib/Driver/ToolChains/PPCLinux.h
@@ -24,9 +24,6 @@ class LLVM_LIBRARY_VISIBILITY PPCLinuxToolChain : public Linux {
AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
- void addFortranRuntimeLibs(const llvm::opt::ArgList &Args,
- llvm::opt::ArgStringList &CmdArgs) const override;
-
private:
bool SupportIEEEFloat128(const Driver &D, const llvm::Triple &Triple,
const llvm::opt::ArgList &Args) const;
diff --git a/flang/test/Driver/linker-flags.f90 b/flang/test/Driver/linker-flags.f90
index 20104276d2e4a..4e62a8c32d360 100644
--- a/flang/test/Driver/linker-flags.f90
+++ b/flang/test/Driver/linker-flags.f90
@@ -2,7 +2,7 @@
! invocation. These libraries are added on top of other standard runtime
! libraries that the Clang driver will include.
-! RUN: %flang -### --target=ppc64le-linux-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,UNIX,UNIX-F128NONE
+! RUN: %flang -### --target=ppc64le-linux-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,UNIX,UNIX-F128%f128-lib
! RUN: %flang -### --target=aarch64-apple-darwin %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,DARWIN,DARWIN-F128%f128-lib
! RUN: %flang -### --target=sparc-sun-solaris2.11 %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,UNIX,SOLARIS-F128%f128-lib
! RUN: %flang -### --target=x86_64-unknown-freebsd %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,BSD,BSD-F128%f128-lib
|
Can you please explain why you want to link the static library by default? For example, Clang has these options https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-static-libgcc that allow users to force the static linking, and, I think, by default it links shared compiler support libraries. Also, how does the full path linking of the shared library works with |
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.
LGTM. Thanks!
May I ask not to merge this yet? I have a question above, but I will not be at work until next Thursday to discuss this in a timely manner. |
@vzakhari Sure, I will wait until you come back. |
The As for the I actually missed that in the shared path in this PR. I will push an amendment. |
This PR may allow us to build the shared library by default llvm-project/flang-rt/CMakeLists.txt Lines 159 to 165 in 6c7c8b4
I would generally prefer if we could avoid having driver behavior depend on the existance of files, but the linker would also pick an existing file as well. I think the static library should be the default (like libclang_rt.builtins). Most uses will already use the static library unless they went into the effort of building the shared library. Then, flang-rt and flang are version-locked without a versioning scheme ( |
Agreed. The |
Agreed to have the static library as default (so I think I posted a question on the Flang |
I pushed another commit to add It defaults to static on AIX. My apology to the reviewers especially those who have already approved this PR @MaskRay @tarunprabhu. |
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.
Thanks Daniel. I don't think a separate PR is necessary for the changes, but perhaps wait to see if @MaskRay has a different opinion
…cify which flang-rt to link to.
f775828
to
d3c811f
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.
Thanks for the update. I have one questions inlined.
Thanks for the review! |
I find this change quite confusing. It seems that
whereas clang uses |
Ah, sorry, now I see that |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/15937 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/10376 Here is the relevant piece of the build log for the reference
|
…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.
The PR is to generalize the re-use of the
compilerRT
code of adding the path oflibflang_rt.runtime.a (so)
from AIX and LoP only to all platforms via a new functionaddFlangRTLibPath
.It also added
-static-libflangrt
and-shared-libflangrt
compiler options to allow users choosing whichflang-rt
to link to. It defaults to sharedflang-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 tolibquadmath
. This PR also fixed that and restored the test case that PR #131041 broke.