CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #89234

wzssyqa · 2024-04-18T13:45:32Z

If LLVM is configured with -DLLVM_DEFAULT_TARGET_TRIPLE, or compiler_rt is configured with -DCOMPILER_RT_DEFAULT_TARGET_TRIPLE, while the argument is not normalized, such as Debian-style vendor-less triple, clang will try to find libclang_rt in lib/<normalized_triple>, while libclang_rt is placed into lib/<triple_arg>.

Let's also place libclang_rt into lib/<normalized_triple>.
libcxx/utils/ci/run-buildbot is also updated to use armv7m-none-unknown-eabi as normalized triple instead of
current armv7m-none-eabi.

ChangeLog of this PR:
This patch has been applied and revert twice:

The first try is CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #88407, and then it is found that it causes some CI failures.
https://lab.llvm.org/buildbot/#/builders/98/builds/36366
It is then resolved by another commit: 1693009

https://lab.llvm.org/buildbot/#/builders/77/builds/36372
It is caused that COMPILER_RT_DEFAULT_TARGET_TRIPLE is overwrite without taking care about CACHE.
The second try CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #88835,
resolves https://lab.llvm.org/buildbot/#/builders/77/builds/36372
and in fact only one execute_process is needed.

Then we find some other CI failures.
https://github.com/mstorsjo/llvm-mingw/actions/runs/8730621159
https://buildkite.com/llvm-project/libcxx-ci/builds/34897#018eec06-612c-47f1-9931-d3bd88bf7ced

It is due to misunderstanding -print-effective-triple: which will output thumbv7-w64-windows-gnu for armv7-w64-windows-gnu or some other thumb-enabled arm triples.
In fact we should use -print-target-triple.

For armv7m-picolibc, armv7m-none-eabi is hardcoded in libcxx/utils/ci/run-buildbot, while in fact armv7m-none-unknown-eabi is the real normalized triple.

If LLVM is configured with -DLLVM_DEFAULT_TARGET_TRIPLE, or compiler_rt is configured with -DCOMPILER_RT_DEFAULT_TARGET_TRIPLE, while the argument is not normalized, such as Debian-style vendor-less triple, clang will try to find libclang_rt in lib/<normalized_triple>, while libclang_rt is placed into lib/<triple_arg>. Let's also place libclang_rt into lib/<normalized_triple>.

llvmbot · 2024-04-18T13:46:04Z

@llvm/pr-subscribers-libcxx

Author: YunQiang Su (wzssyqa)

Changes

If LLVM is configured with -DLLVM_DEFAULT_TARGET_TRIPLE, or compiler_rt is configured with -DCOMPILER_RT_DEFAULT_TARGET_TRIPLE, while the argument is not normalized, such as Debian-style vendor-less triple, clang will try to find libclang_rt in lib/<normalized_triple>, while libclang_rt is placed into lib/<triple_arg>.

Let's also place libclang_rt into lib/<normalized_triple>.

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

2 Files Affected:

(modified) compiler-rt/cmake/Modules/CompilerRTUtils.cmake (+6)
(modified) libcxx/utils/ci/run-buildbot (+1-1)

diff --git a/compiler-rt/cmake/Modules/CompilerRTUtils.cmake b/compiler-rt/cmake/Modules/CompilerRTUtils.cmake
index e8e5f612d5b03c..a6c6ef93500d53 100644
--- a/compiler-rt/cmake/Modules/CompilerRTUtils.cmake
+++ b/compiler-rt/cmake/Modules/CompilerRTUtils.cmake
@@ -368,6 +368,12 @@ macro(construct_compiler_rt_default_triple)
           "Default triple for which compiler-rt runtimes will be built.")
   endif()
 
+  if ("${CMAKE_C_COMPILER_ID}" MATCHES "Clang")
+    execute_process(COMMAND ${CMAKE_C_COMPILER} --target=${COMPILER_RT_DEFAULT_TARGET_TRIPLE} -print-target-triple
+                    OUTPUT_VARIABLE COMPILER_RT_DEFAULT_TARGET_TRIPLE
+                    OUTPUT_STRIP_TRAILING_WHITESPACE)
+  endif()
+
   string(REPLACE "-" ";" LLVM_TARGET_TRIPLE_LIST ${COMPILER_RT_DEFAULT_TARGET_TRIPLE})
   list(GET LLVM_TARGET_TRIPLE_LIST 0 COMPILER_RT_DEFAULT_TARGET_ARCH)
 
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 23a2a1bbbc63fa..60307a7d4f350a 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -217,7 +217,7 @@ function test-armv7m-picolibc() {
         "${@}"
 
     ${NINJA} -vC "${BUILD_DIR}/compiler-rt" install
-    mv "${BUILD_DIR}/install/lib/armv7m-none-eabi"/* "${BUILD_DIR}/install/lib"
+    mv "${BUILD_DIR}/install/lib/armv7m-none-unknown-eabi"/* "${BUILD_DIR}/install/lib"
 
     check-runtimes
 }

DavidSpickett · 2024-04-18T14:55:10Z

Please expand the commit message to describe your previous confusion with effective triple, and why target triple is ok. Also that this changes some folder names hence the libcxx change.

So that we have an audit trail here, as it's turning out to be a tricky change.

mstorsjo · 2024-04-18T20:20:31Z

Thanks, this form seems to work fine for me, but I second @DavidSpickett 's requests for more elaboration on earlier attempts and what's changed from them (plus a reference to earlier PRs or commits, for review trail).

wzssyqa · 2024-04-19T00:01:55Z

Thanks, this form seems to work fine for me, but I second @DavidSpickett 's requests for more elaboration on earlier attempts and what's changed from them (plus a reference to earlier PRs or commits, for review trail).

Thanks. I updated the commit msg.
I saw there is another complain about this PR:
#89150

  Host compiler does not support '-fuse-ld=lld'.  Please make sure that 'lld'
  is installed and that your host compiler can compile a simple program when
  given the option '-fuse-ld=lld'.

So let's have a wait about how to reproduce it, and have a test.

wzssyqa · 2024-04-19T03:19:29Z

@ZijunZhaoCCK said that #89150 is a invalid report.

petrhosek · 2024-04-19T18:08:09Z

This change only covers compiler-rt but it should apply to all runtimes. I've made a similar change that covers all runtimes in https://reviews.llvm.org/D140925 but I forgot to land before the GitHub migration. I reuploaded that change as #89425 and I intend to land unless there are any objects, and after that we could consider undoing this change.

peterwaller-arm · 2024-04-22T08:46:52Z

What I see for baremetal builds is that a target triple of aarch64-none-elf (<arch>-<sys>-<env>) is getting internally defaulted to search for libraries at aarch64-none-unknown-elf (<arch>-<vendor>-<sys>-<env>) but I understand this represents a change of the <sys> part of the triple from none to unknown?

Is this intended? One consequence is that my existing cross compiler setup which uses symlinked clang binaries aarch64-none-elf-clang -> clang stopped working because it searches for builtins at /lib/clang/19/lib/aarch64-none-unknown-elf/libclang_rt.builtins.a.

In response to this change I am currently building compiler-rt with an explicit triple of aarch64-unknown-none-elf.

My expectation was that I should be able to continue to use my old aarch64-none-elf-clang and the extra part of the triple would be considered when searching for libraries, where they would be found at lib/aarch64-unknown-none/.... Please correct me if I'm wrong.

wzssyqa · 2024-04-22T09:09:24Z

I don't think that this PR changes the clang behavior where to search for libclang_rt.builtins.
Before this patch, clang search libclang_rt.builtins from a normalized triple subdir.

I think that your problem is about how aarch64-none-elf is normalized.
In current code, aarch64-none-elf is normalized to aarch64-unknown-none-elf.
If it is incorrect, we can have a new PR for it.
See std::string Triple::normalize(StringRef Str) in llvm/lib/TargetParser/Triple.cpp.

peterwaller-arm · 2024-04-22T09:14:10Z

In current code, aarch64-none-elf is normalized to aarch64-unknown-none-elf.

What is puzzling is that it's being normalized here to aarch64-none-unknown-elf, i.e. none and unknown are in the wrong order compared to what I expect. In the PR body you wrote:

libcxx/utils/ci/run-buildbot is also updated to use armv7m-none-unknown-eabi

This also looks suspicious, I'm expecting unknown-none, not none-unknown.

wzssyqa · 2024-04-22T09:25:48Z

libcxx/utils/ci/run-buildbot is also updated to use armv7m-none-unknown-eabi

This also looks suspicious, I'm expecting unknown-none, not none-unknown.

Sound reasonable. I will try to fix Triple::normalize.

DavidSpickett · 2024-04-22T09:57:20Z

To be clear, the libcxx change is "make it work" not "is correct". I agree with Peter that the normalisation does not look correct.

mstorsjo · 2024-04-22T10:03:56Z

What I see for baremetal builds is that a target triple of aarch64-none-elf (<arch>-<sys>-<env>) is getting internally defaulted to search for libraries at aarch64-none-unknown-elf (<arch>-<vendor>-<sys>-<env>) but I understand this represents a change of the <sys> part of the triple from none to unknown?

Is this intended? One consequence is that my existing cross compiler setup which uses symlinked clang binaries aarch64-none-elf-clang -> clang stopped working because it searches for builtins at /lib/clang/19/lib/aarch64-none-unknown-elf/libclang_rt.builtins.a.

In response to this change I am currently building compiler-rt with an explicit triple of aarch64-unknown-none-elf.

My expectation was that I should be able to continue to use my old aarch64-none-elf-clang and the extra part of the triple would be considered when searching for libraries, where they would be found at lib/aarch64-unknown-none/.... Please correct me if I'm wrong.

While the normalization probably isn't right, I'm curious about how this change here affected things. As this change in itself only affected compiler-rt, not clang, it shouldn't have changed where clang looks for libraries, right? Or has something else changed that, too?

I.e., even before this change, both an aarch64-none-elf-clang symlink, and invoking clang --target=aarch64-none-elf should have already looked in the <lib>/clang/16/lib/aarch64-none-unknown-elf directory, right? That is - which change here is it that has changed where clang looks for things?

peterwaller-arm · 2024-04-22T10:17:07Z

That is - which change here is it that has changed where clang looks for things?

I'm unclear where the breakage happened. I was building a toolchain with something like:

ARGS=(
    "-DLLVM_ENABLE_RUNTIMES=compiler-rt"
    "-DLLVM_BUILTIN_TARGETS=aarch64-none-elf;aarch64-unknown-linux-gnu;aarch64-unknown-linux-musl"
    "-DBUILTINS_aarch64-none-elf_COMPILER_RT_SOME_FLAG=..."
    "-DLLVM_RUNTIME_TARGETS=aarch64-none-elf"
    "-DRUNTIMES_aarch64-none-elf_COMPILER_RT_SOME_FLAG=..."
)

With recent changes landed[0], the installed paths to the binaries changed. I think a different process outside of llvm-project set up to make things work (symlinking some paths into /lib/clang-runtimes/) ceased working because compiler-rt now gets installed to aarch64-none-unknown-elf. Fixing that process to use the correct fully qualified triple (aarch64-unknown-none-elf) isn't enough, even if the cmake invocation above is updated to use a full triple, because it still creates a disagreement in the triple normalization when the user then specifies a compiler invocation with aarch64-none-elf.

In summary, I think the breakage is caused by this change because there exists other things in the wild which expect --target=aarch64-none-elf to resolve at some point /lib/aarch64-none-elf or /lib/aarch64-unknown-none-elf, but this PR has the consequence of moving libraries to aarch64-none-unknown-elf.

[0] (and I suspect this commit specifically we're commenting on but don't have absolute confirmation other than a suspicion from seeing build after the revert of this patch fix it and then it broke again after it relanded).

mstorsjo · 2024-04-22T10:34:47Z

That is - which change here is it that has changed where clang looks for things?

I'm unclear where the breakage happened. I was building a toolchain with something like:
ARGS=(
    "-DLLVM_ENABLE_RUNTIMES=compiler-rt"
    "-DLLVM_BUILTIN_TARGETS=aarch64-none-elf;aarch64-unknown-linux-gnu;aarch64-unknown-linux-musl"
    "-DBUILTINS_aarch64-none-elf_COMPILER_RT_SOME_FLAG=..."
    "-DLLVM_RUNTIME_TARGETS=aarch64-none-elf"
    "-DRUNTIMES_aarch64-none-elf_COMPILER_RT_SOME_FLAG=..."
)
With recent changes landed[0], the installed paths to the binaries changed.

Ah, I see! Right, yes, this commit will have changed where files are installed in this case.

I think a different process outside of llvm-project set up to make things work (symlinking some paths into /lib/clang-runtimes/) ceased working because compiler-rt now gets installed to aarch64-none-unknown-elf.

Ok, so this is the missing clue that explains it to me. Previously, you didn't have clang actually consume the built libraries from the directory they were installed into, but moved them around into another path that clang does look in (based on another rule).

The intent of this commit, was that files are installed into a pathname where clang will look automatically, provided that it is invoked with the same target triple. Previously, it did install files into a directory that clang didn't look into, and hence you need to move/symlink files around.

Fixing that process to use the correct fully qualified triple (aarch64-unknown-none-elf) isn't enough, even if the cmake invocation above is updated to use a full triple, because it still creates a disagreement in the triple normalization when the user then specifies a compiler invocation with aarch64-none-elf.

In summary, I think the breakage is caused by this change because there exists other things in the wild which expect --target=aarch64-none-elf to resolve at some point /lib/aarch64-none-elf or /lib/aarch64-unknown-none-elf, but this PR has the consequence of moving libraries to aarch64-none-unknown-elf.

Hmm, right. There are a couple of different codepaths that add lookup of libraries in a bunch of different directories - some based on the full triple (e.g. what clang -print-target-triple, which is what this change uses), some based on a different kind of normalization of triples, e.g. to match the debian style multiarch directories - https://github.com/llvm/llvm-project/blob/llvmorg-19-init/clang/lib/Driver/ToolChains/Gnu.cpp#L2453-L2455. I guess each tool in ToolChains might have different rules for what it tries to look for here - and I guess some may look for things in the non-normalized directory as well.

peterwaller-arm · 2024-04-23T09:32:36Z

For anyone watching, #89638 by @wzssyqa seeks to fix the canonicalisation to reorder -none-unknown- to -unknown-none-.

peterwaller-arm · 2024-04-23T15:39:38Z

I've posted an RFC on the topic to discourse: https://discourse.llvm.org/t/rfc-baremetal-target-triple-normalization/78524

This is needed for a workaround to make sure the link later succeeds. I don't know the reason for that but it is definitely needed. llvm#89234 will/wants to correct the triple normalisation for -none- and this means that clang prior to 19, and clang 19 and above will have different answers and therefore different library paths. I don't want to bootstrap a clang just for libcxx CI, or require that anyone building for Arm do the same, so ask the compiler what the triple should be. This will be compatible with 17 and 19 when we do update to that version. I'm assuming $CC is what anyone locally would set to override the compiler, and `cc` is the binary name in our CI containers. It's not perfect but it should cover most use cases.

…90722) This is needed for a workaround to make sure the link later succeeds. I don't know the reason for that but it is definitely needed. #89234 will/wants to correct the triple normalisation for -none- and this means that clang prior to 19, and clang 19 and above will have different answers and therefore different library paths. I don't want to bootstrap a clang just for libcxx CI, or require that anyone building for Arm do the same, so ask the compiler what the triple should be. This will be compatible with 17 and 19 when we do update to that version. I'm assuming $CC is what anyone locally would set to override the compiler, and `cc` is the binary name in our CI containers. It's not perfect but it should cover most use cases.

wzssyqa requested a review from a team as a code owner April 18, 2024 13:45

llvmbot added compiler-rt libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. labels Apr 18, 2024

wzssyqa requested a review from DavidSpickett April 18, 2024 13:46

wzssyqa requested review from mstorsjo and MaskRay April 18, 2024 13:46

wzssyqa mentioned this pull request Apr 19, 2024

[Driver] Improve error when a compiler-rt library is not found #81037

Merged

wzssyqa merged commit d3925e6 into llvm:main Apr 19, 2024
54 checks passed

peterwaller-arm mentioned this pull request Apr 23, 2024

Triple::normalize: Use none as OS for XX-none-ABI #89638

Merged

DavidSpickett mentioned this pull request May 1, 2024

[libcxx][ci] In picolib build, ask clang for the normalised triple #90722

Merged

petrhosek added a commit to petrhosek/llvm-project that referenced this pull request Jun 25, 2024

Update the patch to incorporate changes from llvm#89234

302fa32

petrhosek mentioned this pull request Jun 25, 2024

[CMake] Use Clang to infer the target triple #89425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #89234

CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #89234

wzssyqa commented Apr 18, 2024 •

edited

Loading

llvmbot commented Apr 18, 2024

DavidSpickett commented Apr 18, 2024

mstorsjo commented Apr 18, 2024

wzssyqa commented Apr 19, 2024

wzssyqa commented Apr 19, 2024

petrhosek commented Apr 19, 2024

peterwaller-arm commented Apr 22, 2024

wzssyqa commented Apr 22, 2024 •

edited

Loading

peterwaller-arm commented Apr 22, 2024

wzssyqa commented Apr 22, 2024

DavidSpickett commented Apr 22, 2024 •

edited

Loading

mstorsjo commented Apr 22, 2024

peterwaller-arm commented Apr 22, 2024

mstorsjo commented Apr 22, 2024

peterwaller-arm commented Apr 23, 2024

peterwaller-arm commented Apr 23, 2024

CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #89234

CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #89234

Conversation

wzssyqa commented Apr 18, 2024 • edited Loading

llvmbot commented Apr 18, 2024

DavidSpickett commented Apr 18, 2024

mstorsjo commented Apr 18, 2024

wzssyqa commented Apr 19, 2024

wzssyqa commented Apr 19, 2024

petrhosek commented Apr 19, 2024

peterwaller-arm commented Apr 22, 2024

wzssyqa commented Apr 22, 2024 • edited Loading

peterwaller-arm commented Apr 22, 2024

wzssyqa commented Apr 22, 2024

DavidSpickett commented Apr 22, 2024 • edited Loading

mstorsjo commented Apr 22, 2024

peterwaller-arm commented Apr 22, 2024

mstorsjo commented Apr 22, 2024

peterwaller-arm commented Apr 23, 2024

peterwaller-arm commented Apr 23, 2024

wzssyqa commented Apr 18, 2024 •

edited

Loading

wzssyqa commented Apr 22, 2024 •

edited

Loading

DavidSpickett commented Apr 22, 2024 •

edited

Loading