-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang] Fix darwin-related tests' REQUIRES annotation #130138
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
Conversation
The tests updated by this commit were designed to check features in the clang's driver and index that require clang to be targgeting a darwin platform while running on a darwin host. For that, their execution is currently gated by the `REQUIRES: system-darwin` annotation. This approach becomes a problem when trying to run such tests on a cross-compiling build of clang on a darwin platform. When no darwin targets are included in the build, the tests will still run on a darwin host and fail spuriously because of the missing target. To fix this issue, this patch introduces an extra condition to the tests' REQUIRES annotation, `target={{.*}}-darwin{{.*}}`, ensuring they only run when the relevant target is present.
@llvm/pr-subscribers-clang Author: Lucas Duarte Prates (pratlucas) ChangesThe tests updated by this commit were designed to check features in the This approach becomes a problem when trying to run such tests on a To fix this issue, this patch introduces an extra condition to the Full diff: https://github.com/llvm/llvm-project/pull/130138.diff 6 Files Affected:
diff --git a/clang/test/Driver/apple-arm64-arch.c b/clang/test/Driver/apple-arm64-arch.c
index a111260b38a6b..4378e6c36867a 100644
--- a/clang/test/Driver/apple-arm64-arch.c
+++ b/clang/test/Driver/apple-arm64-arch.c
@@ -1,6 +1,6 @@
// RUN: env SDKROOT="/" %clang -arch arm64 -c -### %s 2>&1 | \
// RUN: FileCheck %s
//
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
//
// CHECK: "-triple" "arm64-apple-macosx{{[0-9.]+}}"
diff --git a/clang/test/Driver/compilation_database_multiarch.c b/clang/test/Driver/compilation_database_multiarch.c
index 1540a8d29ec5c..5eecf8647cb1f 100644
--- a/clang/test/Driver/compilation_database_multiarch.c
+++ b/clang/test/Driver/compilation_database_multiarch.c
@@ -1,4 +1,4 @@
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
// RUN: rm -rf %t && mkdir -p %t
// RUN: %clang -fdriver-only -o %t/out %s -mtargetos=macos12 -arch arm64 -arch x86_64 -MJ %t/compilation_database.json
diff --git a/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c b/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c
index 6026ab5d41d34..c752d1fe7898f 100644
--- a/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c
+++ b/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c
@@ -7,4 +7,4 @@
// CHECK: "-platform_version" "macos" "{{[0-9]+}}.0.0" "{{[0-9]+}}.{{[0-9]+}}"
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
diff --git a/clang/test/Driver/mtargetos-darwin.c b/clang/test/Driver/mtargetos-darwin.c
index 7e86ab15279b9..7aba84a7b0051 100644
--- a/clang/test/Driver/mtargetos-darwin.c
+++ b/clang/test/Driver/mtargetos-darwin.c
@@ -11,7 +11,7 @@
// RUN: not %clang -mtargetos=darwin20 -arch arm64 -c %s -o %t.o -### 2>&1 | FileCheck --check-prefix=INVALIDOS %s
// RUN: not %clang -mtargetos=ios -arch arm64 -c %s -o %t.o -### 2>&1 | FileCheck --check-prefix=NOVERSION %s
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
// MACOS: "-cc1" "-triple" "arm64-apple-macosx11.0.0"
// MACOS-NEXT: "-cc1" "-triple" "x86_64-apple-macosx11.0.0"
diff --git a/clang/test/Driver/xros-driver-requires-darwin-host.c b/clang/test/Driver/xros-driver-requires-darwin-host.c
index e5bfccae2c209..70b63159b0671 100644
--- a/clang/test/Driver/xros-driver-requires-darwin-host.c
+++ b/clang/test/Driver/xros-driver-requires-darwin-host.c
@@ -1,4 +1,4 @@
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
// RUN: env XROS_DEPLOYMENT_TARGET=1.0 %clang -arch arm64 -c -### %s 2>&1 | FileCheck %s
diff --git a/clang/test/Index/pch-from-libclang.c b/clang/test/Index/pch-from-libclang.c
index 52722b629982c..807bc8c1b0ae4 100644
--- a/clang/test/Index/pch-from-libclang.c
+++ b/clang/test/Index/pch-from-libclang.c
@@ -18,7 +18,7 @@
// RUN: c-index-test -test-load-source local %s -include %t.clang.h -fmodules -fmodules-cache-path=%t.mcp -Xclang -triple -Xclang x86_64-apple-darwin | FileCheck %s
// FIXME: Still fails on at least some linux boxen.
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
#ifndef HEADER
#define HEADER
|
@llvm/pr-subscribers-clang-driver Author: Lucas Duarte Prates (pratlucas) ChangesThe tests updated by this commit were designed to check features in the This approach becomes a problem when trying to run such tests on a To fix this issue, this patch introduces an extra condition to the Full diff: https://github.com/llvm/llvm-project/pull/130138.diff 6 Files Affected:
diff --git a/clang/test/Driver/apple-arm64-arch.c b/clang/test/Driver/apple-arm64-arch.c
index a111260b38a6b..4378e6c36867a 100644
--- a/clang/test/Driver/apple-arm64-arch.c
+++ b/clang/test/Driver/apple-arm64-arch.c
@@ -1,6 +1,6 @@
// RUN: env SDKROOT="/" %clang -arch arm64 -c -### %s 2>&1 | \
// RUN: FileCheck %s
//
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
//
// CHECK: "-triple" "arm64-apple-macosx{{[0-9.]+}}"
diff --git a/clang/test/Driver/compilation_database_multiarch.c b/clang/test/Driver/compilation_database_multiarch.c
index 1540a8d29ec5c..5eecf8647cb1f 100644
--- a/clang/test/Driver/compilation_database_multiarch.c
+++ b/clang/test/Driver/compilation_database_multiarch.c
@@ -1,4 +1,4 @@
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
// RUN: rm -rf %t && mkdir -p %t
// RUN: %clang -fdriver-only -o %t/out %s -mtargetos=macos12 -arch arm64 -arch x86_64 -MJ %t/compilation_database.json
diff --git a/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c b/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c
index 6026ab5d41d34..c752d1fe7898f 100644
--- a/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c
+++ b/clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c
@@ -7,4 +7,4 @@
// CHECK: "-platform_version" "macos" "{{[0-9]+}}.0.0" "{{[0-9]+}}.{{[0-9]+}}"
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
diff --git a/clang/test/Driver/mtargetos-darwin.c b/clang/test/Driver/mtargetos-darwin.c
index 7e86ab15279b9..7aba84a7b0051 100644
--- a/clang/test/Driver/mtargetos-darwin.c
+++ b/clang/test/Driver/mtargetos-darwin.c
@@ -11,7 +11,7 @@
// RUN: not %clang -mtargetos=darwin20 -arch arm64 -c %s -o %t.o -### 2>&1 | FileCheck --check-prefix=INVALIDOS %s
// RUN: not %clang -mtargetos=ios -arch arm64 -c %s -o %t.o -### 2>&1 | FileCheck --check-prefix=NOVERSION %s
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
// MACOS: "-cc1" "-triple" "arm64-apple-macosx11.0.0"
// MACOS-NEXT: "-cc1" "-triple" "x86_64-apple-macosx11.0.0"
diff --git a/clang/test/Driver/xros-driver-requires-darwin-host.c b/clang/test/Driver/xros-driver-requires-darwin-host.c
index e5bfccae2c209..70b63159b0671 100644
--- a/clang/test/Driver/xros-driver-requires-darwin-host.c
+++ b/clang/test/Driver/xros-driver-requires-darwin-host.c
@@ -1,4 +1,4 @@
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
// RUN: env XROS_DEPLOYMENT_TARGET=1.0 %clang -arch arm64 -c -### %s 2>&1 | FileCheck %s
diff --git a/clang/test/Index/pch-from-libclang.c b/clang/test/Index/pch-from-libclang.c
index 52722b629982c..807bc8c1b0ae4 100644
--- a/clang/test/Index/pch-from-libclang.c
+++ b/clang/test/Index/pch-from-libclang.c
@@ -18,7 +18,7 @@
// RUN: c-index-test -test-load-source local %s -include %t.clang.h -fmodules -fmodules-cache-path=%t.mcp -Xclang -triple -Xclang x86_64-apple-darwin | FileCheck %s
// FIXME: Still fails on at least some linux boxen.
-// REQUIRES: system-darwin
+// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}}
#ifndef HEADER
#define HEADER
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done as much of the review as I can. The syntax in the REQUIRES
lines looks sensible to me, and I see why most of these tests would depend on clang being configured to target its host Darwin platform by default: apple-arm64-arch.c
, compilation_database_multiarch.c
, darwin-ld-platform-version-macos-requires-darwin.c
and xros-driver-requires-darwin-host.c
specifically check for apple
or macos
in the compiler output without having provided one in the input command line, so it makes sense that they wouldn't behave like that if clang's host was Darwin but its default target was something else.
I'm less sure I understand mtargetos-darwin.c
, because I don't know what -mtargetos=...
means. I guess that's supposed to be a modification to the detail of the OS component of the target triple, and doesn't have the side effect of setting it to Darwin in the first place?
And I know I don't understand what pch-from-libclang.c
has to do with Darwin host or target, but the FIXME in the comment suggests that nobody else knows that either, so perhaps that doesn't block the commit.
Can you confirm my understanding of -mtargetos
?
This doesn't happen for tests with
This option should set the darwin target triple. It's the generic equivalent of
|
Do you have a bot or build to point to for test failures? There is no way to disable Darwin target from clang driver. I think the test might be related to the host platform is not Darwin so the driver didn't infer the target correctly without a default target triple? I can see two fixes:
|
For context, we're observing this issue as part of our CI builds for the arm/arm-toolchain project. Looking into it in more detail, I believe I didn't express myself correctly when I wrote "When no darwin
The issue doesn't happen if the
Given the above, I think it's reasonable to expect that these tests will only pass when the default target triple is a darwin one, matching the updated
Unfortunately our CI runs are not public yet, but here's the error output for reference:
|
Do you mean by adding a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fix makes sense for a linux toolchain running on Darwin. I am still surprised that you have a "system-darwin" platform where you don't want the default target triple to be darwin.
I am ok with the fix but it will be good to update the REQUIRES to cover MacOSX target as well.
clang/test/Index/pch-from-libclang.c
Outdated
@@ -18,7 +18,7 @@ | |||
// RUN: c-index-test -test-load-source local %s -include %t.clang.h -fmodules -fmodules-cache-path=%t.mcp -Xclang -triple -Xclang x86_64-apple-darwin | FileCheck %s | |||
|
|||
// FIXME: Still fails on at least some linux boxen. | |||
// REQUIRES: system-darwin | |||
// REQUIRES: system-darwin && target={{.*}}-darwin{{.*}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am interested how this failed since the triple is passed explicitly. Maybe the driver is not using the right "Toolchain" configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one indeed. It looks like the problem is a conflict between the target inference that happens by libclang and the target provided by the -Xclang
option, which is only used by the internal cc1 invocation.
Having a deeper look into it, c-index-tests
appears to follow this execution flow:
c-index-test
uses libclang to get the translation unit for the input file usingclang_parseTranslationUnit2
.- The flow of
clang_parseTranslationUnit2
creates a compiler invocation with arguments based on the current target information, and includes the arguments provided byc-index-test
(via the-Xclang
arguments). - When cc1 is called by the created compiler invocation, it will parse the target provided by
c-index-test
via-Xclang
together with other target information inferred by the compiler invocation.
The issue happens in step #2:
The target argument provided via -Xclang
is not applied to the libclang environment at this point, it is only included as an extra argument for the compiler invocation. This means that when libclang builds target options to pass on to cc1 it bases them on its own inferred target information, which doesn't include c-index-tests
's -Xclang
arguments. When the default target is not compatible with x86_64-apple-darwin
, libclang includes values to arguments such as target-cpu which will be rejected by the x86_64-apple-darwin
target when cc1 runs.
I've updated the annotations to cover MacOSX too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for looking into the question I have. I slightly prefer if it is an option, making your platform either be darwin, or be entirely not darwin. But since you only need test update, I am ok with it since it doesn't make the things too complicated.
Will be good if you can have a public bot for your configuration, otherwise future tests might not be aware of the existence of your special platform and break again.
This cherry-picks the upstream change from llvm/llvm-project#130138, which landed on the `main` branch, into our 20.x release. The change is included as a patch file for llvm.
This cherry-picks the upstream change from llvm/llvm-project#130138, which landed on the `main` branch, into our 20.x release. The change is included as a patch file for llvm.
The tests updated by this commit were designed to check features in the
clang's driver and index that require clang to be targgeting a darwin
platform while running on a darwin host. For that, their execution is
currently gated by the
REQUIRES: system-darwin
annotation.This approach becomes a problem when trying to run such tests on a
cross-compiling build of clang on a darwin platform. When no darwin
targets are included in the build, the tests will still run on a darwin
host and fail spuriously because of the missing target.
To fix this issue, this patch introduces an extra condition to the
tests' REQUIRES annotation,
target={{.*}}-darwin{{.*}}
, ensuring theyonly run when the relevant target is present.