Skip to content

[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

Merged
merged 2 commits into from
Mar 18, 2025

Conversation

pratlucas
Copy link
Contributor

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.

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.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Mar 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 6, 2025

@llvm/pr-subscribers-clang

Author: Lucas Duarte Prates (pratlucas)

Changes

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.


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

6 Files Affected:

  • (modified) clang/test/Driver/apple-arm64-arch.c (+1-1)
  • (modified) clang/test/Driver/compilation_database_multiarch.c (+1-1)
  • (modified) clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c (+1-1)
  • (modified) clang/test/Driver/mtargetos-darwin.c (+1-1)
  • (modified) clang/test/Driver/xros-driver-requires-darwin-host.c (+1-1)
  • (modified) clang/test/Index/pch-from-libclang.c (+1-1)
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

@llvmbot
Copy link
Member

llvmbot commented Mar 6, 2025

@llvm/pr-subscribers-clang-driver

Author: Lucas Duarte Prates (pratlucas)

Changes

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.


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

6 Files Affected:

  • (modified) clang/test/Driver/apple-arm64-arch.c (+1-1)
  • (modified) clang/test/Driver/compilation_database_multiarch.c (+1-1)
  • (modified) clang/test/Driver/darwin-ld-platform-version-macos-requires-darwin.c (+1-1)
  • (modified) clang/test/Driver/mtargetos-darwin.c (+1-1)
  • (modified) clang/test/Driver/xros-driver-requires-darwin-host.c (+1-1)
  • (modified) clang/test/Index/pch-from-libclang.c (+1-1)
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

Copy link
Collaborator

@statham-arm statham-arm left a 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?

@cyndyishida
Copy link
Member

cyndyishida commented Mar 10, 2025

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.

This doesn't happen for tests with target=<arch>-apple<platform&version> right? It's not clear to me why the tests that explicitly pass an alternative way to set the target triple (e.g. with env vars or with mtargetos) fail. It would be good to diagnose that first.

Can you confirm my understanding of -mtargetos?

This option should set the darwin target triple. It's the generic equivalent of

      -macosx_version_min <value>
       -ios_version_min <value>
       -watchos_version_min <value>
       -tvos_version_min <value>

@cachemeifyoucan
Copy link
Collaborator

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:

  • Maybe pass a platform for the failing tests so there is no need for system-darwin or target for REQUIRES
  • The bot should pass the correct host target triple otherwise the clang driver might just not function correctly when you cross-compile.

@pratlucas
Copy link
Contributor Author

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
targets are included in the build". The actual scenario we have in our toolchain is that we explicitly set the value of LLVM_DEFAULT_TARGET_TRIPLE in our CMake files.
With this, the starting target triple used by clang won't be related to darwin, even when running on a darwin host platform. (Note that this is intentional, as our toolchains are only designed to target embedded and linux environments)

This doesn't happen for tests with target=-apple<platform&version> right? It's not clear to me why the tests that explicitly pass an alternative way to set the target triple (e.g. with env vars or with mtargetos) fail. It would be good to diagnose that first.

The issue doesn't happen if the --target option is used. This happens because the -arch and -mtargetos options don't control the entire target triple, but only modify specific parts of it (arch and OS). They are also only considered when the original target is a darwin one, taking no effect otherwise:

  • The processing of the -arch option is guarded by the triple's isOSBinFormatMachO() method in the driver.
  • The -mtargetos= option is only ever evaluated with the Darwin Toolchain implementation, as part of Darwin::AddDeploymentTarget. This is only reachable if the Darwin Toolchain is used by clang, which in turn requires a darwin target triple.

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 REQUIRES annotations.

Do you have a bot or build to point to for test failures?

Unfortunately our CI runs are not public yet, but here's the error output for reference:

<src_path>/clang/test/Driver/apple-arm64-arch.c:6:11: error: CHECK: expected string not found in input
// CHECK: "-triple" "arm64-apple-macosx{{[0-9.]+}}"
          ^
<stdin>:1:1: note: scanning from here
clang version 21.0.0git
^
<stdin>:8:89: note: possible intended match here
 "<src_path>/build/llvm/bin/clang-20" "-cc1" "-triple" "aarch64-unknown-linux-gnu" "-emit-obj" "-disable-free" "-clear-ast-before-backend" "-disable-llvm-verifier" "-discard-value-names" "-main-file-name" "apple-arm64-arch.c" "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-mframe-pointer=non-leaf" "-fmath-errno" "-ffp-contract=on" "-fno-rounding-math" "-mconstructor-aliases" "-funwind-tables=2" "-enable-tlsdesc" "-target-cpu" "generic" "-target-feature" "+v8a" "-target-feature" "+fp-armv8" "-target-feature" "+neon" "-target-abi" "aapcs" "-debugger-tuning=gdb" "-fdebug-compilation-dir=<src_path>/build/llvm/tools/clang/test/Driver" "-target-linker-version" "1115.7.3" "-fcoverage-compilation-dir=<src_path>/build/llvm/tools/clang/test/Driver" "-resource-dir" "<src_path>/build/llvm/lib/clang/21" "-internal-isystem" "<src_path>/build/llvm/lib/clang/21/include" "-internal-isystem" "/usr/local/include" "-internal-externc-isystem" "/include" "-internal-externc-isystem" "/usr/include" "-ferror-limit" "19" "-fno-signed-char" "-fgnuc-version=4.2.1" "-fskip-odr-check-in-gmf" "-target-feature" "-fmv" "-faddrsig" "-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o" "apple-arm64-arch.o" "-x" "c" "<src_path>/clang/test/Driver/apple-arm64-arch.c"
                                                                                        ^

Input file: <stdin>
Check file: <src_path>/clang/test/Driver/apple-arm64-arch.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<
           1: clang version 21.0.0git 
check:6'0     X~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
           2: Target: aarch64-unknown-linux-gnu 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           3: Thread model: posix 
check:6'0     ~~~~~~~~~~~~~~~~~~~~
           4: InstalledDir: <src_path>/build/llvm/bin 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           5: Arm Toolchain ID: E0000-pre (a6d8b454) 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           6: clang: error: unsupported option '-arch' for target 'aarch64-linux-gnu' 

@pratlucas
Copy link
Contributor Author

Maybe pass a platform for the failing tests so there is no need for system-darwin or target for REQUIRES

Do you mean by adding a --target option to the tests? I think this would also be a valid solution, but I can't tell if this would go against the original intent of the tests. (i.e. were they designed to test these options combined with the default triple inference? Or the options alone?)

Copy link
Collaborator

@cachemeifyoucan cachemeifyoucan left a 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.

@@ -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{{.*}}
Copy link
Collaborator

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?

Copy link
Contributor Author

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:

  1. c-index-test uses libclang to get the translation unit for the input file using clang_parseTranslationUnit2.
  2. The flow of clang_parseTranslationUnit2 creates a compiler invocation with arguments based on the current target information, and includes the arguments provided by c-index-test (via the -Xclang arguments).
  3. 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.

@pratlucas
Copy link
Contributor Author

I am ok with the fix but it will be good to update the REQUIRES to cover MacOSX target as well.

I've updated the annotations to cover MacOSX too.

Copy link
Collaborator

@cachemeifyoucan cachemeifyoucan left a 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.

@pratlucas pratlucas merged commit 44e4b27 into llvm:main Mar 18, 2025
11 checks passed
@pratlucas pratlucas deleted the darwin-requires branch March 18, 2025 09:11
pratlucas added a commit to pratlucas/arm-toolchain that referenced this pull request Mar 18, 2025
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.
pratlucas added a commit to arm/arm-toolchain that referenced this pull request Mar 18, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants