On my machine this showed no new failures in check-llvm and increased
the testing time from 138 to 142 seconds, for a Release build with
assertions enabled.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
See LLVM_ENABLE_EXPENSIVE_CHECKS below that already adds this flag.
Debug builds are already slow, and making them even slower doesn't sound good to me.
I think this patch needs broader attention.
Comment Actions
I think this is pretty reasonable. In line with assertions we would get in our own ADT library -- it's odd that these get lost when switching to STL equivalent.
As @barannikov88 pointed out, we should just be moving the existing block from EXPENSIVE_CHECKS though.
Note that there are a number of failures in pre-merge checks -- it looks like we have assertion failures, just not in any of the core projects, but rather flang and bolt. The diagnostics quality isn't exactly great though (apparently these just abort without a message?)
Comment Actions
Ah no, having looked a bit closer, I think we want to have -D_GLIBCXX_ASSERTIONS in normal assertion enabled builds and -DGLIBCXX_DEBUG only under EXPENSIVE_CHECKS, so I think your implementation is about right as-is.
Comment Actions
We already have this define under expensive checks. There are bots that define this macro, and the build stays clean (mostly, as pre-merge checks have shown).
We have been using STL for many years without the additional assertions, and I don't see why we should change it now. Those few failures is not enough motivation to slow down debug builds even further.
I'd be convinced though if someone could measure check-llvm-codegen time with and without this macro and show that the difference is negligible.
Comment Actions
The pre-merge checks show that this is not the case. And I do know that these assertion failures also end up in releases, because Fedora at least builds everything with -D_GLIBCXX_ASSERTIONS enabled by default, and I've seen these kinds of failures before.
We have been using STL for many years without the additional assertions, and I don't see why we should change it now. Those few failures is not enough motivation to slow down debug builds even further.
I'd be convinced though if someone could measure check-llvm-codegen time with and without this macro and show that the difference is negligible.
The summary of this patch suggests that it is indeed negligible:
On my machine this showed no new failures in check-llvm and increased
the testing time from 138 to 142 seconds, for a Release build with
assertions enabled.
Comment Actions
And I've just finished testing a debug build (check-llvm-codegen only).
It is 1000s vs 1030s on my machine. Looks acceptable :)
| llvm/cmake/modules/HandleLLVMOptions.cmake | ||
|---|---|---|
| 85 | Kind of nitpick. | |
| llvm/cmake/modules/HandleLLVMOptions.cmake | ||
|---|---|---|
| 85 | I'm using cmake 3.23.2 and this does not work for me. It adds -D1:_GLIBCXX_ASSERTIONS to the compiler command line which fails with: In file included from <built-in>:413: <command line>:1:9: error: macro name must be an identifier #define 1:_GLIBCXX_ASSERTIONS 1 | |
| llvm/cmake/modules/HandleLLVMOptions.cmake | ||
|---|---|---|
| 85 | I should have checked first, sorry. The correct spelling is: | |
Comment Actions
Note that there are a number of failures in pre-merge checks -- it looks like we have assertion failures, just not in any of the core projects, but rather flang and bolt.
@maksfb @rafauler could you please help look at the BOLT failures? I can reproduce them on my own machine:
Failed Tests (2): BOLT :: runtime/X86/instrumentation-dup-jts.s BOLT :: runtime/meta-merge-fdata.test
They both appear to have the same stack trace:
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_vector.h:1123: std::vector::reference std::vector<llvm::MCSymbol *>::operator[](std::vector::size_type) [_Tp = llvm::MCSymbol *, _Alloc = std::allocator<llvm::MCSymbol *>]: Assertion '__n < this->size()' failed. #0 0x0000000004ba6a87 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x4ba6a87) #1 0x0000000004ba4a8c llvm::sys::RunSignalHandlers() (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x4ba4a8c) #2 0x0000000004ba726a SignalHandler(int) Signals.cpp:0:0 #3 0x00007f003642a520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520) #4 0x00007f003647ea7c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76 #5 0x00007f003647ea7c __pthread_kill_internal ./nptl/pthread_kill.c:78:10 #6 0x00007f003647ea7c pthread_kill ./nptl/pthread_kill.c:89:10 #7 0x00007f003642a476 gsignal ./signal/../sysdeps/posix/raise.c:27:6 #8 0x00007f00364107f3 abort ./stdlib/abort.c:81:7 #9 0x00007f003670a3ef (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd83ef) #10 0x0000000005ca89d6 (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x5ca89d6) #11 0x0000000005c70c43 llvm::bolt::BinaryFunction::replaceJumpTableEntryIn(llvm::bolt::BinaryBasicBlock*, llvm::bolt::BinaryBasicBlock*, llvm::bolt::BinaryBasicBlock*) (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x5c70c43) #12 0x0000000005c70dc8 llvm::bolt::BinaryFunction::splitEdge(llvm::bolt::BinaryBasicBlock*, llvm::bolt::BinaryBasicBlock*) (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x5c70dc8) #13 0x0000000005185a8b llvm::bolt::Instrumentation::instrumentFunction(llvm::bolt::BinaryFunction&, unsigned short) (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x5185a8b) #14 0x0000000005caed2e llvm::bolt::ParallelUtilities::runOnEachFunctionWithUniqueAllocId(llvm::bolt::BinaryContext&, llvm::bolt::ParallelUtilities::SchedulingPolicy, std::function<void (llvm::bolt::BinaryFunction&, unsigned short)>, std::function<bool (llvm::bolt::BinaryFunction const&)>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, unsigned int)::$_1::operator()(std::_Rb_tree_iterator<std::pair<unsigned long const, llvm::bolt::BinaryFunction>>, std::_Rb_tree_iterator<std::pair<unsigned long const, llvm::bolt::BinaryFunction>>, unsigned short) const ParallelUtilities.cpp:0:0 #15 0x0000000005cae5e7 llvm::bolt::ParallelUtilities::runOnEachFunctionWithUniqueAllocId(llvm::bolt::BinaryContext&, llvm::bolt::ParallelUtilities::SchedulingPolicy, std::function<void (llvm::bolt::BinaryFunction&, unsigned short)>, std::function<bool (llvm::bolt::BinaryFunction const&)>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, unsigned int) (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x5cae5e7) #16 0x0000000005186477 llvm::bolt::Instrumentation::runOnFunctions(llvm::bolt::BinaryContext&) (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x5186477) #17 0x0000000004be39de llvm::bolt::BinaryFunctionPassManager::runPasses() (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x4be39de) #18 0x0000000004be69ab llvm::bolt::BinaryFunctionPassManager::runAllPasses(llvm::bolt::BinaryContext&) (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x4be69ab) #19 0x0000000004bf8a9e llvm::bolt::RewriteInstance::run() (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x4bf8a9e) #20 0x00000000033ba540 main (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x33ba540) #21 0x00007f0036411d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #22 0x00007f0036411e40 call_init ./csu/../csu/libc-start.c:128:20 #23 0x00007f0036411e40 __libc_start_main ./csu/../csu/libc-start.c:379:5 #24 0x00000000033b8565 _start (/home/jayfoad2/llvm-release/bin/llvm-bolt+0x33b8565) PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Stack dump: 0. Program arguments: /home/jayfoad2/llvm-release/bin/llvm-bolt /home/jayfoad2/llvm-release/tools/bolt/test/runtime/X86/Output/instrumentation-dup-jts.s.tmp.exe --instrument --instrumentation-file=/home/jayfoad2/llvm-release/tools/bolt/test/runtime/X86/Output/instrumentation-dup-jts.s.tmp.fdata -o /home/jayfoad2/llvm-release/tools/bolt/test/runtime/X86/Output/instrumentation-dup-jts.s.tmp.instrumented /home/jayfoad2/llvm-release/tools/bolt/test/runtime/X86/Output/instrumentation-dup-jts.s.script: line 6: 2457829 Aborted (core dumped) /home/jayfoad2/llvm-release/bin/llvm-bolt /home/jayfoad2/llvm-release/tools/bolt/test/runtime/X86/Output/instrumentation-dup-jts.s.tmp.exe --instrument --instrumentation-file=/home/jayfoad2/llvm-release/tools/bolt/test/runtime/X86/Output/instrumentation-dup-jts.s.tmp.fdata -o /home/jayfoad2/llvm-release/tools/bolt/test/runtime/X86/Output/instrumentation-dup-jts.s.tmp.instrumented
Comment Actions
@sscalpone could you help look at the flang failures? On my own machine with a Release+Asserts build I see:
Failed Tests (10): Flang :: Fir/non-trivial-procedure-binding-description.f90 Flang :: HLFIR/no_reassoc-codegen.fir Flang :: Lower/OpenMP/omp-parallel-lastprivate-clause-scalar.f90 Flang :: Lower/allocatable-assignment.f90 Flang :: Lower/array-expression.f90 Flang :: Lower/forall/character-1.f90 Flang :: Runtime/no-cpp-dep.c Flang :: Semantics/OpenACC/acc-canonicalization-validity.f90 flang-Unit :: Frontend/./FlangFrontendTests/5/7 flang-Unit :: Frontend/./FlangFrontendTests/6/7
Comment Actions
I can't measure the performance impact without a good deal of setup on a Windows machine, but I believe the equivalent for MSVC would be _CONTAINER_DEBUG_LEVEL=1
Comment Actions
@vzakhari @clementval @jeanPerier could any of you help look at these Flang failures please? If you use libstdc++ you should be able to reproduce them in a Debug build (or a Release build with LLVM_ENABLE_ASSERTIONS=ON) by setting CMAKE_CXX_FLAGS=-D_GLIBCXX_ASSERTIONS.
Comment Actions
@vzakhari @clementval @jeanPerier could any of you help look at these Flang failures please? If you use libstdc++ you should be able to reproduce them in a Debug build (or a Release build with LLVM_ENABLE_ASSERTIONS=ON) by setting CMAKE_CXX_FLAGS=-D_GLIBCXX_ASSERTIONS.
Comment Actions
Thanks!
I see one remaining failure in my Release+Asserts build. Can you reproduce this one?
******************** TEST 'Flang :: Runtime/no-cpp-dep.c' FAILED ******************** Script: -- : 'RUN: at line 8'; /usr/lib/ccache/clang -std=c99 /home/jayfoad2/git/llvm-project/flang/test/Runtime/no-cpp-dep.c -I/home/jayfoad2/git/llvm-project/flang/include /home/jayfoad2/llvm-release/lib/libFortranRuntime.a /home/jayfoad2/llvm-release/lib/libFortranDecimal.a -lm -o /dev/null -- Exit Code: 1 Command Output (stderr): -- /usr/bin/ld: /home/jayfoad2/llvm-release/lib/libFortranRuntime.a(unit.cpp.o): in function `Fortran::runtime::io::ExternalFileUnit::OpenUnit(std::optional<Fortran::runtime::io::OpenStatus>, std::optional<Fortran::runtime::io::Action>, Fortran::runtime::io::Position, std::unique_ptr<char, Fortran::runtime::OwningPtrDeleter<char> >&&, unsigned long, Fortran::runtime::Convert, Fortran::runtime::io::IoErrorHandler&)': unit.cpp:(.text._ZN7Fortran7runtime2io16ExternalFileUnit8OpenUnitESt8optionalINS1_10OpenStatusEES3_INS1_6ActionEENS1_8PositionEOSt10unique_ptrIcNS0_16OwningPtrDeleterIcEEEmNS0_7ConvertERNS1_14IoErrorHandlerE+0x563): undefined reference to `std::__glibcxx_assert_fail(char const*, int, char const*, char const*)' /usr/bin/ld: /home/jayfoad2/llvm-release/lib/libFortranRuntime.a(unit.cpp.o): in function `Fortran::runtime::io::ExternalFileUnit::Receive(char*, unsigned long, unsigned long, Fortran::runtime::io::IoErrorHandler&)': unit.cpp:(.text._ZN7Fortran7runtime2io16ExternalFileUnit7ReceiveEPcmmRNS1_14IoErrorHandlerE+0x218): undefined reference to `std::__glibcxx_assert_fail(char const*, int, char const*, char const*)' /usr/bin/ld: /home/jayfoad2/llvm-release/lib/libFortranRuntime.a(unit.cpp.o): in function `Fortran::runtime::io::ExternalFileUnit::BeginReadingRecord(Fortran::runtime::io::IoErrorHandler&)': unit.cpp:(.text._ZN7Fortran7runtime2io16ExternalFileUnit18BeginReadingRecordERNS1_14IoErrorHandlerE+0x199): undefined reference to `std::__glibcxx_assert_fail(char const*, int, char const*, char const*)' /usr/bin/ld: /home/jayfoad2/llvm-release/lib/libFortranRuntime.a(unit.cpp.o): in function `Fortran::runtime::io::ExternalFileUnit::BeginSequentialVariableUnformattedInputRecord(Fortran::runtime::io::IoErrorHandler&)': unit.cpp:(.text._ZN7Fortran7runtime2io16ExternalFileUnit45BeginSequentialVariableUnformattedInputRecordERNS1_14IoErrorHandlerE+0x1c7): undefined reference to `std::__glibcxx_assert_fail(char const*, int, char const*, char const*)' /usr/bin/ld: /home/jayfoad2/llvm-release/lib/libFortranRuntime.a(unit.cpp.o): in function `Fortran::runtime::io::ExternalFileUnit::FinishReadingRecord(Fortran::runtime::io::IoErrorHandler&)': unit.cpp:(.text._ZN7Fortran7runtime2io16ExternalFileUnit19FinishReadingRecordERNS1_14IoErrorHandlerE+0x1ad): undefined reference to `std::__glibcxx_assert_fail(char const*, int, char const*, char const*)' /usr/bin/ld: /home/jayfoad2/llvm-release/lib/libFortranRuntime.a(unit.cpp.o):unit.cpp:(.text._ZN7Fortran7runtime2io16ExternalFileUnit13AdvanceRecordERNS1_14IoErrorHandlerE+0x250): more undefined references to `std::__glibcxx_assert_fail(char const*, int, char const*, char const*)' follow clang-17: error: linker command failed with exit code 1 (use -v to see invocation)
Comment Actions
+ @rovka who added this test. Does it make sense to apply settings like LLVM_ENABLE_ASSERTIONS to code in the runtime library? Does this test pass in an LLVM_ENABLE_EXPENSIVE_CHECKS build?
Comment Actions
Interesting. I can reproduce with clang as the host compiler but not with gcc. I'm using clang-14 and gcc-11 on Ubuntu 22.04.
Comment Actions
Sorry, I could not reproduce it, but I have older clang and gcc. I may suggest preprocessing unit.cpp (that is used to produce unit.cpp.o which is part of libFortranRuntime.a) and see where std::__glibcxx_assert_fail reference is coming from and how/why.
Comment Actions
I've investigated a bit more. It's a difference between libstdc++-11 and libstdc++-12, caused by this patch:
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=a54ce8865a885bca5ab9c4aa6ec725cd13c09901
Previously a failed assertion would print a message with __builtin_printf. Now it prints a message by calling __glibcxx_assert_fail which is defined in the library.
What's the way forward for the flang runtime library? Can we disable libstdc++ assertions? Can we provide our own implementation of __glibcxx_assert_fail?
Comment Actions
Well, the flang runtime library definitely doesn't want a dependency on libstdc++.
I'm not sure what to say about enabling or disabling the libstdc++ assertions. What do other runtimes do? It might be sensible to disable them, since flang users wouldn't be able to control this and may not even be aware of whether or not their flang was built with assertions enabled. From what I can see, for now the assertions only appear in the IO part of the runtime, so we wouldn't be introducing unwanted asserts in people's performance-sensitive code, but it still feels safer to disable these for the runtime libraries (FortranRuntime and FortranDecimal).
EDIT: I asked on the flang slack instance, hopefully we'll get an opinion from someone more actively involved.
Comment Actions
Here's the start of the discussion that inspired this patch: https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/26
Comment Actions
It looks like compiler-rt uses the standard library & it seems OK to me that that runtime could be built with different flags/with assertions or not? What's the concern? That someone might have a runtime library that performs poorly without knowing it? Presumably that's already possible if they build the runtime library without optimizations enabled?
Comment Actions
Thanks! The consensus on slack seemed to be that we should disable assertions (maybe just libstdc++ assertions?) in the runtime library. Does anyone know how to implement that in cmake? I experimented with remove_definitions(_GLIBCXX_ASSERTIONS) in flang/runtime/CMakeLists.txt but couldn't get it to work.
Comment Actions
You may try to modify COMPILE_DEFINITIONS property of the runtime library target or try adding -D in remove_definitions(-D_GLIBCXX_ASSERTIONS).
In both cases, I think, you may have to account for -D vs /D difference.
Comment Actions
@mehdi_amini @rriddle the pre-merge checks for this patch seem to consistently fail MLIR :: Examples/standalone/test.toy but I can't see why and I can't reproduce it locally, even if I follow the "Reproduce build locally" instructions from the builder. Do you have any ideas? Thanks.
Comment Actions
I can reproduce the failure now (I think I somehow missed the cmake option -DLLVM_BUILD_EXAMPLES=ON). I don't know much about cmake, but the problem seems to be that the LLVMConfig.cmake generated in the build directory contains:
set(LLVM_DEFINITIONS "-DBUILD_EXAMPLES -D_GNU_SOURCE -D_DEBUG -D$<$<COMPILE_LANGUAGE:CXX>:_GLIBCXX_ASSERTIONS> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS")
Note that it contains an unevaluated generator expression, which I think is a problem. There is some discussion here which suggests you could use file(GENERATE ...) instead of configure_file(...) to create LLVMConfig.cmake, but I'm not prepared to tackle that.
For completeness there are also some cmake bug reports including this one which might explain how the string containing the unevaluated generator expression is not correctly escaped when cmake uses it in a ninja build, leading to the ninja syntax error:
ninja: error: build.ninja:621: bad $-escape (literal $ must be written as $$)
I'm going to avoid all of this by not using generator expressions.
Comment Actions
I'll never admit this. You can't make me admit it. No amount of torture will surpass the pain of admitting this....
I digress...
Unfortunately because of the way we bake compilation flags into llvm-config, you can't use generator expressions for global compiler definition. Someday we should kill llvm-config and fix that.
One thing you could do is have a check like:
check_cxx_source_compiles("
#include <iosfwd>
#if !defined(__GLIBCXX__)
#error Not using libstcxx
#endif
int main() { return 0; }
"
USING_LIBSTDCXX)This would allow you to detect if you're building against libstcxx or another c++ library. Then you could only add the define if this sets USING_LIBSTDCXX to true.
Comment Actions
This change introduces compilation errors when building in at least some version(s) of gcc/g++; while what we're seeing is clearly a gcc/g++ bug, it would be good to avoid it if possible.
With gcc at version Debian 12.2.0-10:
echo '#include <string>
void f(int n) {
for (int i = 0; i < n; i++) {
std::string s;
switch (i) {
case 0: s = "x"; break;
case 1: s = "y"; break;
case 2: s = "z"; break;
case 3: s = "w"; break;
default: s = std::to_string(i); break;
}
}
}
' | g++ -D_GLIBCXX_ASSERTIONS -x c++ - -O3 -Werror=restrict -c -o /dev/nullThis will result in the compilation failure:
In file included from /usr/include/c++/12/string:40,
from <stdin>:1:
In static member function ‘static std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)’,
inlined from ‘static void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:423:21,
inlined from ‘std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.tcc:532:22,
inlined from ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::assign(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:1647:19,
inlined from ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:815:28,
inlined from ‘void f(int)’ at <stdin>:6:17:
/usr/include/c++/12/bits/char_traits.h:431:56: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ accessing 9223372036854775810 or more bytes at offsets [18, 9223372036854775807] and 17 may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
431 | return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
| ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~...again, while this seems almost certainly to be a bug in defining _GLIBCXX_ASSERTIONS for (some?) versions of gcc, it means you can't compile LLVM from source using this version of gcc with this patch in place.
If it possible to finesse this patch to detect the gcc version and avoid defining _GLIBCXX_ASSERTIONS? Alternately, could there be a way to continue allowing LLVM_ENABLE_ASSERTIONS to be set, without defining _GLIBCXX_ASSERTIONS?
Comment Actions
I can reproduce the error above on a debian:bookworm docker using gcc-12 12.2.0-14. But why are you hitting this when building LLVM? Where does -Werror=restrict come from? I don't think we enable that in normal LLVM builds.
Comment Actions
I can reproduce the error above on a debian:bookworm docker using gcc-12 12.2.0-14. But why are you hitting this when building LLVM? Where does -Werror=restrict come from? I don't think we enable that in normal LLVM builds.
The error comes not when building LLVM itself, but when building something that *uses* LLVM: the LLVM #includes now bring in the _GLIBCXX_ASSERTIONS definition. Our code had previously enabled -Wrestrict, but since _GLIBCXX_ASSERTIONS wasn't enabled before, this apparently-pre-existing-bug in gcc didn't show itself. In other words: the addition of _GLIBCXX_ASSERTIONS changes the downstream compilation environment for anything that is using LLVM as a library and compiling under gcc.
EDIT: I actually get plenty of *warnings* about this when compiling LLVM itself on my system, e.g.
In static member function ‘static std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)’,
inlined from ‘static void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:423:21,
inlined from ‘std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.tcc:532:22,
inlined from ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:2171:19,
inlined from ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::insert(size_type, const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:1928:22,
inlined from ‘std::__cxx11::basic_string<_CharT, _Traits, _Allocator> std::operator+(const _CharT*, __cxx11::basic_string<_CharT, _Traits, _Allocator>&&) [with _CharT = char; _Traits = char_traits<char>; _Alloc = allocator<char>]’ at /usr/include/c++/12/bits/basic_string.h:3541:36,
inlined from ‘std::string getIntrinsicNameImpl(llvm::Intrinsic::ID, llvm::ArrayRef<llvm::Type*>, llvm::Module*, llvm::FunctionType*, bool)’ at /usr/local/google/home/srj/GitHub/llvm-project/17/llvm/lib/IR/Function.cpp:996:19:
/usr/include/c++/12/bits/char_traits.h:431:56: warning: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ accessing 9223372036854775810 or more bytes at offsets [2, 9223372036854775807] and 1 may overlap up to 9223372036854775813 bytes at offset -3 [-Wrestrict]
431 | return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
| ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~...but since LLVM doesn't compile with treat-warnings-as-errors, these don't prevent it from compiling. (Out project normally builds with treat-warnings-as-errors and so this breaks out build.)
Comment Actions
The warning is reported here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105545
I'm not sure what's best to do. The status quo is that you get lots of compile time warnings. We could add a check for that in the cmake scripts, and disable _GLIBCXX_ASSERTIONS, but then you would silently lose the assertions.
Comment Actions
At this point, we're just gonna work around it by explicitly removing -D_GLIBCXX_ASSERTIONS from the set of cxxflags that LLVM specifies.
Comment Actions
Seems important that LLVM users can use LLVM assertions without libstdc++ assertions, since it'd put a wider restriction on their code (especially if there are bugs in some versions of libstdc++ assertions) - though having one imply the other by default seems OK too.
Comment Actions
The part I don't understand is if you have a project that is (quote) "using LLVM as a library", why would flags defined in LLVM's cmake scripts "leak out" and affect your whole project? That just seems wrong to me. But then I really don't know anything about including cmake projects in other cmake projects.
Comment Actions
LLVM’s C++ interfaces do not provide a stable ABI across all configuration flags (which is not uncommon for C++). As a result llvm-config and the LLVM CMake exports provide the options LLVM was built with for the user.
While it may be true that we could filter some of those options safely, it gets a bit tricky. Options like enabling assertions, or dump methods need to be in sync between the prebuilt LLVM and the user’s library.
Comment Actions
Yes, this. As a practical matter, code using LLVM as a library needs to know about the compiler flags and -D definitions it was built with in order to have sane ABI compatibility, unfortunately.
Comment Actions
Specifically, assertions are intended to break ABI (so you should be able to mix/match LLVM and user code that has different LLVM assertions enabled/disabled) - there's a separate abi breaking checks macro for the cases that need abi break.
I guess by that logic we could say that LLVM assertions enabled/disabled (& libstdc++ assertions enabled/disabled) shouldn't go into llvm-config at all? Users can opt in/out of it as they see fit in their code, idnependent of how the llvm library was built? or at least keep those on-for-users by default, but the ability to configure them to not be on-by-defaeult?
Comment Actions
Specifically, assertions are intended to break ABI
I hope you meant "to not break ABI". That's certainly true of _GLIBCXX_ASSERTIONS which is not "ABI-changing" according to https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html
Kind of nitpick.
This would require CMake 3.12. The current requirement is higher (3.14 and is going to be 3.20).
It would also be nice to only add this flag when compiling against libstdc++, but I don't know how to do that.