Do not use __builtin_unreachable() in PCRE2_ASSERT() #485
Conversation
__builting_unreachable() implementation is not defined and has been known to not trigger failures under some compilers. Default instead to using assert(), which has the added benefit of printing a descriptive message and it is also likely more portable as it is part of ANSI C. While at it, really allow configuring builtins with cmake.
| #if defined(HAVE_BUILTIN_EXPECT) && defined(HAVE_BUILTIN_UNREACHABLE) | ||
| #define PCRE2_ASSERT(x) do { if (__builtin_expect(!(x), 0)) __builtin_unreachable(); } while (0) | ||
| #elif defined(HAVE_ASSERT_H) | ||
| #if defined(HAVE_ASSERT_H) && !defined(NDEBUG) |
There was a problem hiding this comment.
Would it make sense for the non-PCRE2_DEBUG case to do use the old (removed in this commit) version of PCRE2_ASSERT there?
There was a problem hiding this comment.
I don't think so, what is that you would like to accomplish by doing so?
There was a problem hiding this comment.
I'm not really bothered about it, I just wanted to note that it would let you keep the optimisation benefits in non-debug builds and also get a trap from UBSAN when asserts are disabled. If it's not desired, no problem.
There was a problem hiding this comment.
I see; PCRE2_ASSERT() doesn't generate any code right now in non debug builds, so it is already optimized for speed AFAIK.
There was a problem hiding this comment.
I prefer no code generated by asserts in release builds. There could be release asserts if it is needed, but I have no plans for that.
There was a problem hiding this comment.
Yeah, it just means you don't get the optimisation benefit of saying "this shouldn't be reachable" (i.e. the asserts don't turn into optimisation hints when asserts are disabled).
There was a problem hiding this comment.
it just means you don't get the optimisation benefit of saying "this shouldn't be reachable"
for that there is a different macro PCRE2_UNREACHABLE(), but their use is also likely to change (see #490), PCRE2_ASSERT() was never meant to be used to signal that, and the fact that it accidentally did because of the implementation is what is being corrected here.
There was a problem hiding this comment.
I suspect he mentions a different thing. When (!(a < 5)) goes to unreachable, the compiler can tell that a < 5, and it can use it for optimization. However, I have doubts about the usefulness of such knowledge, and the condition part of the assertion may also have side effects (forcing extra memory accesses), if the compiler cannot fully eliminate them.
Simplify PCRE2_ASSERT() and while at it remove no longer needed dependency on __builtin_expect()
Fixes: #484