[DTLTO][TEST] Fix the DTLTO Clang driver tests failing on some bots #159050
Conversation
…bots Not all builds name the compiler executable clang. For example, the Fuchsia buildbots use llvm as their single toolchain executable name, as they combine everything together in a busybox-style binary. This is currently causing the new ps5-dtlto.c to fail on such build bots. Update the Clang driver tests to simply check that a non-empty path is provided for the --thinlto-remote-compiler argument, rather than hardcoding the executable name. The cross-project tests will verify that the path is valid later. This is the same fix as applied earlier in llvm#148908. However, that fix left a case in the dtlto.c test that was subsequently reflected into the new ps5-dtlto.c test where it caused a failure. Why it doesn't cause a failure in the existing dtlto.c test is a mystery to me - perhaps the substring "clang" is now included in the path to the busybox-style binary, or perhaps that test was disabled for affected buildbots and then not re-enabled? It's clearly a latent issue though so I have also fixed the dtlto.c test in this patch. Should fix the buildbot failures caused by: llvm#158041.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: bd1976bris (bd1976bris) ChangesNot all builds name the compiler executable clang. For example, the Fuchsia buildbots use llvm as their single toolchain executable name, as they combine everything together in a busybox-style binary. This is currently causing the new ps5-dtlto.c to fail on such build bots. Update the Clang driver tests to simply check that a non-empty path is provided for the --thinlto-remote-compiler argument, rather than hardcoding the executable name. The cross-project tests will verify that the path is valid later. This is the same fix as applied earlier in #148908. However, that fix left a case in the dtlto.c test that was subsequently reflected into the new ps5-dtlto.c test where it caused a failure. Why it doesn't cause a failure in the existing dtlto.c test is a mystery to me - perhaps the substring "clang" is now included in the path to the busybox-style binary, or perhaps that test was disabled for affected buildbots and then not re-enabled? It's clearly a latent issue though so I have also fixed the dtlto.c test in this patch. Should fix the buildbot failures caused by: #158041. Full diff: https://github.com/llvm/llvm-project/pull/159050.diff 2 Files Affected:
diff --git a/clang/test/Driver/DTLTO/dtlto.c b/clang/test/Driver/DTLTO/dtlto.c
index 96795d9a4e6a4..bb1bd4ace0487 100644
--- a/clang/test/Driver/DTLTO/dtlto.c
+++ b/clang/test/Driver/DTLTO/dtlto.c
@@ -35,7 +35,7 @@
// DEFAULT: ld.lld
// DEFAULT-SAME: "--thinlto-distributor=d.exe"
-// DEFAULT-SAME: "--thinlto-remote-compiler={{.*}}clang{{[^\"]*}}"
+// DEFAULT-SAME: "--thinlto-remote-compiler={{[^"]+}}"
/// Check that nothing is forwarded when the compiler is not in LTO mode, and that
/// appropriate unused option warnings are issued.
diff --git a/clang/test/Driver/DTLTO/ps5-dtlto.c b/clang/test/Driver/DTLTO/ps5-dtlto.c
index 9b70c88257a85..a37073b85aa32 100644
--- a/clang/test/Driver/DTLTO/ps5-dtlto.c
+++ b/clang/test/Driver/DTLTO/ps5-dtlto.c
@@ -35,7 +35,7 @@
// DEFAULT: prospero-lld
// DEFAULT-SAME: "--thinlto-distributor=d.exe"
-// DEFAULT-SAME: "--thinlto-remote-compiler={{.*}}clang{{[^\"]*}}"
+// DEFAULT-SAME: "--thinlto-remote-compiler={{[^"]+}}"
/// Check that the arguments are forwarded unconditionally even when the
/// compiler is not in LTO mode.
|
bd1976bris
requested review from
andrewb1999 and
nga888
and removed request for
andrewb1999
|
@petrhosek thanks for the approval. After a deeper investigation by my colleague Andrew, we found that the changes here merely mask the real issue: the DTLTO Clang driver changes do not work with the Multicall toolchain executable. I’m going to close this and replace it with a change that XFAILs these tests under Multicall. |
Thanks, would it be also possible to file an issue with the details and reference it in the test? |
|
Done. #159129. |
3 participants
Not all builds name the compiler executable clang. For example, the Fuchsia buildbots use llvm as their single toolchain executable name, as they combine everything together in a busybox-style binary. This is currently causing the new ps5-dtlto.c to fail on such build bots.
Update the Clang driver tests to simply check that a non-empty path is provided for the --thinlto-remote-compiler argument, rather than hardcoding the executable name. The cross-project tests will verify that the path is valid later.
This is the same fix as applied earlier in #148908. However, that fix left a case in the dtlto.c test that was subsequently reflected into the new ps5-dtlto.c test where it caused a failure. Why it doesn't cause a failure in the existing dtlto.c test is a mystery to me - perhaps the substring "clang" is now included in the path to the busybox-style binary, or perhaps that test was disabled for affected buildbots and then not re-enabled? It's clearly a latent issue though so I have also fixed the dtlto.c test in this patch.
Should fix the buildbot failures caused by: #158041.