[libcxx] [cmake] Strip possibly-inherited compiler flags in in-tree build only

[mgorny]

Summary

Strip the set of flags (including debug defs, -stdlib, -m32) that could be inherited from top-level LLVM build only when in-tree build is performed. This prevents libcxx from confusingly and undesiredly stripping user-supplied flags e.g. when performing packaging system controlled multi-ABI build.

Otherwise, in order to perform 32-bit builds the build scripts would have to use LIBCXX_BUILD_32_BITS. However, -m32 is only one of the many different ABI flags for different targets, and it really makes no sense to add separate CMake options for each possible -m* flag and then keep a mapping from well-known flags to the custom CMake options.

[EricWF]

Stop stripping -m32 from the user-supplied flags. There is no valid reason to do that, stripping it silently is thoroughly confusing and makes it impossible to do distribution multi-ABI builds without resorting to ugly hacks.

The reason for stripping it is configurations like -DLLVM_BUILD_32_BITS=ON -DLIBCXX_BUILD_32_BITS=OFF, where we inherit the incorrect set of flags from the LLVM parent project. I understand your frustration with this behavior but I'm not sure how else to handle that.

Otherwise, in order to perform 32-bit builds the build scripts would have to use LIBCXX_BUILD_32_BITS. However, -m32 is only one of the many different ABI flags for different targets, and it really makes no sense to add separate CMake options for each possible -m* flag and then keep a mapping from well-known flags to the custom CMake options.

I don't see how using -DLLVM_BUILD_32_BITS=ON is impossible compared to using -DCMAKE_CXX_FLAGS=-m32, but the fact it acts differently than other ABI flags is unfortunate.

I'm not sure what the correct thing to do is. Does anybody else want to weigh in?

[mgorny]

In D24809#553415, @EricWF wrote:

Stop stripping -m32 from the user-supplied flags. There is no valid reason to do that, stripping it silently is thoroughly confusing and makes it impossible to do distribution multi-ABI builds without resorting to ugly hacks.

The reason for stripping it is configurations like -DLLVM_BUILD_32_BITS=ON -DLIBCXX_BUILD_32_BITS=OFF, where we inherit the incorrect set of flags from the LLVM parent project. I understand your frustration with this behavior but I'm not sure how else to handle that.

Well, I'm afraid this only proves that the whole concept is broken and shouldn't be there in the first place ;-).

However, if this is the only case, maybe the stripping should be made conditional to in-tree builds?

[mgorny]

Updated the patch to affect only stand-alone builds. Adding @beanz since he seems to have worked on out-of-tree LLVM build support.

[EricWF]

LGTM modulo inline comments.

CMakeLists.txt

312

We always want to strip -stdlib=<foo> because it's just nonsense to enable the STL while building the STL.

[mgorny]

Thanks for the review. Tested and committing now.

CMakeLists.txt

312

Ok, moved it below the endif().

CMakeLists.txt

 #===============================================================================
 # Setup Compiler Flags
 #===============================================================================
 
 include(HandleLibCXXABI) # Setup the ABI library flags
 
 # Remove flags that may have snuck in.
 remove_flags(-DNDEBUG -UNDEBUG -D_DEBUG
-             -stdlib=libc++ -stdlib=libstdc++ -lc++abi -m32)
+             -stdlib=libc++ -stdlib=libstdc++ -lc++abi)
 
 # FIXME(EricWF): See the FIXME on LIBCXX_ENABLE_PEDANTIC.
 # Remove the -pedantic flag and -Wno-pedantic and -pedantic-errors
 # so they don't get transformed into -Wno and -errors respectivly.
 remove_flags(-Wno-pedantic -pedantic-errors -pedantic)
 
 # Required flags ==============================================================
 set(LIBCXX_STANDARD_VER c++11 CACHE INTERNAL "internal option to change build dialect")