[libcxx-commits] [PATCH] D122397: [libc++] Use __builtin_expect and __builtin_assume in _LIBCPP_ASSERT
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Mar 24 12:23:53 PDT 2022
Mordante accepted this revision as: Mordante.
Mordante added a comment.
In D122397#3406074 <https://reviews.llvm.org/D122397#3406074>, @ldionne wrote:
> In D122397#3405618 <https://reviews.llvm.org/D122397#3405618>, @philnik wrote:
>
>> Maybe it would make sense to use `__builtin_assume` if assertions are disabled?
>
> My understanding is that `__builtin_assume(x)` does not evaluate `x` ever at runtime, but the documentation is a little bit light on the details. I think this is a fantastic idea, I'll try it out. If `__builtin_assume` does evaluate `x`, I added a test that should fail in my assertions handler patch.
Clang's documentation is truncated. Based on my archeology (D122423 <https://reviews.llvm.org/D122423>) the last sentence should read
`The argument itself is never evaluated, so any side effects of the expression will be discarded.`
We already claim that the assertions should be fatal and returning from the assertion handler is undefined behaviour. Therefore I think using `__builtin_assume` isn't wrong. I only wonder whether we should add an opt-out for vendors who don't like possible undefined behaviour of `__builtin_assume` in case the code hits an assertion that's wrong. (Much like the false positives in the Clang assertions.)
The original patch LGTM, but I would like you to investigate @philnik's suggestion further.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122397/new/
https://reviews.llvm.org/D122397
More information about the libcxx-commits
mailing list