[windows][toolchain] Build sanitizers and builtins standalone for all SDKs #78861

weliveindetail · 2025-01-23T19:04:15Z

Unified build of compiler-rt together with LLVM failed for the Android SDKs. It got too complicated to redirect the way LLVM would configure the nested build-trees. Standalone builds slightly increase build time, but they turned out much simpler and we end up with less duplication of definitions.

… SDKs

weliveindetail

Two minor notes

weliveindetail · 2025-01-23T19:19:21Z

utils/build.ps1

+function Build-Sanitizers([Platform]$Platform, $Arch) {
+  $BareTarget = $Arch.LLVMTarget.Replace("$AndroidAPILevel", "")
+  $LLVMDir = "$(Get-TargetProjectBinaryCache $Arch LLVM)\lib\cmake\llvm"
+  $InstallTo = "$($HostArch.ToolchainInstallRoot)\usr\lib\clang\19"


Can we keep it like that? I can probably get it from running stage-1 Clang, or lit

I think that we should extract this from the build, the resource dir changes on each rebranch, and this will complicate things.

Ok, done. I think asking llvm-config would usually be the right way, but for cross-compiles we cannot execute it. We are asking llvm-lit.py now. Seems more reliable than parsing a CMake config file or a header somewhere in the build-tree.

weliveindetail · 2025-01-23T19:20:49Z

utils/build.ps1

+      COMPILER_RT_BUILD_ORC = "NO";
+      COMPILER_RT_BUILD_XRAY = "NO";
+      COMPILER_RT_BUILD_PROFILE = "YES";
+      COMPILER_RT_BUILD_SANITIZERS = "YES";


I could put this into a CMake cache file, but it won't get much simpler

Yeah, I can see why it would not simplify much, but, it does make it more obvious that this is static configuration and not logic when you need a last second change to the build.

OTOH the structure in https://github.com/swiftlang/swift/tree/main/cmake/caches forces us to add 7 new files. And they all have identical content. (Unless we outsource CMAKE_SYSTEM_NAME, but it won't make that much of a difference.) I guess it should be: LLVM-$Platform-$($Arch.LLVMName).cmake

Could look like this for the two arches I am testing right now: weliveindetail@windows-toolchain-sanitizers-standalone-with-caches

Not sure. Or should I break the scheme and make just one?

weliveindetail · 2025-01-23T19:22:48Z

@swift-ci Please test Windows

compnerd · 2025-01-23T21:23:40Z

utils/build.ps1

@@ -1606,6 +1608,52 @@ function Build-LLVM([Platform]$Platform, $Arch) {
    }
 }

+function Build-Sanitizers([Platform]$Platform, $Arch) {
+  $BareTarget = $Arch.LLVMTarget.Replace("$AndroidAPILevel", "")


I believe that @andrurogerz is working on creating a ModuleTriple parameter in the architecture definition to help with this.

Oh good, yeah this difference is nifty. The unified build used to pass it via LLVM_RUNTIMES_TARGET
here, but in the standalone build CMake reports it as unused. Will drop it here.

compnerd · 2025-01-23T21:24:11Z

utils/build.ps1

@@ -1606,6 +1608,52 @@ function Build-LLVM([Platform]$Platform, $Arch) {
    }
 }

+function Build-Sanitizers([Platform]$Platform, $Arch) {
+  $BareTarget = $Arch.LLVMTarget.Replace("$AndroidAPILevel", "")
+  $LLVMDir = "$(Get-TargetProjectBinaryCache $Arch LLVM)\lib\cmake\llvm"


Should this be target? What happens if we don't have a target build of LLVM? (e.g. Android)

This came up as a warning during configuration. In many cases standalone builds reach out to LLVM for configuration details or test dependencies. That should work because we do configure LLVM for Android targets. Let me double-check that.

Done. It's required for testing and otherwise silences this warning: https://github.com/llvm/llvm-project/blob/57b48987f6c21e369e7bb1626dc79ca74aa34fdb/compiler-rt/cmake/Modules/CompilerRTUtils.cmake#L314

compnerd · 2025-01-23T21:24:49Z

utils/build.ps1

+function Build-Sanitizers([Platform]$Platform, $Arch) {
+  $BareTarget = $Arch.LLVMTarget.Replace("$AndroidAPILevel", "")
+  $LLVMDir = "$(Get-TargetProjectBinaryCache $Arch LLVM)\lib\cmake\llvm"
+  $InstallTo = "$($HostArch.ToolchainInstallRoot)\usr\lib\clang\19"


I think that we should extract this from the build, the resource dir changes on each rebranch, and this will complicate things.

compnerd · 2025-01-23T21:25:02Z

utils/build.ps1

+    -UseBuiltCompilers C,CXX `
+    -BuildTargets "install-compiler-rt" `
+    -Defines (@{
+      CMAKE_MT = "mt";


Is this still needed?

Oh right, it's not. Dropped.

compnerd · 2025-01-23T21:25:32Z

utils/build.ps1

+    -UseBuiltCompilers C,CXX `
+    -BuildTargets "install-compiler-rt" `
+    -Defines (@{
+      CMAKE_MT = "mt";


Is this still needed?

Dropped as well

compnerd · 2025-01-23T21:26:13Z

utils/build.ps1

+      COMPILER_RT_BUILD_ORC = "NO";
+      COMPILER_RT_BUILD_XRAY = "NO";
+      COMPILER_RT_BUILD_PROFILE = "YES";
+      COMPILER_RT_BUILD_SANITIZERS = "YES";


Yeah, I can see why it would not simplify much, but, it does make it more obvious that this is static configuration and not logic when you need a last second change to the build.

…e ones

weliveindetail · 2025-01-24T11:53:22Z

@swift-ci Please smoke test

weliveindetail · 2025-01-27T07:55:41Z

Build failure were unrelated

@swift-ci Please smoke test

weliveindetail · 2025-01-27T12:37:14Z

This should be ready to land once I reverted the temporary change that enabled Android x86_64 in CI. Configure & build took less than 1min per target. We can keep it enabled for all SDKs and shouldn't need a switch for opt-out.

This reverts commit ee702d9.

weliveindetail · 2025-01-27T12:45:36Z

utils/build.ps1

@@ -1599,13 +1601,61 @@ function Build-LLVM([Platform]$Platform, $Arch) {
    -Bin (Get-TargetProjectBinaryCache $Arch LLVM) `
    -Arch $Arch `
    -Platform $Platform `
-    -UseMSVCCompilers C,CXX `
+    -UseBuiltCompilers C,CXX `


This is not strictly related, but I think it's fine to land with this PR.

weliveindetail · 2025-01-27T12:46:23Z

@swift-ci Please smoke test

weliveindetail · 2025-01-28T11:43:26Z

The cross-build for Windows arm64 failed with:

FAILED: lib/aarch64-unknown-windows-msvc/clang_rt.builtins.lib 
C:\Windows\system32\cmd.exe /C "cd . && T:\5\bin\llvm-lib.exe /nologo /machine:ARM64 /out:lib\aarch64-unknown-windows-msvc\clang_rt.builtins.lib @CMakeFiles\clang_rt.builtins-aarch64.rsp && cd ."
CMakeFiles\clang_rt.builtins-aarch64.dir\outline_atomic_helpers.dir\outline_atomic_cas1_1.S.obj: file machine type x64 conflicts with library machine type arm64 (from '/machine:ARM64' flag)

ninja: build stopped: subcommand failed.
Error: Error: cmake.exe exited with code 1.
Invocation:
  cmake.exe --build T:\306 --target install-compiler-rt

The build step is generated here in CMake and there was a recent change (downstream in swiftlang LLVM) to skip it with MSVC: swiftlang/llvm-project@95a1047

I can reproduce the failure locally and keep investigating.

weliveindetail · 2025-01-28T12:09:13Z

@swift-ci Please smoke test

weliveindetail added 3 commits January 23, 2025 20:02

[windows][toolchain] Build sanitizers and builtins standalone for all…

3625597

… SDKs

Mark failing Swift sanitizer tests as XFAIL instead of UNSUPPORTED

fe50fea

[dev] Temporarily enable Android x86_64 in CI

ee702d9

weliveindetail requested review from compnerd, slavapestov, rjmccall and artemcm as code owners January 23, 2025 19:04

weliveindetail commented Jan 23, 2025

View reviewed changes

weliveindetail mentioned this pull request Jan 23, 2025

Reland "[windows][toolchain] Enable builtins and sanitizers" #78630

Closed

compnerd reviewed Jan 23, 2025

View reviewed changes

weliveindetail added 4 commits January 24, 2025 11:28

Configure target LLVM with stage-1 compilers and not the build machin…

9f57c93

…e ones

Infer LLVM major version from version string of llvm-lit.py

9c61e36

Drop LLVM_RUNTIMES_TARGET parameter from sanitizer config

86f4bcf

Drop CMAKE_MT parameter from both configs

7ec4021

Revert "[dev] Temporarily enable Android x86_64 in CI"

98f85d7

This reverts commit ee702d9.

weliveindetail commented Jan 27, 2025

View reviewed changes

Explicitly configure ASM compiler along with C and C++

e2f501f

weliveindetail merged commit 76243a2 into swiftlang:main Jan 28, 2025
3 checks passed

weliveindetail deleted the windows-toolchain-sanitizers-standalone branch January 28, 2025 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[windows][toolchain] Build sanitizers and builtins standalone for all SDKs #78861

[windows][toolchain] Build sanitizers and builtins standalone for all SDKs #78861

weliveindetail commented Jan 23, 2025

weliveindetail left a comment

weliveindetail Jan 23, 2025

compnerd Jan 23, 2025

weliveindetail Jan 24, 2025

weliveindetail Jan 23, 2025

compnerd Jan 23, 2025

weliveindetail Jan 24, 2025

weliveindetail Jan 24, 2025

weliveindetail commented Jan 23, 2025

compnerd Jan 23, 2025

weliveindetail Jan 24, 2025

weliveindetail Jan 24, 2025

compnerd Jan 23, 2025

weliveindetail Jan 24, 2025

weliveindetail Jan 24, 2025

compnerd Jan 23, 2025

compnerd Jan 23, 2025

weliveindetail Jan 24, 2025

compnerd Jan 23, 2025

weliveindetail Jan 24, 2025

compnerd Jan 23, 2025

weliveindetail commented Jan 24, 2025

weliveindetail commented Jan 27, 2025

weliveindetail commented Jan 27, 2025

weliveindetail Jan 27, 2025

weliveindetail commented Jan 27, 2025

weliveindetail commented Jan 28, 2025

weliveindetail commented Jan 28, 2025

[windows][toolchain] Build sanitizers and builtins standalone for all SDKs #78861

[windows][toolchain] Build sanitizers and builtins standalone for all SDKs #78861

Conversation

weliveindetail commented Jan 23, 2025

weliveindetail left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weliveindetail commented Jan 23, 2025

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

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

weliveindetail commented Jan 24, 2025

weliveindetail commented Jan 27, 2025

weliveindetail commented Jan 27, 2025

Choose a reason for hiding this comment

weliveindetail commented Jan 27, 2025

weliveindetail commented Jan 28, 2025

weliveindetail commented Jan 28, 2025