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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

weliveindetail
Copy link
Contributor

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.

Copy link
Contributor Author

@weliveindetail weliveindetail left a comment

Choose a reason for hiding this comment

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

Two minor notes

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"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

COMPILER_RT_BUILD_ORC = "NO";
COMPILER_RT_BUILD_XRAY = "NO";
COMPILER_RT_BUILD_PROFILE = "YES";
COMPILER_RT_BUILD_SANITIZERS = "YES";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@weliveindetail
Copy link
Contributor Author

@swift-ci Please test Windows

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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"
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Is this still needed?

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

Choose a reason for hiding this comment

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

Is this still needed?

COMPILER_RT_BUILD_ORC = "NO";
COMPILER_RT_BUILD_XRAY = "NO";
COMPILER_RT_BUILD_PROFILE = "YES";
COMPILER_RT_BUILD_SANITIZERS = "YES";
Copy link
Member

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants