Skip to content
This repository has been archived by the owner on Mar 28, 2020. It is now read-only.
/ swift-llvm Public archive

Disable LLVM ABI breaking checks by default when building swift #45

Open
wants to merge 1 commit into
base: swift-4.0-branch
Choose a base branch
from

Conversation

llvm-beanz
Copy link
Contributor

The LLVM ABI breaking checks require that all uses of llvm's ADT library must also link libSupport. Parts of the Swift standard library use ADT without Support intentionally. To make this work we must disable the ABI breaking checks when building swift.

This patch changes the default value of the option that controls the ABI breaking checks to disable the checks by default if the swift source directory is present in the LLVM build tree.

Currently disabling these checks is handled by the swift build-script, but to support non-build-script builds having the default work is ideal.

The LLVM ABI breaking checks require that all uses of llvm's ADT library must also link libSupport. Parts of the Swift standard library use ADT without Support intentionally. To make this work we must disable the ABI breaking checks when building swift.

This patch changes the default value of the option that controls the ABI breaking checks to disable the checks by default if the swift source directory is present in the LLVM build tree.
@llvm-beanz
Copy link
Contributor Author

@swift-ci please test

@jrose-apple
Copy link
Contributor

This should be going to master, not swift-4.0-branch.

@llvm-beanz
Copy link
Contributor Author

@jrose-apple it is an LLVM change. There is no master. My thought was to put it in swift-4.0 (which merges to stable), then upstream-with-swift. Is there a reason to leave it out of 4.0?

@jrose-apple
Copy link
Contributor

Oops, sorry, I thought it was a Swift change. Usually we would put something in upstream-with-swift first and then cherry-pick down, just like Clang changes usually go into trunk before they're cherry-picked.

However, why turn this off by default? Why not just turn it off for swiftRuntime?

@llvm-beanz
Copy link
Contributor Author

The setting gets baked into an LLVM configuration-driven header that is included by the ADT headers. It has to be off in the configuration of LLVM in order for it to be off, you can't just override it for a subdirectory. We currently disable it in build-script-impl when building LLVM.

@jrose-apple
Copy link
Contributor

:-/ All right. That doesn't seem like a good long-term answer to me, but I'm not going to put myself into the discussion for now.

@llvm-beanz
Copy link
Contributor Author

@swift-ci please smoke test

@llvm-beanz
Copy link
Contributor Author

@swift-ci please test

@joker-eph
Copy link
Contributor

Do you need to change LLVM? @bob-wilson had added a way for client to include ADT without getting check inject, why isn't it enough on the swift side?

@llvm-beanz
Copy link
Contributor Author

As far as I can tell, Swift disables the checks when building LLVM, and that is how it gets around the problem.

All this patch does is default the value to off if you are building Swift, so that running CMake with no options produces build files that will actually build successfully.

@joker-eph
Copy link
Contributor

The files in Swift that include ADT needs to have this defined on the command line: -DLLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING, without any need to change the way LLVM is configured at all. That the most recent plan I've been aware of, but @bob-wilson should confirm the direction here.

@llvm-beanz
Copy link
Contributor Author

@joker-eph The Swift workaround @bob-wilson made is here: https://github.com/apple/swift/blob/6c1eec81c44400fd4cefb5d7df32b861a3e6a066/utils/build-script-impl#L728

From this patch:
swiftlang/swift@4081418

This patch allows you to put Swift under LLVM/tools, and run "cmake && ninja" and get a swift compiler and standard library.

Without this patch the standard library build will fail.

@joker-eph
Copy link
Contributor

To clarify, when I wrote -DLLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING needs to be set on the command line, I didn't mean the cmake invocation, but the clang++ invocation that will compile the swift source code.

@joker-eph
Copy link
Contributor

The LLVM change to make this possible was: https://reviews.llvm.org/D29919 "allow migrating away from cmake option for LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING"

@llvm-beanz
Copy link
Contributor Author

Ah. That patch is newer than this PR. This PR has been around for over a month, but PR testing was broken when I first tried to land it.

@bob-wilson
Copy link
Contributor

I already put my LLVM change r295090 into swift-4.0-branch (22f170f). I have been holding the Swift side of the change for a few weeks to minimize the impact on Swift developers, giving more time for people to rebuild their copy of LLVM (swiftlang/swift#7469). I suppose it has been long enough. I'll plan to do that tomorrow.

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

Successfully merging this pull request may close these issues.

4 participants