This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on Oct 21 2020, 3:17 PM.

Details

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

Diff Detail

Event Timeline

ldionne created this revision.Oct 21 2020, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2020, 3:17 PM
Herald added subscribers: Restricted Project, jkorous, mgorny, dberris. · View Herald Transcript
ldionne requested review of this revision.Oct 21 2020, 3:17 PM

@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 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)'

You're using libstdc++.

ldionne updated this revision to Diff 299837.Oct 21 2020, 5:57 PM

Don't remove -stdlib=

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

You're using libstdc++.

I guess that was the point of -nostdinc++

@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).

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.

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.

@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?

dmajor added a subscriber: dmajor.Oct 22 2020, 7:10 PM

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

phosek accepted this revision.Oct 22 2020, 9:18 PM

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.

This revision is now accepted and ready to land.Oct 22 2020, 9:18 PM

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 updated this revision to Diff 300273.Oct 23 2020, 6:48 AM

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).

ldionne retitled this revision from [compiler-rt] Don't explicitly include the libc++ headers in MSAN to [compiler-rt] Don't include libc++ headers from the source tree in MSAN.Oct 23 2020, 6:49 AM
ldionne edited the summary of this revision. (Show Details)
ldionne added a reviewer: eugenis.

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.

use custom libc++

vitalybuka accepted this revision.Oct 24 2020, 1:05 AM

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

vitalybuka added a comment.EditedOct 24 2020, 1:43 AM

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 accepted this revision.Oct 28 2020, 12:33 PM

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 added inline comments.Oct 28 2020, 3:30 PM
compiler-rt/lib/msan/tests/CMakeLists.txt
69–70

Could we add this to MSAN_UNITTEST_COMMON_CFLAGS?

phosek added inline comments.Oct 30 2020, 1:14 AM
compiler-rt/lib/msan/tests/CMakeLists.txt
69–70

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