Skip to content

Conversation

@zisko
Copy link
Contributor

@zisko zisko commented Oct 7, 2016

This fixes relwithdebuginfo in the swift-3.0-branch

…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
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.
@zisko
Copy link
Contributor Author

zisko commented Oct 7, 2016

@swift-ci please test

@erg
Copy link
Contributor

erg commented Oct 7, 2016

LGTM

@najacque najacque merged commit 8832e5d into swiftlang:swift-3.0-branch Oct 8, 2016
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.

4 participants