[PATCH] D142279: [cmake] Use LLVM_ENABLE_ASSERTIONS to enable assertions in libstdc++

Sat Jan 21 08:30:55 PST 2023

nikic added a comment.

In D142279#4071163 <https://reviews.llvm.org/D142279#4071163>, @barannikov88 wrote:

> In D142279#4071142 <https://reviews.llvm.org/D142279#4071142>, @nikic wrote:
>
>> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142279/new/

https://reviews.llvm.org/D142279