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

petrhosek · 2024-04-19T18:03:07Z

When using Clang as a compiler, use Clang to normalize the triple that's used to construct path for runtime library build and install paths. This ensures that paths are consistent and avoids the issue where the build uses a different triple spelling.

Differential Revision: https://reviews.llvm.org/D140925

When using Clang as a compiler, use Clang to normalize the triple that's used to construct path for runtime library build and install paths. This ensures that paths are consistent and avoids the issue where the build uses a different triple spelling. Differential Revision: https://reviews.llvm.org/D140925

petrhosek · 2024-04-19T18:03:30Z

This is a re-upload of https://reviews.llvm.org/D140925, I plan to land this change if there are no objections.

llvmbot · 2024-04-19T18:03:41Z

@llvm/pr-subscribers-clang

Author: Petr Hosek (petrhosek)

Changes

When using Clang as a compiler, use Clang to normalize the triple that's used to construct path for runtime library build and install paths. This ensures that paths are consistent and avoids the issue where the build uses a different triple spelling.

Differential Revision: https://reviews.llvm.org/D140925

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

3 Files Affected:

(modified) clang/cmake/caches/Fuchsia-stage2.cmake (+1-1)
(modified) compiler-rt/lib/builtins/CMakeLists.txt (+13)
(modified) runtimes/CMakeLists.txt (+14)

diff --git a/clang/cmake/caches/Fuchsia-stage2.cmake b/clang/cmake/caches/Fuchsia-stage2.cmake
index d5546e20873b3c..029c069997396d 100644
--- a/clang/cmake/caches/Fuchsia-stage2.cmake
+++ b/clang/cmake/caches/Fuchsia-stage2.cmake
@@ -142,7 +142,7 @@ if(WIN32 OR LLVM_WINSYSROOT)
   set(RUNTIMES_${target}_CMAKE_MODULE_LINKER_FLAGS ${WINDOWS_LINK_FLAGS} CACHE STRING "")
 endif()
 
-foreach(target aarch64-unknown-linux-gnu;armv7-unknown-linux-gnueabihf;i386-unknown-linux-gnu;riscv64-unknown-linux-gnu;x86_64-unknown-linux-gnu)
+foreach(target aarch64-linux-gnu;armv7-linux-gnueabihf;i386-linux-gnu;riscv64-linux-gnu;x86_64-linux-gnu)
   if(LINUX_${target}_SYSROOT)
     # Set the per-target builtins options.
     list(APPEND BUILTIN_TARGETS "${target}")
diff --git a/compiler-rt/lib/builtins/CMakeLists.txt b/compiler-rt/lib/builtins/CMakeLists.txt
index f9611574a562b4..4c6de992204c16 100644
--- a/compiler-rt/lib/builtins/CMakeLists.txt
+++ b/compiler-rt/lib/builtins/CMakeLists.txt
@@ -28,6 +28,19 @@ if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   if (NOT LLVM_RUNTIMES_BUILD)
     load_llvm_config()
   endif()
+  if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+    set(print_target_triple ${CMAKE_CXX_COMPILER} --target=${LLVM_RUNTIME_TRIPLE} -print-target-triple)
+    execute_process(COMMAND ${print_target_triple}
+      RESULT_VARIABLE result
+      OUTPUT_VARIABLE output
+      OUTPUT_STRIP_TRAILING_WHITESPACE)
+    if(result EQUAL 0)
+      set(LLVM_RUNTIME_TRIPLE ${output})
+    else()
+      string(REPLACE ";" " " print_target_triple "${print_target_triple}")
+      message(WARNING "Failed to execute `${print_target_triple}` to normalize target triple.")
+    endif()
+  endif()
   construct_compiler_rt_default_triple()
 
   include(SetPlatformToolchainTools)
diff --git a/runtimes/CMakeLists.txt b/runtimes/CMakeLists.txt
index 6f24fbcccec955..bb1f544706f2bf 100644
--- a/runtimes/CMakeLists.txt
+++ b/runtimes/CMakeLists.txt
@@ -181,6 +181,20 @@ message(STATUS "LLVM default target triple: ${LLVM_DEFAULT_TARGET_TRIPLE}")
 
 set(LLVM_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}")
 
+if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+  set(print_target_triple ${CMAKE_CXX_COMPILER} --target=${LLVM_RUNTIME_TRIPLE} -print-target-triple)
+  execute_process(COMMAND ${print_target_triple}
+    RESULT_VARIABLE result
+    OUTPUT_VARIABLE output
+    OUTPUT_STRIP_TRAILING_WHITESPACE)
+  if(result EQUAL 0)
+    set(LLVM_RUNTIME_TRIPLE ${output})
+  else()
+    string(REPLACE ";" " " print_target_triple "${print_target_triple}")
+    message(WARNING "Failed to execute `${print_target_triple}` to normalize target triple.")
+  endif()
+endif()
+
 option(LLVM_INCLUDE_TESTS "Generate build targets for the runtimes unit tests." ON)
 option(LLVM_INCLUDE_DOCS "Generate build targets for the runtimes documentation." ON)
 option(LLVM_ENABLE_SPHINX "Use Sphinx to generate the runtimes documentation." OFF)

wzssyqa · 2024-04-19T23:50:17Z

Does it really needed?

wzssyqa · 2024-04-19T23:51:40Z

clang/cmake/caches/Fuchsia-stage2.cmake

@@ -142,7 +142,7 @@ if(WIN32 OR LLVM_WINSYSROOT)
  set(RUNTIMES_${target}_CMAKE_MODULE_LINKER_FLAGS ${WINDOWS_LINK_FLAGS} CACHE STRING "")
 endif()

-foreach(target aarch64-unknown-linux-gnu;armv7-unknown-linux-gnueabihf;i386-unknown-linux-gnu;riscv64-unknown-linux-gnu;x86_64-unknown-linux-gnu)
+foreach(target aarch64-linux-gnu;armv7-linux-gnueabihf;i386-linux-gnu;riscv64-linux-gnu;x86_64-linux-gnu)


It seems offtopic of commit msg?
Since the normalize format of aarch64-linux-gnu is just aarch64-unknown-linux-gnu, why do we need to modify them?

I included this as a regression test, without this change the build of our toolchain would break, but I'm fine omitting this if you prefer.

compiler-rt/lib/builtins/CMakeLists.txt

runtimes/CMakeLists.txt

compiler-rt/lib/builtins/CMakeLists.txt

ldionne

LGTM, the equivalent patch was approved on Phab too.

…ce-dir-triple

petrhosek · 2024-06-25T07:53:35Z

Does it really needed?

It is, #89234 only covers compiler-rt but not other runtime libraries such as libc or libc++ so you might end up with the following:

lib/19/clang/lib/x86_64-unknown-linux-gnu/libclang_rt.builtins.a
lib/x86_64-linux-gnu/libc++.a

The driver will then fail to fine libc++.a leading to a link failure.

This change expand the normalization to cover all runtime libraries.

wzssyqa · 2024-06-25T09:54:34Z

compiler-rt/cmake/Modules/CompilerRTUtils.cmake

+      set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${output})
+    else()
+      string(REPLACE ";" " " print_target_triple "${print_target_triple}")
+      message(WARNING "Failed to execute `${print_target_triple}` to normalize target triple.")


I think that it should be a fetal error instead of just a warning?

Currently we ignore any errors so my preference would be to start with a warning and change it to an error in a follow up change after we verify there are no issues.

Maybe you are right. I tested with some quite bad --target values, it will output the original values.
So, I think that it will be safe. Anyway, a warning is OK for me as I think that it won't be used at all.

wzssyqa · 2024-06-25T09:56:16Z

runtimes/CMakeLists.txt

+  if(result EQUAL 0)
+    set(LLVM_DEFAULT_TARGET_TRIPLE ${output})
+  else()
+    string(REPLACE ";" " " print_target_triple "${print_target_triple}")


Ditto. I guess that we need to test on more target. As some target doesn't use the new filesystem struct.

Since most of the CIs monitor the commit only. I have no idea how to trigger them before commit.

petrhosek · 2024-07-03T15:04:25Z

@wzssyqa Is it OK with you if I go ahead and merge this PR? I plan to follow up with further improvements but this is necessary to fix the build of LLVM runtime libraries in the bootstrapping build.

ldionne · 2024-07-03T15:35:12Z

@wzssyqa Is it OK with you if I go ahead and merge this PR? I plan to follow up with further improvements but this is necessary to fix the build of LLVM runtime libraries in the bootstrapping build.

Is there a filed issue linked to this?

wzssyqa · 2024-07-05T07:37:36Z

@wzssyqa Is it OK with you if I go ahead and merge this PR? I plan to follow up with further improvements but this is necessary to fix the build of LLVM runtime libraries in the bootstrapping build.

It is OK for me, while I have no permission to approve such a patch.

petrhosek · 2024-07-06T05:56:09Z

@wzssyqa Is it OK with you if I go ahead and merge this PR? I plan to follow up with further improvements but this is necessary to fix the build of LLVM runtime libraries in the bootstrapping build.

Is there a filed issue linked to this?

I filed #97876 and left a TODO comment.

We switched to denormalized triples in llvm#89425 but that introduced a lot of downstream issues so switch back to normalized triples.

We switched to denormalized triples in #89425 but that introduced a lot of downstream issues so switch back to normalized triples.

We switched to denormalized triples in llvm#89425 but that introduced a lot of downstream issues so switch back to normalized triples.

This is a temporary work around to make the host triple names to be in compliance with llvm/llvm-project#89425. Original-Bug: 351847167 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/1078114 Original-Revision: cf50d5e45116c4146c17bcb1795c7aeceaef957d GitOrigin-RevId: 2f2b88cd676080ace3074ce7b96e6ae804dd11f3 Change-Id: I43e565bae1101dea1e03093a92f2e87a31878874

This reverts commit cf50d5e45116c4146c17bcb1795c7aeceaef957d. Reason for revert: Upstream affecting change reverted Original change's description: > [clang] Update host triple for Linux > > This is a temporary work around to make the host triple names to be > in compliance with llvm/llvm-project#89425. > > Original-Bug: 351847167 > Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/1078114 Original-Bug: 351847167 No-Presubmit: true No-Tree-Checks: true No-Try: true Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/1078377 Original-Revision: 67e297e1d48d3a8a164ac6642c745d35530d4d0c GitOrigin-RevId: 639b5d162831cc765280bb929ba1b2fd001776ce Change-Id: I2ee22447be790fe3afe4010f2019ddb6c655dacd

petrhosek assigned ldionne and smeenai Apr 19, 2024

petrhosek requested a review from a team as a code owner April 19, 2024 18:03

llvmbot added cmake Build system in general and CMake in particular clang Clang issues not falling into any other category compiler-rt compiler-rt:builtins labels Apr 19, 2024

petrhosek mentioned this pull request Apr 19, 2024

CompilerRT: Normalize COMPILER_RT_DEFAULT_TARGET_TRIPLE #89234

Merged

wzssyqa requested changes Apr 20, 2024

View reviewed changes

wzssyqa reviewed Apr 20, 2024

View reviewed changes

compiler-rt/lib/builtins/CMakeLists.txt Outdated Show resolved Hide resolved

ldionne approved these changes Apr 22, 2024

View reviewed changes

petrhosek added 2 commits June 25, 2024 06:38

Merge remote-tracking branch 'origin/main' into runtimes-clang-resour…

2c89c50

…ce-dir-triple

Update the patch to incorporate changes from llvm#89234

302fa32

wzssyqa reviewed Jun 25, 2024

View reviewed changes

Add a TODO comment

5044cde

petrhosek merged commit 9cb9a97 into llvm:main Jul 6, 2024
8 of 12 checks passed

petrhosek mentioned this pull request Jul 9, 2024

[CMake][Fuchsia] Revert back to using normalized triples #98189

Merged

zeroomega pushed a commit that referenced this pull request Jul 9, 2024

[CMake][Fuchsia] Revert back to using normalized triples (#98189)

d9e4f2c

We switched to denormalized triples in #89425 but that introduced a lot of downstream issues so switch back to normalized triples.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petrhosek commented Apr 19, 2024

petrhosek commented Apr 19, 2024

llvmbot commented Apr 19, 2024

wzssyqa commented Apr 19, 2024

wzssyqa Apr 19, 2024

petrhosek Jun 25, 2024

ldionne left a comment

petrhosek commented Jun 25, 2024

wzssyqa Jun 25, 2024

petrhosek Jul 3, 2024

wzssyqa Jul 5, 2024

wzssyqa Jun 25, 2024

petrhosek Jul 3, 2024

wzssyqa Jul 5, 2024

petrhosek commented Jul 3, 2024

ldionne commented Jul 3, 2024

wzssyqa commented Jul 5, 2024

petrhosek commented Jul 6, 2024

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

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

Conversation

petrhosek commented Apr 19, 2024

petrhosek commented Apr 19, 2024

llvmbot commented Apr 19, 2024

wzssyqa commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

petrhosek commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrhosek commented Jul 3, 2024

ldionne commented Jul 3, 2024

wzssyqa commented Jul 5, 2024

petrhosek commented Jul 6, 2024