Skip to content

Conversation

@phuang
Copy link
Contributor

@phuang phuang commented Jan 2, 2025

…DIR=OFF

The problem is because libclang_rt.builtins-{arch}.a for all the arches will be installed into the same folder with old compiler rt layout (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF) and OHOS driver will get a wrong library in this case. To fix the problem, -ohos suffix will be added to the ohos builtin libraries search path.

Note: OHOS driver doesn't support the old layout, compiler-rt for ${arch}-linux-unknown-ohos targets have to be built with
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON

…DIR=OFF

The problem is because libclang_rt.builtins-{arch}.a for all the
arches will be installed into the same folder with old compiler rt
layout (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF) and OHOS driver will
get a wrong library in this case. To fix the problem, `-ohos`
suffix will be added to the ohos builtin libraries search path.

Note: OHOS driver doesn't support the old layout,
${arch}-linux-unknown-ohos targets have to be built with
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jan 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 2, 2025

@llvm/pr-subscribers-clang

Author: Peng Huang (phuang)

Changes

…DIR=OFF

The problem is because libclang_rt.builtins-{arch}.a for all the arches will be installed into the same folder with old compiler rt layout (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF) and OHOS driver will get a wrong library in this case. To fix the problem, -ohos suffix will be added to the ohos builtin libraries search path.

Note: OHOS driver doesn't support the old layout, compiler-rt for ${arch}-linux-unknown-ohos targets have to be built with
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON


Full diff: https://github.com/llvm/llvm-project/pull/121484.diff

1 Files Affected:

  • (modified) clang/lib/Driver/ToolChain.cpp (+5-1)
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 9f174fbda398b5..06ff0918245ea4 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -745,7 +745,11 @@ 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 = "";
+    if (TT.isAndroid())
+      Env = "-android";
+    else if (TT.isOHOSFamily())
+      Env = "-ohos";
     ArchAndEnv = ("-" + Arch + Env).str();
   }
   return (Prefix + Twine("clang_rt.") + Component + ArchAndEnv + Suffix).str();

@llvmbot
Copy link
Member

llvmbot commented Jan 2, 2025

@llvm/pr-subscribers-clang-driver

Author: Peng Huang (phuang)

Changes

…DIR=OFF

The problem is because libclang_rt.builtins-{arch}.a for all the arches will be installed into the same folder with old compiler rt layout (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF) and OHOS driver will get a wrong library in this case. To fix the problem, -ohos suffix will be added to the ohos builtin libraries search path.

Note: OHOS driver doesn't support the old layout, compiler-rt for ${arch}-linux-unknown-ohos targets have to be built with
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON


Full diff: https://github.com/llvm/llvm-project/pull/121484.diff

1 Files Affected:

  • (modified) clang/lib/Driver/ToolChain.cpp (+5-1)
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 9f174fbda398b5..06ff0918245ea4 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -745,7 +745,11 @@ 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 = "";
+    if (TT.isAndroid())
+      Env = "-android";
+    else if (TT.isOHOSFamily())
+      Env = "-ohos";
     ArchAndEnv = ("-" + Arch + Env).str();
   }
   return (Prefix + Twine("clang_rt.") + Component + ArchAndEnv + Suffix).str();

@carlocab
Copy link
Member

carlocab commented Jan 2, 2025

Note: OHOS driver doesn't support the old layout, compiler-rt for ${arch}-linux-unknown-ohos targets have to be built with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON

Can we make cmake error out if you try to build for *-linux-unknown-ohos with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF?

@phuang
Copy link
Contributor Author

phuang commented Jan 2, 2025

Note: OHOS driver doesn't support the old layout, compiler-rt for ${arch}-linux-unknown-ohos targets have to be built with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON

Can we make cmake error out if you try to build for *-linux-unknown-ohos with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF?

Sure. I will try adding it.
And one question: will llvm deprecate the old layout? should we consider adding the old layout support for ohos?

@carlocab
Copy link
Member

carlocab commented Jan 2, 2025

And one question: will llvm deprecate the old layout?

There was some desire to, but it seems to have fizzled out: https://discourse.llvm.org/t/rfc-time-to-drop-legacy-runtime-paths/64628

I imagine it'll happen eventually, but it will probably take a while.

should we consider adding the old layout support for ohos?

I don't know enough about OHOS and its users to be able to say. If you're not sure: I suggest leaving out support for now, and someone who needs it can contribute support for it. But if we do leave support out: good error messages for users who happen to try to use/build it in the wrong configuration are probably essential.

@phuang
Copy link
Contributor Author

phuang commented Jan 2, 2025

To add this cmake configure error, where should I look at? any hint?

@phuang
Copy link
Contributor Author

phuang commented Jan 2, 2025

Updated the cmake script to output error message if LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is OFF for ohos targets.

OHOS driver doesn't support old runtime libraries layout, so make
cmake configure report error if LLVM_ENABLE_PER_TARGET_RUNTIME_DIR
is OFF.
@phuang phuang force-pushed the wip_fix_ohos_link_error branch from b41e3c3 to ee85a1c Compare January 2, 2025 18:30
@MaskRay
Copy link
Member

MaskRay commented Jan 3, 2025

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.

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@MaskRay MaskRay closed this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category compiler-rt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants