Page MenuHomePhabricator

[CMake] Use -isystem flag to access libc++ headers
ClosedPublic

Authored by phosek on Sep 28 2020, 4:17 PM.

Details

Reviewers
beanz
ldionne
Group Reviewers
Restricted Project
Commits
rG8d26760a95ba: [CMake] Use -isystem flag to access libc++ headers
Summary

This is a partial revert of D62155. Rather than copying libc++ headers
into the build directory to be later overwritten by the final headers,
use -isystem flag to access libc++ headers during CMake checks. This
should address the occasional flake we've seen, especially on Windows
builders where CMake fails to overwrite __config with the final version.

Diff Detail

Event Timeline

phosek created this revision.Sep 28 2020, 4:17 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 28 2020, 4:17 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek requested review of this revision.Sep 28 2020, 4:17 PM
phosek updated this revision to Diff 294827.Sep 28 2020, 4:29 PM

I was abusing this functionality to be able to get the C++ headers installed without having to build all the runtimes build prerequisites; we have a configuration where we only need the C++ headers installed (and not the library), and using the runtime-libcxx-headers was an easy way to get them without needing to first build Clang and all the other tools the runtimes build requires. I should be able to come up with something else for that use case though.

@beanz I've tested this on macOS and it seems to be working fine.

ldionne accepted this revision.Sep 30 2020, 7:44 AM

I think that's a lot cleaner. Thanks!

This revision is now accepted and ready to land.Sep 30 2020, 7:44 AM

@smeenai do you already have an alternative for your use case?

@smeenai do you already have an alternative for your use case?

Yup. Thanks for checking!

This revision was automatically updated to reflect the committed changes.
uabelho added a subscriber: uabelho.Oct 1 2020, 9:29 PM

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:45:7: error: '__sanitizer::FlagHandler<bool>' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
class FlagHandler : public FlagHandlerBase {
      ^
/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:71:13: note: in instantiation of template class '__sanitizer::FlagHandler<bool>' requested here
inline bool FlagHandler<bool>::Parse(const char *value) {
            ^
phosek added a comment.Oct 2 2020, 1:50 AM

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:45:7: error: '__sanitizer::FlagHandler<bool>' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
class FlagHandler : public FlagHandlerBase {
      ^
/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:71:13: note: in instantiation of template class '__sanitizer::FlagHandler<bool>' requested here
inline bool FlagHandler<bool>::Parse(const char *value) {
            ^

That's a bit surprising. What's your build configuration? Do you use runtimes build? I haven't managed to reproduce it locally so far.

bjope added a subscriber: bjope.Oct 2 2020, 4:52 AM

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:45:7: error: '__sanitizer::FlagHandler<bool>' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
class FlagHandler : public FlagHandlerBase {
      ^
/data/repo/master/compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:71:13: note: in instantiation of template class '__sanitizer::FlagHandler<bool>' requested here
inline bool FlagHandler<bool>::Parse(const char *value) {
            ^

That's a bit surprising. What's your build configuration? Do you use runtimes build? I haven't managed to reproduce it locally so far.

Sorry if there are some local paths in there, but this is our cmake command (answering for @uabelho ):

CC='/proj/flexasic/app/llvm/8.0/bin/clang -march=corei7' CXX='/proj/flexasic/app/llvm/8.0/bin/clang++ -march=corei7' LDFLAGS='-L/proj/flexasic/app/llvm/8.0/lib64 -Wl,-R/proj/flexasic/app/llvm/8.0/lib64:/proj/bbi_twh/wh_bbi/x86_64-Linux3/z3/4.8.8-1/lib64' PATH=/proj/flexasic/app/ninja/1.8.2/SLED11-64/bin:$PATH  /app/vbuild/RHEL6-x86_64/cmake/3.16.4/bin/cmake /repo/uabbpet/llvm-upstream/llvm --debug-trycompile -G Ninja -DCMAKE_MAKE_PROGRAM=/proj/flexasic/app/ninja/1.8.2/SLED11-64/bin/ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_INSTALL_PREFIX=/compiler-clang -DLLVM_APPEND_VC_REV=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_WERROR=ON -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra' -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON -DLLVM_BUILTIN_TARGETS=default -DLLVM_ENABLE_RUNTIMES=all -DRUNTIMES_x86_64-unknown-linux-gnu_SANITIZER_USE_STATIC_LLVM_UNWINDER=ON -DRUNTIMES_x86_64-unknown-linux-gnu_SANITIZER_USE_STATIC_CXX_ABI=ON -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_USE_BUILTINS_LIBRARY=ON -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_CAN_EXECUTE_TESTS=OFF -DRUNTIMES_x86_64-unknown-linux-gnu_LIBCXX_HAS_ATOMIC_LIB=0 -DLLVM_RUNTIME_TARGETS='x86_64-unknown-linux-gnu' -DRUNTIMES_x86_64-unknown-linux-gnu_COMPILER_RT_BUILD_XRAY=OFF -DLLVM_ENABLE_LIBPFM=OFF

When it gets to the runtime build I start seeing some diffs in the config output, e.g. these used to be sucess, now we get "not found" and "Failed":

...
-- Looking for fopen in c - not found
-- Found compiler-rt builtins library: /repo/uabbpet/llvm-upstream/llvm/build-all-builtins/lib/clang/12.0.0/lib/x86_64-unknown-linux-gnu/libclang_rt.builtins.a
-- Performing Test COMPILER_RT_HAS_NODEFAULTLIBS_FLAG
-- Performing Test COMPILER_RT_HAS_NODEFAULTLIBS_FLAG - Success
-- Performing Test COMPILER_RT_HAS_FFREESTANDING_FLAG
-- Performing Test COMPILER_RT_HAS_FFREESTANDING_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_STD_C11_FLAG
-- Performing Test COMPILER_RT_HAS_STD_C11_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FPIC_FLAG
-- Performing Test COMPILER_RT_HAS_FPIC_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FPIE_FLAG
-- Performing Test COMPILER_RT_HAS_FPIE_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_BUILTIN_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_BUILTIN_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_EXCEPTIONS_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_EXCEPTIONS_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FOMIT_FRAME_POINTER_FLAG
-- Performing Test COMPILER_RT_HAS_FOMIT_FRAME_POINTER_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FUNWIND_TABLES_FLAG
-- Performing Test COMPILER_RT_HAS_FUNWIND_TABLES_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_STACK_PROTECTOR_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_STACK_PROTECTOR_FLAG - Failed
-- Performing Test COMPILER_RT_HAS_FNO_SANITIZE_SAFE_STACK_FLAG
-- Performing Test COMPILER_RT_HAS_FNO_SANITIZE_SAFE_STACK_FLAG - Failed
...

Maybe that is expected and unrelated.
Also, the warnings we get now seem correct afaict (those structs have virtual functions but non-virtual dtor), even though it feels strange that they popped up when mergine this patch.

bjope added a comment.Oct 2 2020, 12:45 PM

Looking at build.ninja we used to get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -std=c++14 -Wno-unused-parameter -O3     -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -fno-rtti -Wframe-larger-than=570 -Wglobal-constructors -UNDEBUG

but now we get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3     -O3 -UNDEBUG

I haven't tracked down why we for example no longer get -Wno-non-virtual-dtor when building the sanitizers. But it looks like before this patch those warnings were suppressed, but not with this patch.

phosek added a comment.Oct 2 2020, 2:21 PM

Looking at build.ninja we used to get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -std=c++14 -Wno-unused-parameter -O3     -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -fno-rtti -Wframe-larger-than=570 -Wglobal-constructors -UNDEBUG

but now we get

FLAGS = -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3     -O3 -UNDEBUG

I haven't tracked down why we for example no longer get -Wno-non-virtual-dtor when building the sanitizers. But it looks like before this patch those warnings were suppressed, but not with this patch.

I think I figured it out, D88756 should address the issue.

bjope added a comment.Oct 2 2020, 3:16 PM

I think I figured it out, D88756 should address the issue.

Thanks! I tried it, and that seems to resolve the problem for us!