Page MenuHomePhabricator

[CMake] Use a different source depending on C++ support
ClosedPublic

Authored by phosek on May 21 2018, 4:40 PM.

Details

Summary

When using system C++ library, assume we have a working C++ compiler and
try to compile a complete C++ program. When using in tree C++ library,
only check the C compiler since the C++ library likely won't have been
built yet at time of running the check.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.May 21 2018, 4:40 PM
Herald added subscribers: Restricted Project, llvm-commits, mgorny. · View Herald TranscriptMay 21 2018, 4:40 PM
vitalybuka accepted this revision.May 21 2018, 5:23 PM
This revision is now accepted and ready to land.May 21 2018, 5:23 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

This breaks the build if you are not building a 32bit C++ library, that's why I introduced a check for a full C++ program in D23654. I therefore request this change to be reverted, we need to come up with a better solution that correctly works for in-tree builds.

This breaks the build if you are not building a 32bit C++ library, that's why I introduced a check for a full C++ program in D23654. I therefore request this change to be reverted, we need to come up with a better solution that correctly works for in-tree builds.

Ping, this change is not correct!

Due to the lack of replies, I've reverted this change in rL334903 to unbreak my build. Feel free to submit a patch that doesn't regress the build system, thanks.

Due to the lack of replies, I've reverted this change in rL334903 to unbreak my build. Feel free to submit a patch that doesn't regress the build system, thanks.

I never got any email, sorry about the lack of the response, I just noticed this because your revert broke our build. What is your build configuration where you're seeing the failure? I'd like to replicate the issue and try to come up with a better solution because now this is blocking us.

I never got any email, sorry about the lack of the response, I just noticed this because your revert broke our build.

The messages seem to be on llvm-commits (http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180528/557038.html and http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180611/thread.html#559742), but in any case you should have received a direct notification from Phabricator. Maybe you might want to check your filters?

What is your build configuration where you're seeing the failure? I'd like to replicate the issue and try to come up with a better solution because now this is blocking us.

I'm on x86_64 but don't have a 32bit version of the C++ libraries (we are bootstrapping Clang with libc++ and -DCLANG_DEFAULT_CXX_STDLIB=libc++). I think this configuration will become more important soon as distributions try to shave support for 32bit libraries and binaries.

The only "correct" solution that I can currently think of is fixing the dependency in runtimes/ (which I assume you are using for Fuchsia):

  1. Configure and compile builtins
  2. Configure and compile libunwind, libc++abi, and libc++
  3. Configure all other runtime libraries, including the sanitizers.

The other approach would be to kill the dependency of the sanitizers to the C++ library - I thought somebody wanted to tackle this in the past, but I'd need to look through the mailing lists.

The messages seem to be on llvm-commits (http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180528/557038.html and http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180611/thread.html#559742), but in any case you should have received a direct notification from Phabricator. Maybe you might want to check your filters?

Seems like a Gmail issue, I've already archived the original thread after submitting the change and Gmail didn't move it back to Inbox after receiving your response which is why I missed it.

I'm on x86_64 but don't have a 32bit version of the C++ libraries (we are bootstrapping Clang with libc++ and -DCLANG_DEFAULT_CXX_STDLIB=libc++). I think this configuration will become more important soon as distributions try to shave support for 32bit libraries and binaries.

We use -DCLANG_DEFAULT_CXX_STDLIB=libc++ in Fuchsia but we also use -DCLANG_DEFAULT_RTLIB=compiler-rt, removing the latter option replicates the issue.

The only "correct" solution that I can currently think of is fixing the dependency in runtimes/ (which I assume you are using for Fuchsia):

  1. Configure and compile builtins
  2. Configure and compile libunwind, libc++abi, and libc++
  3. Configure all other runtime libraries, including the sanitizers.

I came to the same the same conclusion, but I'm not particularly excited about that idea primarily because it'll mean having another CMake invocation making the build even slower.

The other approach would be to kill the dependency of the sanitizers to the C++ library - I thought somebody wanted to tackle this in the past, but I'd need to look through the mailing lists.

I think that's unlikely at this point, libFuzzer and XRay both depend on C++ library and someone would need to rewrite them. Furthermore, many other sanitizers depend on C++ ABI.

There's another solution that's simpler than the two mentioned above which would be to simply avoid using the compiler-rt logic for determining the set of supported targets and instead rely on CMake which is already what runtimes/ build does except for the host (it's the COMPILER_RT_DEFAULT_TARGET_ONLY option). I know which targets I want to build runtimes for, I don't want CMake to go on and try guessing which targets my compiler may or may not support. This needs D45604 but that change is ready to land so I might try and go with that approach.

There's another solution that's simpler than the two mentioned above which would be to simply avoid using the compiler-rt logic for determining the set of supported targets and instead rely on CMake which is already what runtimes/ build does except for the host (it's the COMPILER_RT_DEFAULT_TARGET_ONLY option). I know which targets I want to build runtimes for, I don't want CMake to go on and try guessing which targets my compiler may or may not support. This needs D45604 but that change is ready to land so I might try and go with that approach.

I think that should work: Overriding the detection if runtimes/ is known to produce the required dependencies. I don't see how this relies on D45604, but I might be missing something here.