Skip to content

[WIP] MDEV-35770 Augment DBUG_ASSERT with __builtin_assume #3944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

UltraGreed
Copy link

@UltraGreed UltraGreed commented Apr 3, 2025

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

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

This PR:

  1. Introduces new platform-dependent macro DBUG_ASSUME, implemented for GCC >= 13.0, Clang and MSVC.
  2. Augments DBUG_ASSERT to use it by default.
  3. Provides DBUG_ASSERT_NO_ASSUME (identical to the old DBUG_ASSERT) for described above cases.

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

TODO: Measure actual performance improvements and document LLVM/assembly diffs.

How can this PR be tested?

No additional test cases are needed, since this patch doesn't intend to change the behaviour. We may have missed a few “special” patterns, but we’ve covered all known occurrences.

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
Copy link

CLAassistant commented Apr 3, 2025

CLA assistant check
All committers have signed the CLA.

@svoj svoj added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Apr 3, 2025
@svoj
Copy link
Contributor

svoj commented Apr 16, 2025

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
Copy link
Contributor

svoj commented Apr 18, 2025

@FooBarrior is mentoring this effort.

@UltraGreed
Copy link
Author

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.

@UltraGreed UltraGreed force-pushed the main branch 2 times, most recently from ae701e4 to e996848 Compare May 30, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.
Development

Successfully merging this pull request may close these issues.

4 participants