This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Passthrough additional flags to custom libcxx CMake build
ClosedPublic

Authored by phosek on Jun 6 2018, 10:17 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Jun 6 2018, 10:17 AM
Herald added subscribers: Restricted Project, llvm-commits, mgorny. · View Herald Transcript
morehouse added inline comments.Jun 6 2018, 10:54 AM
compiler-rt/cmake/Modules/AddCompilerRT.cmake
522 ↗(On Diff #150159)

What is this code block doing?

phosek updated this revision to Diff 150176.Jun 6 2018, 12:16 PM
phosek marked an inline comment as done.
morehouse added inline comments.Jun 6 2018, 12:39 PM
compiler-rt/cmake/Modules/AddCompilerRT.cmake
513 ↗(On Diff #150176)

Why is this replacement necessary?

phosek marked an inline comment as done.Jun 6 2018, 1:06 PM
phosek added inline comments.
compiler-rt/cmake/Modules/AddCompilerRT.cmake
513 ↗(On Diff #150176)

This is an equivalent of the foreach originally on line 477 but done as a one-liner. The reason is that flags passed via an argument will be a ; separated CMake list, so we need to convert it to a string where individual flags are separated by spaces.

522 ↗(On Diff #150159)

I was planning on using variables like CLANG_DEFAULT_RTLIB to infer whether to use compiler-rt builtins by default in libc++ like we do in compiler-rt now hence passing these variables through, but that functionality isn't implemented yet so it's probably easier to just punt on it for now and do it later in a separate patch.

morehouse accepted this revision.Jun 6 2018, 2:14 PM
morehouse added inline comments.
compiler-rt/cmake/Modules/AddCompilerRT.cmake
516 ↗(On Diff #150176)

Does it make sense to move this whole section closer to the ExternalProject_Add below?

This revision is now accepted and ready to land.Jun 6 2018, 2:14 PM
phosek updated this revision to Diff 150203.Jun 6 2018, 2:45 PM
phosek marked 3 inline comments as done.
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Jun 12 2018, 12:51 PM

Thanks for the heads up and sorry about the breakage, I'll see if I can reproduce this locally and come up with a fix.

phosek updated this revision to Diff 151082.Jun 12 2018, 6:39 PM

Turned out that passing all CFLAGS, CXXFLAGS and LDFLAGS throught to libc++ isn't really going to work because some of those such as -Werror -Wall might break some CMake checks. Instead we should pass flags to libc++ more selectively.

@morehouse is it still LGTM? Can I try to reland this?

This revision was automatically updated to reflect the committed changes.