This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mgorny on Sep 21 2016, 9:51 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 72077.Sep 21 2016, 9:51 AM
mgorny retitled this revision from to [libcxx] [cmake] Stop stripping -m32 from compiler flags.
mgorny updated this object.
mgorny added a reviewer: EricWF.
mgorny added a subscriber: cfe-commits.
EricWF edited edge metadata.Sep 26 2016, 7:06 PM

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?

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 this revision to Diff 72602.Sep 26 2016, 11:05 PM
mgorny retitled this revision from [libcxx] [cmake] Stop stripping -m32 from compiler flags to [libcxx] [cmake] Strip possibly-inherited compiler flags in in-tree build only.
mgorny updated this object.
mgorny added a reviewer: beanz.

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 accepted this revision.Sep 26 2016, 11:59 PM
EricWF edited edge metadata.

LGTM modulo inline comments.

CMakeLists.txt
310 ↗(On Diff #72602)

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

This revision is now accepted and ready to land.Sep 26 2016, 11:59 PM
mgorny marked an inline comment as done.Sep 27 2016, 1:03 AM

Thanks for the review. Tested and committing now.

CMakeLists.txt
310 ↗(On Diff #72602)

Ok, moved it below the endif().

This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.