MDEV-35770 Augment DBUG_ASSERT with __builtin_assume

[UltraGreed]

• The Jira issue number for this PR is: MDEV-35770

Description

The DBUG_ASSERT macro tests an invariant at runtime in debug build and aborts if the check fails. Although assertions are disabled in optimized builds, code is usually written in such a manner, that failing an assertion condition would lead to undefined behavior. It therefore makes sense to introduce platform-specific analog of C++23 [[assume(cond)]] and use it in these cases, which potentially could lead to generation of more optimized builds in production.
E.g. compiler would be able to optimize while loop into do-while loop (which means less instructions in assembly) if it knows that the loop will run at least once.

Special patterns case

There are lots of code fragments like:

DBUG_ASSERT(!thd->in_sub_stmt);
if (thd->in_sub_stmt)  {
  ...
}

and

switch (i)
{
  case 1: x=myisam_sint1korr(from); break;
  case 2: x=myisam_sint2korr(from); break;
  case 3: x=myisam_sint3korr(from); break;
  case 4: x=myisam_sint4korr(from); break;
  default: DBUG_ASSERT(0); x= 0;
}

If we simply wrap DBUG_ASSERT in an assume, the compiler will elide both the if-guard and the default branch — removing important fallback code.

There are also cases like:

  void acquire()
  {
    IF_DBUG(auto prev= ,)
    ref_count.fetch_add(1);
    DBUG_ASSERT(prev != 0);
  }

In this case compilation in release mode would fail, because assume is called with undeclared variable.

This PR:

1. Introduces new platform-dependent macro MY_ASSUME, implemented for GCC >= 13.0, Clang and MSVC.
2. Augments DBUG_ASSERT to use it by default in DBUG. Notably, it will enable MY_ASSUME in the release mode as well.
3. Provides DBUG_ASSERT_NO_ASSUME (identical to the old DBUG_ASSERT) for described above special cases.

MY_ASSUME is implemented for GCC >= 13.0, Clang and MSVC.

TODO: Measure actual performance improvements and document LLVM/assembly diffs. -- will stay out of scope for this PR

How can this PR be tested?

This patch doesn't intend to change the behaviour, so it's only important that the existing tests will pass. We assume that test coverage of the special cases is out of scope for this work -- it was supposed to have been done in scope of the same task when it was added, if that was feasible at all.

We acknowledge we may have missed a few “special” patterns, but we’ve covered all the occurrences we could find.

Basing the PR against the correct MariaDB version

• This is a new feature or a refactoring, and the PR is based against the main branch.
• This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

• I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
• For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

[CLAassistant]

All committers have signed the CLA.

[svoj]

Hi @UltraGreed! What is the intention of this pull request? Why is it submitted under MDEV-35770 umbrella? What kind of feedback are you expecting from MariaDB developers?

[svoj]

@FooBarrior is mentoring this effort.

[UltraGreed]

Hello, @svoj. Sorry for the delayed response. I've updated the PR to better reflect its intention. The only commit uploaded so far is a small fix (related to described in the PR problem), which will be squashed into the main commit later.

[svoj]

Reset draft status as the ball is in mentor's hands.

[FooBarrior]

I have made the (minor) changes I wanted, now this looks good to me.

@svoj please also take a look as a second reviewer

[FooBarrior]

as a good example where it clearly has an effect, I would provide translog_mark_file_finished in ma_loghandler.c.

There we could see, that not only clang 21 does a loop inversion, but also a partial loop unrolling, even under -O2

	movq	log_descriptor+8391888(%rip), %rax
	cmpl	%ebx, (%rax) // <------------- check the first element agressively
	jne	.LBB92_5
	xorl	%ecx, %ecx
	jmp	.LBB92_8
.LBB92_5:
	movl	$1, %ecx  
	cmpl	%ebx, 8(%rax) // <------------- check the second element agressively
	je	.LBB92_8
	movl	$1, %edx
.LBB92_7:
	leaq	1(%rdx), %rcx // <------------ start from 2:  rcx (i) = rdx + 1 (т.е. 1+1=2)
	cmpl	%ebx, 8(%rax,%rdx,8)
	movq	%rcx, %rdx
	jne	.LBB92_7

[vaintroub]

please take a look on Windows optimized build. CI won't block that for you, on whatever reason, but appveyor does actually complain.
C:\projects\server\mysys\my_virtual_mem.c(71,5): error C2220: the following warning is treated as an error DBUG_ASSERT(is_memory_committed(ptr, size)); ^ C:\projects\server\mysys\my_virtual_mem.c(71,5): warning C4013: 'is_memory_committed' undefined; assuming extern returning int

[FooBarrior]

@vaintroub can you tell something about appveyor results now -- is it somehing common, or is it this PR's issue?

[vaintroub]

If this does not happen elsewhere, e.g in the base branch for this one, but happens here, then it is PR issue.
If you look at what it complains (destructor never returns), and look at the corresponding code, it might appear obvious (not for GCC, though)

 virtual ~Sql_cmd()
 {
   /*
     Sql_cmd objects are allocated in thd->mem_root.
     In MySQL, the C++ destructor is never called, the underlying MEM_ROOT is
     simply destroyed instead.
     Do not rely on the destructor for any cleanup.
   */
   DBUG_ASSERT(false);
 }

[svoj]

If this does not happen elsewhere, e.g in the base branch for this one, but happens here, then it is PR issue. If you look at what it complains (destructor never returns), and look at the corresponding code, it might appear obvious (not for GCC, though)

 virtual ~Sql_cmd()
 {
   /*
     Sql_cmd objects are allocated in thd->mem_root.
     In MySQL, the C++ destructor is never called, the underlying MEM_ROOT is
     simply destroyed instead.
     Do not rely on the destructor for any cleanup.
   */
   DBUG_ASSERT(false);
 }

Should this be rewritten as something like ~Sql_cmd()= delete;? Or why don't we move Sql_cmd cleanup towards destructor and call it explicitly?

[FooBarrior]

Should this be rewritten as something like ~Sql_cmd()= delete;?

yes, and perhaps remove virtual?

[FooBarrior]

Hmm I guess we can't do it @svoj.
g++ - <<< 'struct A{ A(){}; ~A()=delete; }; A *a = new A; int main(){}'
builds just fine.

However adding derivation breaks it:

g++ -xc++ - <<< 'struct A{ A(){}; ~A()=delete; }; 
struct B:A{ B():A(){}; int x; ~B()=delete; };
 B *a = new B; int main(){}'
<stdin>: In constructor ‘B::B()’:
<stdin>:1:51: error: use of deleted function ‘A::~A()’
<stdin>:1:18: note: declared here

[FooBarrior]

So our best bet as i see it is to make those assertions _NO_ASSUME. It seems that C++ requires a reachable destructor (even though we know it will not be reachable,).

[svoj]

A few concerns from my side, mostly inline with assert != assume whitepaper shared by @FooBarrior.

1. It is primarily advertised as a performance optimisation. Though I do trust that assume may give some moderate performance improvement, it has to be proven by benchmarks. E.g. Xiang Fan mentioned in the whitepaper:

Much to our surprise, the C1xx front end actually got a 1-2% faster with __assume statements removed.

2. There is no clear explanation how to choose between DBUG_ASSERT_NO_ASSUME() and DBUG_ASSERT(). Even if we come up with one, it most probably going to be quite sophisticated document. I would expect broad misuse of DBUG_ASSERT(), which may lead to p.4.

3. There is no way to disable it at compile time. Although this is easily solvable.

4. New set of potential (practically undebuggable) issues in release builds. These are discussed in the whitepaper.

Unless there's a clear performance improvement, I'd definitely vote against "assert is assume" approach. I'd even be very cautious with "assume is assert" approach advertised by the whitepaper: too easy to misuse.

But the good thing is that now we know places where while can be replaced with do ... while.

[vuvova]

I generally disagree with the assert != assume whitepaper, I think they use asserts incorrectly, and think that assert = assume.

Still, the point on whether it will actually improve the performance is valid. If it doesn't help, it's just useless code change that only makes it more complex.

[svoj]

@vuvova we have ~10000 DBUG_ASSERT occurrences, ~300 were converted to DBUG_ASSERT_NO_ASSUME(), which already gives 3% cases that didn't work with assume. And I guess we'll get some more as PR appears to be still WiP.

A few examples...

debug assert = debug assume, but debug assert != release assume, because it depends on some DBUG specific code.

static void remove_ptr_from_dynarray(DYNAMIC_ARRAY *array, void *ptr)
{
  bool found __attribute__((unused))= false;
  for (size_t i= 0; i < array->elements; i++)
  {
    if (ptr == *dynamic_element(array, i, void**))
    {
      DBUG_ASSERT(!found);
      delete_dynamic_element(array, i);
      IF_DBUG_ASSERT(found= true, break);
    }
  }
  DBUG_ASSERT(found);
}

Somewhat unrelated, but still... here we don't want to assume mutex is locked, we want to terminate execution. It generally applies to all ut_a() calls.

ut_a(!pthread_mutex_lock(&m_mutex));

We may want to interrupt debug builds on incorrect inputs, but let release builds handle it. We don't want this handling to be optimised away.

my_off_t my_get_ptr(uchar *ptr, size_t pack_length)
{
  my_off_t pos;
  switch (pack_length) {
#if SIZEOF_OFF_T > 4
  case 8: pos= (my_off_t) mi_uint8korr(ptr); break;
  case 7: pos= (my_off_t) mi_uint7korr(ptr); break;
  case 6: pos= (my_off_t) mi_uint6korr(ptr); break;
  case 5: pos= (my_off_t) mi_uint5korr(ptr); break;
#endif
  case 4: pos= (my_off_t) mi_uint4korr(ptr); break;
  case 3: pos= (my_off_t) mi_uint3korr(ptr); break;
  case 2: pos= (my_off_t) mi_uint2korr(ptr); break;
  case 1: pos= (my_off_t) *(uchar*) ptr; break;
  default: DBUG_ASSER(0); return 0;
  }
 return pos;
}

This one wouldn't be correct assumption either, we want to interrupt here:

  /*
    If one day we will support execution of multi-statements
    through PS API we should not issue SHOW WARNINGS until
    we have not read all results...
  */
  DBUG_ASSERT(!mysql_more_results(mysql));

Yes, these can be incorrect DBUG_ASSERT() use patterns. But we have them, and we will be getting more of them. If we blindly replace them with assume in release builds we'll get code mined with bugs that can probably be debugged in assembler mode only. That's expensive.

[vuvova]

1. agree, if we assert an expression that uses debug-only variables, it won't compile without debug, there's nothing to assume.
2. ut_a(!pthread_mutex_lock(&m_mutex)); — I guess, it's not a debug assert at all, it's production "crash if not true" code.
3. seems safe to assume, my_get_ptr() is only used on values that were previously written by my_store_ptr(). In memory during this process execution time. It's a correct usage of an assert, it shows that something that can never happen.
4. this is safe to assume as long as we don't support "multi-statements through PS API". When we will, the assert will fire in the debug more, we'll remove it, and this will remove the assume too. Strictly speaking, it's a wrong assert, as it asserts values read over the socket from another process, but it's mysqltest, not a production code, within its domain the assert is fine

Here only case 1 is a valid reason not to assume asserts

[gkodinov]

This is a preliminary review. Can you please address the comments by the reviewer? There's also failing build tests that need to be fixed.

[gkodinov]

I have played a bit with the diff: merged it, cleaned it up etc.

The only thing failing is the clang memsan build atm. And it's failing massively. I didn't prove it, but my guess is that it's now checking something that depends on the data structures being set to 0 in debug mode and not initialized otherwise. Or there's some side effect of the function calls in DBUG_ASSERT().

I would join @svoj in requesting a benchmark.

I've grep-ed through the DBUG_ASSERTs: there's about 1.5k of these. A serious amount of them are calling functions. Some of them quite heavy. So having these in assume() I believe doesn't contribute to the performance speedup, but further slows the run in optimized mode since it now needs to evaluate these functions. Also a good % of the ASSERTs do not take advantage of assuming the things that are asserted: most of them are double-checking state of complex data structures before executing an action on them.

So, while I understand and can relate to the basic premise of the PR, I believe that we need to take a different approach: instead of doing that to all DBUG_ASSERTs by default we need to have a DBUG_ASSERT_ASSUME() instead, then evaluate each case where DBUG_ASSERT() is used and replace with _ASSUME() if there's going to be benefits.

IMHO, to have benefit from the assert_assume, the DBUG_ASSERT should:

• not be calling any functions
• there should be a similar condition right after the DBUG_ASSERT() or close to it.
• ideally the DBUG_ASSERT() should just be checking a single condition over a single variable.
• The expression should reference a variable. constant expressions are not worth it.