[compiler-rt] Don't include libc++ headers from the source tree in MSAN

[ldionne]

Summary

We shouldn't be including the libc++ headers from the source tree directly, since those headers are not configured (i.e. they don't use the __config_site) header like they should, which could mean up to ABI differences

[ldionne]

@vitalybuka This is an attempt to fix the issue you were seeing in https://reviews.llvm.org/D89041. Can you please confirm it works? If it does, feel free to commit it on my behalf, otherwise, I'll do it first time tomorrow morning (it's getting late here).

[vitalybuka]

In D89915#2345796, @ldionne wrote:

@vitalybuka This is an attempt to fix the issue you were seeing in https://reviews.llvm.org/D89041. Can you please confirm it works? If it does, feel free to commit it on my behalf, otherwise, I'll do it first time tomorrow morning (it's getting late here).

it compiles but check-msan does not link unittests:

MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `std::vector<int, std::allocator<int> >::_M_check_len(unsigned long, char const*) const':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_vector.h:1756: undefined reference to `std::__throw_length_error(char const*)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `std::vector<int, std::allocator<int> >::_S_check_init_len(unsigned long, std::allocator<int> const&)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_vector.h:1767: undefined reference to `std::__throw_length_error(char const*)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `std::ostream::operator<<(unsigned long)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ostream:171: undefined reference to `std::ostream& std::ostream::_M_insert<unsigned long>(unsigned long)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `bool std::operator==<char, std::char_traits<char>, std::allocator<char> >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char const*)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.h:6177: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::compare(char const*) const'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.tcc:219: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
/usr/bin/ld: /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.tcc:219: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `std::vector<testing::internal::ParameterizedTestCaseInfoBase*, std::allocator<testing::internal::ParameterizedTestCaseInfoBase*> >::_M_check_len(unsigned long, char const*) const':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_vector.h:1756: undefined reference to `std::__throw_length_error(char const*)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.tcc:219: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.tcc:219: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.tcc:219: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.tcc:219: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
/usr/bin/ld: MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o: in function `void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag)':
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.tcc:219: undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'

[ldionne]

You're using libstdc++.

[ldionne]

Don't remove -stdlib=

[ldionne]

@Hahnfeld Can you advise on the proper way to fix this? From git blame, it seems like you might know.

[vitalybuka]

In D89915#2346067, @ldionne wrote:

You're using libstdc++.

I guess that was the point of -nostdinc++

[Hahnfeld]

In D89915#2346071, @ldionne wrote:

@Hahnfeld Can you advise on the proper way to fix this? From git blame, it seems like you might know.

So it's been a while, but I can try to write up what I remember about the situation: For using (and in this case testing) the sanitizers with C++ code, we need custom variants of libc++ where the shared library is instrumented. They're compiled via add_custom_libcxx (see lib/tsan/CMakeLists.txt and lib/msan/tests/CMakeLists.txt). As to why this is done at different levels, I can't give a definitive answer, but maybe it's because test/tsan/ also needs libcxx_tsan?

Anyway, the fragile part about this setup is that there are multiple competing C++ libraries, which is always a source of trouble:

1. There's the headers from the instrumented build, which I'd argue are those that we should use (because its config matches that of the shared object).
2. If you're enabling libcxx as part of the build (via LLVM_ENABLE_PROJECTS), there's a second set of headers and shared library, which is what users would link to. That is hopefully the same version of libcxx and maybe has a matching configuration, I don't know. Maybe we're lucky enough to not run into issues with this, even with LLVM_ENABLE_LIBCXX...
3. And then there's a third possibility which is what I've been maintaining and worrying about in the past: If you build LLVM with an older version of clang, chances are it also has its own libcxx and matters get worse if clang was configured with CLANG_DEFAULT_CXX_STDLIB = libc++. By definition, this will try to pull in a different version and lead to all kinds of problems.

This seems to be confirmed by commit rG6870dc7311b769f400fdcc2a041aaf5cdc041dfc which added the -nostdinc++ in the first place, but I can't tell which options from above led to the conflict (probably not 3 because some of the options didn't exist back then?). My change for removing -stdlib= was just to silence warnings during build, it shouldn't have an influence on the built result.

Now with all the things in mind that could go wrong, I can't really advise on a proper way to fix this. It could be that -nostdinc++ isn't needed anymore and it'll just work (for most configurations), but I wouldn't bet on it. I find it very likely that some configuration will still break, and maybe some already is because I haven't been testing it anymore lately (in particular number 3 from above's list).

[Hahnfeld]

By the way, there's no commit list subscribed which sounds bad. Adding llvm-commits to give people a chance to be aware of this.

[ldionne]

In D89915#2346251, @vitalybuka wrote:

In D89915#2346067, @ldionne wrote:

You're using libstdc++.

I guess that was the point of -nostdinc++

No, I think the point of -nostdinc++ was specifically to use libc++, since it was combined with -isystem <path-to-libcxx>/include. Note that this always has been incorrect, cause you wouldn't get the __config_site header of libc++, which means you could have configuration differences betwen the libc++ you're actually using and the one you think you're using. Some of these have serious impacts (e.g. on ABI). This started being noticeable with my commit because I basically made the headers in libcxx/include not usable as-is -- you need to go through an installation step for them to work. That's an unintended consequence of my change, however it is a good one, since it caught misuses like this.

In D89915#2346513, @Hahnfeld wrote:

In D89915#2346071, @ldionne wrote:

@Hahnfeld Can you advise on the proper way to fix this? From git blame, it seems like you might know.

So it's been a while, but I can try to write up what I remember about the situation: For using (and in this case testing) the sanitizers with C++ code, we need custom variants of libc++ where the shared library is instrumented. They're compiled via add_custom_libcxx (see lib/tsan/CMakeLists.txt and lib/msan/tests/CMakeLists.txt). As to why this is done at different levels, I can't give a definitive answer, but maybe it's because test/tsan/ also needs libcxx_tsan?

Ok, thanks for the context. However, whatever they were doing here, they were not trying to use the custom libc++ they built (or if they were, they were doing that wrong), because they were using the includes directly from libcxx/include, not the ones that should be installed in add_custom_libcxx. Using the headers installed by that target would be the correct way of solving this problem if I understand the intent correctly.

I'm not really sure where to go from there. Who owns this code? @eugenis do you have any insights?

[smeenai]

Subscribing @phosek because he's been looking into some sanitizer build issues with the C++ headers recently as well (D88922).

[phosek]

I already talked to @ldionne over chat. D88922 is a larger cleanup and I'm still trying to get it to a working state, but I think we should land this change in the meantime to address the current failures.

[ldionne]

I've reverted the other changes, please see D89041 for details. We still need to figure out what's the right thing to do here though, cause we're including libc++ headers from the source tree which are mis-configured, and that's not what we want.

[ldionne]

Reword the review in the context of improving the build, instead of fixing the build bots (which should be OK now since I've reverted the __config_site change).

[vitalybuka]

As is it does not work for

cmake -GNinja llvm-project/llvm '-DLLVM_ENABLE_PROJECTS=clang;clang-tools-extra;compiler-rt;lld;libcxx;libcxxabi' -DLLVM_ENABLE_WERROR=OFF -DLLVM_ENABLE_LLD=ON -DCOMPILER_RT_BUILD_LIBFUZZER=ON -DCMAKE_C_COMPILER=...clang/bin/clang -DCMAKE_CXX_COMPILER=...clang/bin/clang++ -DLLVM_BINUTILS_INCDIR=/usr/include -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=OFF

ninja check-msan

If this was incorrect, I suppose we just need to replace "-isystem ${COMPILER_RT_LIBCXX_PATH}/include" with custom build libc++ correctly.

[vitalybuka]

use custom libc++

[vitalybuka]

@ldionne This works for me, does this makes sense to you?

[vitalybuka]

this fixes "#include <__config_site>" errors from your previous patches, but something else needs to be done to address new error from 529ac33197f6408952ae995075ac5e2dc5287e81"Please provide the path to where the libc++ headers have been installed."

[ldionne]

This makes sense to me. Since 529ac33197f was reverted, I reckon the patch works as-is? If so, please feel free to go ahead and commit!

[phosek]

compiler-rt/lib/msan/tests/CMakeLists.txt

69–70

Could we add this to MSAN_UNITTEST_COMMON_CFLAGS?

[phosek]

compiler-rt/lib/msan/tests/CMakeLists.txt

69–70

Never mind, I see that the headers are per-arch now.

compiler-rt/lib/msan/tests/CMakeLists.txt

set(MSAN_LOADABLE_SOURCE
   msan_loadable.cpp
   )
 set(MSAN_UNITTEST_HEADERS
   msan_test_config.h
   ../../../include/sanitizer/msan_interface.h
   )
 set(MSAN_UNITTEST_COMMON_CFLAGS
   -nostdinc++
-  -isystem ${COMPILER_RT_LIBCXX_PATH}/include
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}
   -I${COMPILER_RT_SOURCE_DIR}/include
   -I${COMPILER_RT_SOURCE_DIR}/lib
   -I${COMPILER_RT_SOURCE_DIR}/lib/msan
   -g
   -O2
   -fno-omit-frame-pointer

 append_list_if(COMPILER_RT_HAS_LIBDL -ldl MSAN_UNITTEST_LINK_FLAGS)
 
 macro(msan_compile obj_list source arch kind cflags)
   sanitizer_test_compile(
     ${obj_list} ${source} ${arch}
     KIND ${kind}
     COMPILE_DEPS ${MSAN_UNITTEST_HEADERS}
     DEPS gtest msan
-    CFLAGS ${MSAN_UNITTEST_INSTRUMENTED_CFLAGS} ${cflags}
+    CFLAGS -isystem ${CMAKE_CURRENT_BINARY_DIR}/../libcxx_msan_${arch}/include/c++/v1
+           ${MSAN_UNITTEST_INSTRUMENTED_CFLAGS} ${cflags}
   )
 endmacro()
 
 macro(msan_link_shared so_list so_name arch kind)
   cmake_parse_arguments(SOURCE "" "" "OBJECTS;LINK_FLAGS;DEPS" ${ARGN})
   set(output_so "${CMAKE_CURRENT_BINARY_DIR}/${so_name}.${arch}${kind}.so")
   get_target_flags_for_arch(${arch} TARGET_LINK_FLAGS)
   if(NOT COMPILER_RT_STANDALONE_BUILD)