Fix no asserts build #5137
Merged
Fix no asserts build #5137
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch resolves some post-commit feedback from @llvmbeanz. Specifically, the only time that we need to hard code these flags are when compiling with RelWithDebInfo since we need extra control in that case to ensure that CMake does not sneak in a -g flag on the command line. This is important to ensure that we are compiling with -gline-tables-only instead of -g. Given that it is a performance fix specific to compiling with LTO with debug info, we should no perform such a change when compiling with: 1. MINRELSIZE since we do not ever perform that build. 2. RELEASE since it does not have debug info. 3. DEBUG since why would one perform an LTO build in DEBUG mode. In terms of 3, I can potentially imagine someone trying to compile Swift in Debug with LTO to try to debug an LTO bug, but I imagine such a test case to be such a rare need that there is no point in trying to make it faster.
…S_RELWITHDEBINFO for swift and llvm. Turns out -DLLVM_ENABLE_ASSERTIONS does not actually enable assertions in LLVM! What it actually does is rewrite the -DNDEBUG flag to be -UNDEBUG. This means that we always want to pass in -DNDEBUG to swift. There are some complications though. This means that we can not compile the C++ part of the runtime with a different assertions setting than the swift compiler/llvm. This is a trade off that for now we are ok with to get the no-asserts build working. As part of a separate change, we are going to make -DLLVM_ENABLE_ASSERTIONS more robust by doing what swift does now, putting -DNDEBUG or -UNDEBUG after CMake's -DNDEBUG so we get the correct behavior of disabling or enabling assertions. rdar://28485525
|
@swift-ci Please clean test |
|
I am also going to try a benchmarking test since that test is broken without this change. |
|
@swift-ci Please benchmark |
Build comment file:Build failed before running benchmark. |
|
Benchmark failed due to |
|
Hm. That is literally what this change is supposed to fix. |
|
I do not think the benchmark test did a clean build. @shahmishal Can you cause it to always do a clean build? |
|
Or maybe it is using the wrong commits. This is the build-script line:
When I look in the tree with my sources, I am literally seeing no -DCMAKE_C_FLAGS_RELEASE. I do not know how that line is getting there. |
|
@shahmishal ping |
|
@gottesmm Benchmark job builds master branch first and compares the results with the PR branch. We are seeing this failure because master does not have this change yet, so I would recommend we merge this fix and test it on https://ci.swift.org/job/oss-swift_tools-R_stdlib-RD_test-simulator/ |
|
SGTM! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This group of commits does two things:
I am going to file a radar for a separate piece of work to fix LLVM to do something sane, namely, adding -DNDEBUG or -UNDEBUG to the end of command lines and perhaps create some sort of verification on the swift side that we are getting asserts where we think they are supposed to be.