Reapply "[Driver][OHOS] Fix lld link issue for OHOS (#118192)" #120159
Conversation
This reverts commit 1911919.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Peng Huang (phuang) ChangesThe problem in original change is because OHOS::getCompilerRT() Full diff: https://github.com/llvm/llvm-project/pull/120159.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 9f174fbda398b5..f12a2744391e9a 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -745,7 +745,13 @@ std::string ToolChain::buildCompilerRTBasename(const llvm::opt::ArgList &Args,
std::string ArchAndEnv;
if (AddArch) {
StringRef Arch = getArchNameForCompilerRTLib(*this, Args);
- const char *Env = TT.isAndroid() ? "-android" : "";
+ const char *Env = NULL;
+ if (TT.isAndroid())
+ Env = "-android";
+ else if (TT.isOHOSFamily())
+ Env = "-ohos";
+ else
+ Env = "";
ArchAndEnv = ("-" + Arch + Env).str();
}
return (Prefix + Twine("clang_rt.") + Component + ArchAndEnv + Suffix).str();
diff --git a/clang/lib/Driver/ToolChains/OHOS.cpp b/clang/lib/Driver/ToolChains/OHOS.cpp
index 6e1a09ae908b2f..c9a532771b99e5 100644
--- a/clang/lib/Driver/ToolChains/OHOS.cpp
+++ b/clang/lib/Driver/ToolChains/OHOS.cpp
@@ -19,8 +19,8 @@
#include "llvm/ProfileData/InstrProf.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
-#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/VirtualFileSystem.h"
using namespace clang::driver;
using namespace clang::driver::toolchains;
@@ -58,11 +58,9 @@ static bool findOHOSMuslMultilibs(const Driver &D,
return false;
}
-static bool findOHOSMultilibs(const Driver &D,
- const ToolChain &TC,
- const llvm::Triple &TargetTriple,
- StringRef Path, const ArgList &Args,
- DetectedMultilibs &Result) {
+static bool findOHOSMultilibs(const Driver &D, const ToolChain &TC,
+ const llvm::Triple &TargetTriple, StringRef Path,
+ const ArgList &Args, DetectedMultilibs &Result) {
Multilib::flags_list Flags;
bool IsA7 = false;
if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ))
@@ -172,8 +170,7 @@ OHOS::OHOS(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
Paths);
}
-ToolChain::RuntimeLibType OHOS::GetRuntimeLibType(
- const ArgList &Args) const {
+ToolChain::RuntimeLibType OHOS::GetRuntimeLibType(const ArgList &Args) const {
if (Arg *A = Args.getLastArg(clang::driver::options::OPT_rtlib_EQ)) {
StringRef Value = A->getValue();
if (Value != "compiler-rt")
@@ -184,20 +181,19 @@ ToolChain::RuntimeLibType OHOS::GetRuntimeLibType(
return ToolChain::RLT_CompilerRT;
}
-ToolChain::CXXStdlibType
-OHOS::GetCXXStdlibType(const ArgList &Args) const {
+ToolChain::CXXStdlibType OHOS::GetCXXStdlibType(const ArgList &Args) const {
if (Arg *A = Args.getLastArg(options::OPT_stdlib_EQ)) {
StringRef Value = A->getValue();
if (Value != "libc++")
getDriver().Diag(diag::err_drv_invalid_stdlib_name)
- << A->getAsString(Args);
+ << A->getAsString(Args);
}
return ToolChain::CST_Libcxx;
}
void OHOS::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
- ArgStringList &CC1Args) const {
+ ArgStringList &CC1Args) const {
const Driver &D = getDriver();
const llvm::Triple &Triple = getTriple();
std::string SysRoot = computeSysRoot();
@@ -258,7 +254,7 @@ void OHOS::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
}
void OHOS::AddCXXStdlibLibArgs(const ArgList &Args,
- ArgStringList &CmdArgs) const {
+ ArgStringList &CmdArgs) const {
switch (GetCXXStdlibType(Args)) {
case ToolChain::CST_Libcxx:
CmdArgs.push_back("-lc++");
@@ -291,7 +287,8 @@ ToolChain::path_list OHOS::getRuntimePaths() const {
// First try the triple passed to driver as --target=<triple>.
P.assign(D.ResourceDir);
- llvm::sys::path::append(P, "lib", D.getTargetTriple(), SelectedMultilib.gccSuffix());
+ llvm::sys::path::append(P, "lib", D.getTargetTriple(),
+ SelectedMultilib.gccSuffix());
Paths.push_back(P.c_str());
// Second try the normalized triple.
@@ -340,26 +337,20 @@ std::string OHOS::getDynamicLinker(const ArgList &Args) const {
std::string OHOS::getCompilerRT(const ArgList &Args, StringRef Component,
FileType Type) const {
+ std::string CRTBasename =
+ buildCompilerRTBasename(Args, Component, Type, /*AddArch=*/false);
+
SmallString<128> Path(getDriver().ResourceDir);
llvm::sys::path::append(Path, "lib", getMultiarchTriple(getTriple()),
- SelectedMultilib.gccSuffix());
- const char *Prefix =
- Type == ToolChain::FT_Object ? "" : "lib";
- const char *Suffix;
- switch (Type) {
- case ToolChain::FT_Object:
- Suffix = ".o";
- break;
- case ToolChain::FT_Static:
- Suffix = ".a";
- break;
- case ToolChain::FT_Shared:
- Suffix = ".so";
- break;
- }
- llvm::sys::path::append(
- Path, Prefix + Twine("clang_rt.") + Component + Suffix);
- return static_cast<std::string>(Path.str());
+ SelectedMultilib.gccSuffix(), CRTBasename);
+ if (getVFS().exists(Path))
+ return std::string(Path);
+
+ std::string NewPath = ToolChain::getCompilerRT(Args, Component, Type);
+ if (getVFS().exists(NewPath))
+ return NewPath;
+
+ return std::string(Path);
}
void OHOS::addExtraOpts(llvm::opt::ArgStringList &CmdArgs) const {
@@ -396,7 +387,7 @@ SanitizerMask OHOS::getSupportedSanitizers() const {
// TODO: Make a base class for Linux and OHOS and move this there.
void OHOS::addProfileRTLibs(const llvm::opt::ArgList &Args,
- llvm::opt::ArgStringList &CmdArgs) const {
+ llvm::opt::ArgStringList &CmdArgs) const {
// Add linker option -u__llvm_profile_runtime to cause runtime
// initialization module to be linked in.
if (needsProfileRT(Args))
@@ -413,7 +404,8 @@ ToolChain::path_list OHOS::getArchSpecificLibPaths() const {
return Paths;
}
-ToolChain::UnwindLibType OHOS::GetUnwindLibType(const llvm::opt::ArgList &Args) const {
+ToolChain::UnwindLibType
+OHOS::GetUnwindLibType(const llvm::opt::ArgList &Args) const {
if (Args.getLastArg(options::OPT_unwindlib_EQ))
return Generic_ELF::GetUnwindLibType(Args);
return GetDefaultUnwindLibType();
|
|
IIRC there are two possible compiler-rt directory layouts. @MaskRay worked on moving many platforms from one to another. I think the newer layout doesn't use platform suffixes. We shouldn't change where clang looks for things just for my bot. Thanks for figuring it out; I can delete the file from that bot. (What about developers that are in the same position though -- maybe it'd make sense to have a cmake action that deletes the file in the old location for a bit? Wouldn't help my bot since it doesn't use cmake, but might help others.) We should figure out which path we want independently of my bot :) I don't know the current naming rules. Maybe @MaskRay remembers if we have documentation on this. Else, just relanding the patch as it was is fine. |
MaskRay
left a comment
MaskRay
left a comment
There was a problem hiding this comment.
Without a test this should not land. #118192 (comment)
Hi @MaskRay could you please suggest how to test the changes? I though |
|
@MaskRay friendly ping. thanks. |
MaskRay
left a comment
MaskRay
left a comment
There was a problem hiding this comment.
We need a test to clang/test/Driver/ohos.c similar to linux-ld.c clang_rt.crtbegin.o.
You can add UNSUPPORTED: system-windows to ohos.c so that we don't need backslashes.
ohos.c already has many tests for it. This PR changes behavior of function OHOS::getCompilerRT(). It will search extra folders for compiler rt builtin libraries. But in tests, OHOS::getCompilerRT()'s return value will not be changed, becasue those extra folders don't have those requested builtin files. It will just returns the default library file path. |
|
Thanks for reviewing it. Could you please land it for me? Seems I cannot land it. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/6297 Here is the relevant piece of the build log for the reference |
|
I checked the log. And I don't think the failed test is caused by this PR. |
|
I did remove the contents of out/gn/lib/clang/20/lib/linux, but I'm guessing this might break tests in cmake builds that use |
|
In that case, probably we have to add |
|
Here's a command you can use to reproduce this: |
#120159)" This reverts commit bd154e8. Test fails with -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF, see #120159 (comment)
|
Thanks for the repro command. I reproduced the problem and verified it can be fixed with adding |
|
This patch is reverted. We can disable the test on -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF layouts, but I don't find a lit feature that. The usual way is to create a compiler-rt file hierarchy under Inputs. Just fd libclang_rt.builtins.a for some examples. #121484 , which is intrusive to the generic code, should not be allowed. Android is popular and has that hack for historical reasons. New platforms should not do that. |
It cannot work, becasue in the failed case, llvm runtime |
|
I created a new PR , PTAL. Thanks |
…S (#118192)" (#120159)" This reverts commit bd154e8. Test fails with -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF, see llvm/llvm-project#120159 (comment)
5 participants
The problem in original change is because OHOS::getCompilerRT()
pickes a wrong builtin runtime
./lib/clang/20/lib/linux/libclang_rt.builtins-x86_64.a,if
./lib/clang/20/lib/linux/libclang_rt.builtins-x86_64.aexist on thetest filesystem. It shouldn't happen with a clean build.