MDEV-34579: sp-no-valgrind fails when server is compiled with valgrind

[cvicentiu]

• [x] The Jira issue number for this PR is: MDEV-34579

Description

The issue is described in detail in JIRA. This sets of commits reverts #3393 and fixes the typo related to HAVE_VALGRIND flag.

How can this PR be tested?

Compile the server with WITH_VALGRIND support. Run sp-no-valgrind without running under --valgrind MTR flag. The test should be skipped.

Basing the PR against the correct MariaDB version

• [x] This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

[grooverdan]

Yes: -DWITH_VALGRIND=OFF , mtr passes

As does: -DWITH_ASAN=ON, mtr (not valgrind)

But not:

• -DWITH_VALGRIND=ON , mtr
• or -DWITH_ASAN=ON, mtr --valgrind

But our build infrastructure only one valid configuration, WITH_VALGRIND=ON, mtr --valgrind. Should we really be maintaining another build configuration for the one test case where it will make a difference? The purpose of test case if measuring memory usage is independent of any address or memory sanitizer.

Alternate:

• Can we just better document the test case to say this really only applies to the build valgrind option? and not do a revert?
• If we don't need a build valgrind mtr test we could remove the mysqld.cc adding suffix as nothing uses it.

If there really is a need to maintain a valgrind the version, because of ./include/my_valgrind.h:

#if __has_feature(memory_sanitizer)
# include <sanitizer/msan_interface.h>
# define HAVE_valgrind

The actual define would be #if defined HAVE_valgrind && !__has_feature(memory_sanitizer)

Can we go though a minimization of valid configurations at some stage? The number of special conditions under HAVE_valgrind assumedly to make it pass is truely scary.

[LinuxJedi]

I do think the HAVE_valgrind is a bad name, I assume it is like that to stop potential conflicts with HAVE_VALGRIND in the git submodules.

I also think that some of the HAVE_valgrind sections could be fixed properly with no performance impact.

Maybe mtr should detect the -valgrind version and enable --valgrind implicitly? Is there a reason why you wouldn't want to do --valgrind every time when you build with Valgrind support?

[grooverdan]

I do think the HAVE_valgrind is a bad name, I assume it is like that to stop potential conflicts with HAVE_VALGRIND in the git submodules.

libmariadb/libmariadb/ma_context.c - right. And the cmake define isn't passed though.

I also think that some of the HAVE_valgrind sections could be fixed properly with no performance impact.

me too. And better than no performance impact, higher reliability. I have a branch in testing.

Maybe mtr should detect the -valgrind version and enable --valgrind implicitly? Is there a reason why you wouldn't want to do --valgrind every time when you build with Valgrind support?

There's

• HAVE_valgrind defining a number of memory protection macros
• HAVE_valgrind is cover a number a) undefined behaviour and b) bugs/coverage deficiencies, likely former bugs/deficiencies, in valgrind
• mtr --valgrind - running the build binary in emulation (which is independent of the memory protection macros). The libmariadbd might be runtime.

With the correction the 2) items they are a independent I think.