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.

Sign up for GitHub

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

Jump to bottom

[windows][toolchain] Enable builtins and sanitizers #77770

Merged
merged 11 commits into from
Dec 5, 2024
Merged

[windows][toolchain] Enable builtins and sanitizers #77770

merged 11 commits into from
Dec 5, 2024

Conversation

weliveindetail
Copy link
Contributor

Profile and builtin libraries from compiler-rt are static and do not require a link step. We can enable them in our CMake caches and we can build and install them in the unified LLVM build step. Sanitizers contain dynamic libraries. We need the target-specific Visual Studio shell to perform the link step. This patch adds extra build steps in build.ps1 that reconfigure, build and install the target-specific build-trees in the LLVM runtimes directory through their respective Visual Studio shells.

compnerd
compnerd reviewed Nov 21, 2024
View reviewed changes
utils/build.ps1 Outdated Show resolved Hide resolved
utils/build.ps1 Outdated Show resolved Hide resolved
@weliveindetail weliveindetail marked this pull request as ready for review November 26, 2024 15:57
@weliveindetail
Copy link
Contributor Author

weliveindetail commented Nov 28, 2024 •
edited
Loading

I was able to build and test the small asan example with this. Any reason not to merge this?

@hjyamauchi Great! I think this is ready to land. (Once I default-enabled them.)

@weliveindetail
Copy link
Contributor Author

Let's give the CI a try. If it works, this can land. Please note: I'd like to squash commits before/while merging.

@hjyamauchi
Copy link
Contributor

@swift-ci please test

@compnerd
Copy link
Member

compnerd commented Dec 2, 2024

@swift-ci please test Windows platform

compnerd
compnerd reviewed Dec 2, 2024
View reviewed changes
cmake/caches/Windows-aarch64.cmake Outdated Show resolved Hide resolved
cmake/caches/Windows-aarch64.cmake
foreach(target ${LLVM_RUNTIME_TARGETS})
set(RUNTIMES_${target}_LLVM_ENABLE_RUNTIMES
compiler-rt
CACHE STRING "")
set(RUNTIMES_${target}_CMAKE_MT mt CACHE STRING "")
set(RUNTIMES_${target}_CMAKE_SYSTEM_NAME Windows CACHE STRING "")
set(RUNTIMES_${target}_CMAKE_BUILD_TYPE Release CACHE STRING "")
set(RUNTIMES_${target}_COMPILER_RT_BUILD_BUILTINS YES CACHE BOOL "")
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly worried about this as this is the "legacy" path - the builtins build I thought was being replaced with the runtimes build.

Copy link
Contributor Author

@weliveindetail weliveindetail Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding was that this option lets the runtimes build include llvm-project\compiler-rt\lib\builtins which results in a builtins-<triple> build-tree next to the runtimes-<triple> one in the runtimes build directory. I don't see a deprecation warning in CMake, neither swiftlang not upstream https://github.com/llvm/llvm-project/blob/c9fbabfdc92f12b2b0148762e6e789157a172e4d/compiler-rt/lib/CMakeLists.txt#L16 I might be wrong though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, double-checked: I should should switch this off as we get duplicate build artifacts in:

runtimes\runtimes-x86_64-unknown-windows-msvc-bins\compiler-rt\lib\builtins
runtimes\builtins-x86_64-unknown-windows-msvc-bins

utils/build.ps1 Show resolved Hide resolved
@weliveindetail
Copy link
Contributor Author

The Windows tests failed, because the patch implicitly enabled 9 previously unsupported tests in Swift. 3 of them pass, but the others fail:

Failed Tests (6):
  Swift(windows-x86_64) :: Driver/sanitize_coverage.swift
  Swift(windows-x86_64) :: IRGen/address_sanitizer_use_odr_indicator.swift
  Swift(windows-x86_64) :: Interpreter/indirect_enum.swift
  Swift(windows-x86_64) :: Reflection/typeref_decoding_asan.swift
  Swift(windows-x86_64) :: Sanitizers/asan/asan.swift
  Swift(windows-x86_64) :: Sanitizers/sanitizer_coverage.swift

@weliveindetail
Copy link
Contributor Author

I set all failing tests to UNSUPPORTED on Windows. @hjyamauchi Can we give this another try as well please?

IRGen/address_sanitizer_use_odr_indicator.swift appears to have failed with an ODR-related mangling issue. All the others failed because the driver seems to reject the -sanitizer option on Windows:

error: unsupported option '-sanitize=address' for target 'x86_64-unknown-windows-msvc'

@hjyamauchi
Copy link
Contributor

@swift-ci please test Windows platform

@compnerd
Copy link
Member

compnerd commented Dec 4, 2024

@swift-ci please smoke test

@compnerd compnerd enabled auto-merge December 4, 2024 16:18
@hjyamauchi
Copy link
Contributor

The Windows CI seems to have this (unrelated) issue at the moment.

SIL verification failed: Should not have an operand for the opened existential: AMI->getTypeDependentOperands().empty()

@weliveindetail
Copy link
Contributor Author

weliveindetail commented Dec 5, 2024 •
edited
Loading

Will #77972 fix the issue? (It touches the AMI check as well.)

@compnerd
Copy link
Member

compnerd commented Dec 5, 2024

@swift-ci please test Windows platform

@compnerd
Copy link
Member

compnerd commented Dec 5, 2024

@swift-ci please smoke test Linux platform

@hjyamauchi
Copy link
Contributor

@swift-ci please test Windows platform

@weliveindetail weliveindetail mentioned this pull request Dec 5, 2024
Closed
@compnerd compnerd merged commit 3ab2a3e into swiftlang:main Dec 5, 2024
3 checks passed
@hjyamauchi hjyamauchi mentioned this pull request Dec 6, 2024
Closed
hjyamauchi added a commit to hjyamauchi/swift that referenced this pull request Dec 6, 2024
@hjyamauchi
Revert "[windows][toolchain] Enable builtins and sanitizers"
7f3e183 
Revert swiftlang#77770

To fix CI https://ci-external.swift.org/job/swift-main-windows-toolchain-arm64/785/consoleText
@hjyamauchi hjyamauchi mentioned this pull request Dec 6, 2024
Merged
hjyamauchi added a commit to hjyamauchi/swift that referenced this pull request Dec 6, 2024
@hjyamauchi
Revert "[windows][toolchain] Enable builtins and sanitizers"
88fe797 
Revert swiftlang#77770

To fix CI https://ci-external.swift.org/job/swift-main-windows-toolchain-arm64/785/consoleText
hjyamauchi added a commit to hjyamauchi/swift that referenced this pull request Dec 6, 2024
@hjyamauchi
Revert "[windows][toolchain] Enable builtins and sanitizers"
32832dd 
Revert swiftlang#77770

To fix CI https://ci-external.swift.org/job/swift-main-windows-toolchain-arm64/785/consoleText
@bnbarham bnbarham mentioned this pull request Jan 2, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjyamauchi hjyamauchi hjyamauchi left review comments

@tristanlabelle tristanlabelle tristanlabelle left review comments

@compnerd compnerd compnerd left review comments

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@artemcm artemcm Awaiting requested review from artemcm artemcm is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@weliveindetail @hjyamauchi @compnerd @tristanlabelle