Skip to content
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

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

Merged
merged 2 commits into from
May 1, 2024

Conversation

DavidSpickett
Copy link
Collaborator

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.

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.
@DavidSpickett DavidSpickett requested a review from a team as a code owner May 1, 2024 10:53
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 1, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 1, 2024

@llvm/pr-subscribers-libcxx

Author: David Spickett (DavidSpickett)

Changes

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.


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

1 Files Affected:

  • (modified) libcxx/utils/ci/run-buildbot (+13-1)
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 60307a7d4f350a..132c98486de430 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -217,7 +217,19 @@ function test-armv7m-picolibc() {
         "${@}"
 
     ${NINJA} -vC "${BUILD_DIR}/compiler-rt" install
-    mv "${BUILD_DIR}/install/lib/armv7m-none-unknown-eabi"/* "${BUILD_DIR}/install/lib"
+
+    # Prior to clang 19, armv7m-none-eabi normalised to armv7m-none-unknown-eabi.
+    # clang 19 changed this to armv7m-unknown-none-eabi. So for as long as 18.x
+    # is supported, we have to ask clang what the triple will be.
+    if [ ! -z "${CC}" ]
+    then
+        C_COMPILER=${CC};
+    else
+        C_COMPILER=cc;
+    fi
+    NORMALISED_TARGET_TRIPLE=$(${C_COMPILER} --target=armv7m-none-eabi -print-target-triple)
+    # Without this step linking fails later in the build.
+    mv "${BUILD_DIR}/install/lib/${NORMALISED_TARGET_TRIPLE}"/* "${BUILD_DIR}/install/lib"
 
     check-runtimes
 }

@DavidSpickett
Copy link
Collaborator Author

This should work for the current state and when #89638 lands.

@DavidSpickett
Copy link
Collaborator Author

And if anyone is brave enough to build with gcc:

$ gcc --print-target-triple
gcc: error: unrecognized command line option ‘--print-target-triple’

At least they will be pointed right to the line they need to hack to make it work.

@wzssyqa
Copy link
Contributor

wzssyqa commented May 1, 2024

And if anyone is brave enough to build with gcc:

These scripts are so llvm-hardcoded, so it won't be a big problem to introduce one more ;)

LGTM.

Copy link
Contributor

@peterwaller-arm peterwaller-arm left a comment

Choose a reason for hiding this comment

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

LGTM, optional nits, since you might have valid reasons for writing it as you did.

libcxx/utils/ci/run-buildbot Outdated Show resolved Hide resolved
@DavidSpickett
Copy link
Collaborator Author

I'm going to land this without libcxx approval given that it is limited to the picolib build, is required to keep the CI green once the other PR lands, and has passed existing CI (aside from AIX 32 bit that I assume is a pre-existing issue).

Post-commit review welcome of course.

@DavidSpickett DavidSpickett merged commit 167b506 into llvm:main May 1, 2024
8 of 9 checks passed
@DavidSpickett DavidSpickett deleted the libcxx-triple branch May 1, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants