Skip to content
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

MDEV-31727: pcre stack size not functioning on clang-16 #2700

Merged
merged 2 commits into from Jul 21, 2023

Conversation

grooverdan
Copy link
Member

Thanks Zhixu Liu for the great bug report.

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

Description

noinline attribute was being ignored by clang-16 and reporting 32 stack size on Gentoo, 16 locally on Fedora 38.

Based on https://stackoverflow.com/questions/54481855/clang-ignoring-attribute-noinline changed the noinline to noopt.

After that the -pcre_exec(NULL, NULL, NULL, -999, -999, 0, NULL, 0) returned 1056, simlar to gcc.

From https://bugs.gentoo.org/910188.

How can this PR be tested?

Manual compile against bundled pcre with code per gentoo bug report.

Basing the PR against the correct MariaDB version

  • This is a new feature and the PR is based against the latest MariaDB development branch.
  • [ X ] 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

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

@grooverdan grooverdan requested a review from vuvova July 18, 2023 03:03
@grooverdan grooverdan added the MariaDB Foundation Pull requests created by MariaDB Foundation label Jul 18, 2023
@clan
Copy link

clan commented Jul 18, 2023

Does gcc support attribute((optnone)) ?
If not, maybe we should add a compiler check?

noinline attribute was being ignored by clang-16 and reporting
32 stack size on Gentoo, 16 locally on Fedora 38.

Based on https://stackoverflow.com/questions/54481855/clang-ignoring-attribute-noinline
appended noopt in addition to the gcc recognised attributes.

After that the -pcre_exec(NULL, NULL, NULL, -999, -999, 0, NULL, 0)
returned 1056, simlar to gcc.

From https://bugs.gentoo.org/910188.

Thanks Zhixu Liu for the great bug report.
@grooverdan
Copy link
Member Author

gcc-13.1.1 doesn't support the optnone, and clang didn't support the noclone, but both seem to ignore the ones they don't support.

@vuvova
Copy link
Member

vuvova commented Jul 21, 2023

ideally, the fix should work with the system pcre too. If we patch the bundled version, MariaDB will not be able to use system pcre anymore

UPD: ok, it's 10.4-only, and we check whether system pcre is usable or not, then I suppose we can fix only bundled. It'll be up to Gentoo/Fedora to patch their distro version of pcre if they want to.

@grooverdan
Copy link
Member Author

And luckily for the most part system builds are still gcc based.

@vuvova still got an account to update https://bugs.exim.org/show_bug.cgi?id=2173, apparently other's had trouble registering.

@grooverdan grooverdan merged commit 73c9415 into MariaDB:10.4 Jul 21, 2023
8 of 11 checks passed
@grooverdan grooverdan deleted the bb-10.4-MDEV-31727 branch July 21, 2023 10:21
@thesamesam
Copy link

ideally, the fix should work with the system pcre too. If we patch the bundled version, MariaDB will not be able to use system pcre anymore

UPD: ok, it's 10.4-only, and we check whether system pcre is usable or not, then I suppose we can fix only bundled. It'll be up to Gentoo/Fedora to patch their distro version of pcre if they want to.

Done for Gentoo, thanks!

@dr-m
Copy link
Contributor

dr-m commented Aug 4, 2023

I wonder if this is in any way related to MDEV-31605, which involves JSON functions.

I was thinking of adding the following to include/my_compiler.h:

#ifdef __clang__
# define ATTRIBUTE_OPTNONE __attribute__((optnone))
#elif defined __GNUC__
# define ATTRIBUTE_OPTNONE __attribute__((optimize(0)))
#else
# define ATTRIBUTE_OPTNONE /* empty */
#endif

As you probably know, Clang defines __GNUC__ (as 4), so we would need the checks in that order. The oldest GCC version that we currently support is 4.8.5, and it does recognize the optimize attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MariaDB Foundation Pull requests created by MariaDB Foundation
5 participants