This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Use LLVM_ENABLE_ASSERTIONS to enable assertions in libstdc++
ClosedPublic

Authored by foad on Jan 21 2023, 2:43 AM.

Details

Summary

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.

Diff Detail

Event Timeline

foad created this revision.Jan 21 2023, 2:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2023, 2:43 AM
foad requested review of this revision.Jan 21 2023, 2:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2023, 2:43 AM

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.

nikic added a subscriber: nikic.Jan 21 2023, 7:28 AM

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

nikic added a comment.Jan 21 2023, 7:34 AM

As @barannikov88 pointed out, we should just be moving the existing block from EXPENSIVE_CHECKS though.

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.

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.

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.

nikic added a comment.Jan 21 2023, 8:30 AM

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.

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

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.

barannikov88 added a comment.EditedJan 21 2023, 9:04 AM

The summary of this patch suggests that it is indeed negligible:

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

foad added inline comments.Jan 22 2023, 5:03 AM
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
barannikov88 added inline comments.Jan 22 2023, 5:45 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
85

I should have checked first, sorry. The correct spelling is:
add_compile_definitions($<$<COMPILE_LANGUAGE:CXX>:_GLIBCXX_ASSERTIONS>)

foad updated this revision to Diff 491164.Jan 22 2023, 8:03 AM

Use add_compile_definitions.

foad added a subscriber: maksfb.EditedJan 22 2023, 8:06 AM

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

@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
foad added a subscriber: rafauler.Jan 23 2023, 6:29 AM
foad added subscribers: gulfem, Amir.Jan 25 2023, 1:52 AM

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

@Amir @gulfem could you help look at the BOLT failures? 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.

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

Thanks for bringing this to our attention. Fixed it in 7768f63e5

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

foad added a comment.Jan 26 2023, 2:43 AM

Thanks for bringing this to our attention. Fixed it in 7768f63e5

Thanks! That works for me.

@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

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

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

Will be fixed in D142652 and D142671

foad added a comment.Jan 27 2023, 4:04 AM

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

Will be fixed in D142652 and D142671

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)
foad added a subscriber: rovka.Jan 27 2023, 9:10 AM

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

Will be fixed in D142652 and D142671

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)

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

Trying to reproduce Runtime/no-cpp-dep.c...

Trying to reproduce Runtime/no-cpp-dep.c...

It works for me with gcc as the build compiler.

foad added a comment.Jan 27 2023, 10:33 AM

Trying to reproduce Runtime/no-cpp-dep.c...

It works for me with gcc as the build compiler.

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.

Trying to reproduce Runtime/no-cpp-dep.c...

It works for me with gcc as the build compiler.

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.

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.

foad added a comment.EditedJan 28 2023, 7:20 AM

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?

Sorry, I just noticed this, I'll have a look.

rovka added a comment.EditedJan 30 2023, 2:25 AM

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?

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.

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?

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.

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?

foad added a comment.Feb 1 2023, 5:31 AM

EDIT: I asked on the flang slack instance, hopefully we'll get an opinion from someone more actively involved.

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.

EDIT: I asked on the flang slack instance, hopefully we'll get an opinion from someone more actively involved.

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.

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.

foad updated this revision to Diff 494253.Feb 2 2023, 4:23 AM

Rebase.

EDIT: I asked on the flang slack instance, hopefully we'll get an opinion from someone more actively involved.

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.

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.

Thanks. I finally found something that works for me: D143168

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

foad added a comment.Feb 6 2023, 6:55 AM

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

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.

foad added a subscriber: beanz.Feb 6 2023, 6:57 AM

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

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.

@beanz FYI - I think I heard you were a cmake expert!

foad updated this revision to Diff 495113.Feb 6 2023, 6:58 AM

Stop using generator expressions.

beanz added a comment.Feb 6 2023, 10:05 AM

@beanz FYI - I think I heard you were a cmake expert!

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.

Is anyone prepared to approve this now that the premerge is happy?

nikic accepted this revision.Feb 6 2023, 9:55 PM

LGTM

This revision is now accepted and ready to land.Feb 6 2023, 9:55 PM
srj added a subscriber: srj.Feb 8 2023, 1:57 PM

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/null

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

foad added a comment.Feb 9 2023, 12:18 AM

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/null

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

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.

srj added a comment.EditedFeb 9 2023, 9:52 AM

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

foad added a comment.Feb 10 2023, 2:36 AM

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.

srj added a comment.Feb 10 2023, 9:02 AM

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.

At this point, we're just gonna work around it by explicitly removing -D_GLIBCXX_ASSERTIONS from the set of cxxflags that LLVM specifies.

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.

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.

foad added a comment.Feb 11 2023, 1:05 AM

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.

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.

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.

beanz added a comment.Feb 11 2023, 5:46 AM

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.

srj added a comment.Feb 13 2023, 9:50 AM

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.

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.

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.

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.

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?

foad added a comment.Feb 13 2023, 12:59 PM

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

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

Right, yes, that. Sorry for the typo/confusion.

JOE1994 added a subscriber: JOE1994.Mar 8 2023, 6:09 PM