[Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind
Sergey Kaplun
skaplun at tarantool.org
Mon Dec 16 19:40:44 MSK 2024
Hi, Sergey!
Thansk for the review!
See my answers below, all patches are force-pushed to the branch.
The branch is rebased on master.
On 13.12.24, Sergey Bronnikov wrote:
> Hi, Sergey,
>
> thanks for the patch! Please see comments below
>
> On 11.12.2024 16:21, Sergey Kaplun wrote:
> > From: Maksim Tiushev<mandesero at gmail.com>
> >
> > This patch enables running tests with Valgrind. There is a
> > `VALGRIND_OPTS` variable [1] that we can set -- it makes the usage of
> > valgrind more flexible -- we can define any necessary flags in the
> s/valgrind/Valgrind/
Fixed, thanks!
> > command line (not at the building stage). By default, the suppression
>
> There is also a config file .valgrindrc [1], why env variable is preffered?
>
> [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts
It is more flexible. For example, we may easily test different
configurations locally for the single build without changing any
sources.
>
> > files are set to <src/lj.supp> (original suppression file in LuaJIT) and
> > an additional one <src/lj_extra.supp> (maintained by us).
> >
> > Also, this patch disables the following tests when running with Valgrind
> > due to failures:
>
> Please add a ticket url to commit message
> (https://github.com/tarantool/tarantool/issues/10803).
It is already mentioned (see the line below).
>
> >
> > Disabled due tarantool/tarantool#10803:
> > - tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> > - tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
Updated the commit message regarding this test.
> > - tarantool-tests/lj-726-profile-flush-close.test.lua
> > - tarantool-tests/misclib-sysprof-lapi.test.lua
> > - tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
> this test is missed in a ticket description, please add
Added.
> >
> > Timed out due to running under Valgrind:
> > - tarantool-tests/gh-7745-oom-on-trace.test.lua
> > - tarantool-tests/lj-1034-tabov-error-frame.test.lua
> > - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test
> > [1]:https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts
> >
> > Part of tarantool/tarantool#3705
| cmake: run tests with Valgrind
|
| This patch enables running tests with Valgrind. There is a
| `VALGRIND_OPTS` variable [1] that we can set -- it makes the usage of
| Valgrind more flexible -- we can define any necessary flags in the
| command line (not at the building stage). By default, the suppression
| files are set to <src/lj.supp> (original suppression file in LuaJIT) and
| an additional one <src/lj_extra.supp> (maintained by us).
|
| Also, this patch disables the following tests when running with Valgrind
| due to failures.
|
| The <tarantool-tests/lj-512-profiler-hook-finalizers.test.lua> test is
| disabled due to its time sensitivity (it is not run the expected amount
| of time with Valgrind).
|
| These tests from the tarantool-tests suite are disabled due to
| tarantool/tarantool#10803:
| - lj-726-profile-flush-close.test.lua
| - profilers/gh-5688-tool-cli-flag.test.lua
| - profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
| - profilers/misclib-sysprof-lapi.test.lua
|
| Timed out due to running under Valgrind:
| - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test
| - tarantool-tests/gh-7745-oom-on-trace.test.lua
| - tarantool-tests/lj-1034-tabov-error-frame.test.lua
|
| [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts
|
| Part of tarantool/tarantool#3705
> > ---
> > CMakeLists.txt | 5 +++++
> > src/lj_extra.supp | 19 +++++++++++++++++++
> > test/CMakeLists.txt | 19 +++++++++++++++++++
> > test/tarantool-c-tests/CMakeLists.txt | 11 ++++++++++-
> > .../gh-8594-sysprof-ffunc-crash.test.c | 9 +++++++++
> > test/tarantool-tests/CMakeLists.txt | 6 ++++++
> > .../gh-7745-oom-on-trace.test.lua | 1 +
> > .../lj-1034-tabov-error-frame.test.lua | 1 +
> > .../lj-512-profiler-hook-finalizers.test.lua | 5 ++++-
> > .../lj-726-profile-flush-close.test.lua | 5 ++++-
> > .../profilers/gh-5688-tool-cli-flag.test.lua | 2 ++
> > ...4-add-proto-trace-sysprof-default.test.lua | 2 ++
> > .../profilers/misclib-sysprof-lapi.test.lua | 2 ++
> > 13 files changed, 84 insertions(+), 3 deletions(-)
> > create mode 100644 src/lj_extra.supp
> >
<snipped>
> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index 0db2dd8b..bda85ec1 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -69,6 +69,25 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
> > )
> >
> > set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
> > +
> > +if(LUAJIT_USE_VALGRIND)
> > + if (NOT LUAJIT_USE_SYSMALLOC)
Fixed indentation.
===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index bda85ec1..4590e065 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -71,7 +71,7 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
if(LUAJIT_USE_VALGRIND)
- if (NOT LUAJIT_USE_SYSMALLOC)
+ if(NOT LUAJIT_USE_SYSMALLOC)
message(WARNING
"LUAJIT_USE_SYSMALLOC option is mandatory for Valgrind's memcheck tool"
" on x64 and the only way to get useful results from it for all other"
===================================================================
> > + message(WARNING
> > + "LUAJIT_USE_SYSMALLOC option is mandatory for Valgrind's memcheck tool"
> > + " on x64 and the only way to get useful results from it for all other"
> > + " architectures.")
> > + endif()
> > +
> > + find_program(VALGRIND valgrind)
>
> Please check that VALGRIND variable is not empty
>
> and put this condition before "if (NOT LUAJIT_USE_SYSMALLOC)"
Added, now the error is raised in that case:
===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 4590e065..d9cab615 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -71,6 +71,12 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
if(LUAJIT_USE_VALGRIND)
+ find_program(VALGRIND valgrind)
+ if(NOT VALGRIND)
+ message(FATAL_ERROR "`valgrind' not found when LUAJIT_USE_VALGRIND option "
+ "is enabled.")
+ endif()
+
if(NOT LUAJIT_USE_SYSMALLOC)
message(WARNING
"LUAJIT_USE_SYSMALLOC option is mandatory for Valgrind's memcheck tool"
@@ -78,7 +84,6 @@ if(LUAJIT_USE_VALGRIND)
" architectures.")
endif()
- find_program(VALGRIND valgrind)
list(APPEND LUAJIT_TEST_VALGRIND_SUPP
--suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
--suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
===================================================================
>
> > + list(APPEND LUAJIT_TEST_VALGRIND_SUPP
> > + --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
> > + --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
> > + )
> > + set(LUAJIT_TEST_COMMAND
> > + "${VALGRIND} --error-exitcode=1 "
>
> please add a comment why --error-exitcode is used. From a manual page:
>
> --error-exitcode=<number> [default: 0]
> Specifies an alternative exit code to return if Valgrind
> reported any errors in the run. When set to the default value (zero),
> the return value from Valgrind will always be the return value of the
> process being simulated. When set to a nonzero value, that value is
> returned instead, if Valgrind detects any errors. This is useful for
> using Valgrind as part of an automated test suite, since it makes it
> easy to detect test cases for which Valgrind has reported errors, just
> by inspecting return codes.
Added the comment.
===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index d9cab615..9685c494 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -88,6 +88,8 @@ if(LUAJIT_USE_VALGRIND)
--suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
--suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
)
+ # Set the exit code to non-zero to automatically detect
+ # failing tests.
set(LUAJIT_TEST_COMMAND
"${VALGRIND} --error-exitcode=1 "
"${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
===================================================================
>
> > + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
>
> With "list(APPEND BENCHMARK_CMAKE_FLAGS "AA" "BB")" you don't need
> trailing whitespaces.
>
> Please replace "set()" with "list(APPEND ... )" like above.
I suppose you mean `list(PREPEND ...)` [1] (since `APPEND` inserts
elements into the end of the list), but this is available only from
CMake version 3.15, while the minimum supported version is 2.8. So,
ignored for now.
[1]: https://cmake.org/cmake/help/latest/command/list.html#prepend
>
> > +endif()
> > +
> > separate_arguments(LUAJIT_TEST_COMMAND)
> >
> > set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
> > diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> > index 30d174bb..5f6c45da 100644
> > --- a/test/tarantool-c-tests/CMakeLists.txt
> > +++ b/test/tarantool-c-tests/CMakeLists.txt
> > @@ -56,10 +56,19 @@ foreach(test_source ${tests})
> >
> > # Generate CMake tests.
> > set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}")
> > + set(test_command ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX})
> > +
> > + if(LUAJIT_USE_VALGRIND)
> > + set(test_command ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}
> The same already defined in test/CMakeLists.txt, could you reuse already
> defined value?
Yes, good catch!
===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 9685c494..fcd697c9 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -90,9 +90,10 @@ if(LUAJIT_USE_VALGRIND)
)
# Set the exit code to non-zero to automatically detect
# failing tests.
+ set(LUAJIT_TEST_VALGRIND_COMMAND
+ ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP})
set(LUAJIT_TEST_COMMAND
- "${VALGRIND} --error-exitcode=1 "
- "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
+ "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
endif()
separate_arguments(LUAJIT_TEST_COMMAND)
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index 5f6c45da..c4a402d0 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -59,9 +59,7 @@ foreach(test_source ${tests})
set(test_command ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX})
if(LUAJIT_USE_VALGRIND)
- set(test_command ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}
- ${test_command}
- )
+ set(test_command ${LUAJIT_TEST_VALGRIND_COMMAND} ${test_command})
endif()
add_test(NAME ${test_title}
===================================================================
> > + ${test_command}
> > + )
> > + endif()
> > +
> > add_test(NAME ${test_title}
> > - COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}
> > + COMMAND ${test_command}
> > WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> > )
> > +
> > set_tests_properties(${test_title} PROPERTIES
> > LABELS ${TEST_SUITE_NAME}
> > DEPENDS tarantool-c-tests-deps
> > diff --git a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
> > index cf1d815a..35108e77 100644
> > --- a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
> > +++ b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
> > @@ -46,6 +46,8 @@
> > * *https://github.com/tarantool/tarantool/issues/9387
> > */
> >
> > +#define UNUSED(x) ((void)(x))
> > +
> > #define MESSAGE "Canary is alive"
> > #define LUACALL "local a = tostring('" MESSAGE "') return a"
> >
> > @@ -248,6 +250,12 @@ static int tracer(pid_t chpid)
> >
> > static int test_tostring_call(void *ctx)
> > {
> > +#if LUAJIT_USE_VALGRIND
> > + UNUSED(ctx);
> > + UNUSED(tracer);
> > + UNUSED(tracee);
> > + return skip("Disabled with Valgrind (Timeout)");
> > +#else
> > pid_t chpid = fork();
> > switch(chpid) {
> > case -1:
> > @@ -264,6 +272,7 @@ static int test_tostring_call(void *ctx)
> > default:
> > return tracer(chpid);
> > }
> > +#endif
>
> I propose to use a testsuite specific timeout for tarantool-c-tests
>
> and increase it's value instead of disabling tests.
>
> Hint: we can set increased timeout under option LUAJIT_USE_VALGRIND only.
The problem is that this test will run for too long, so it is just
easier to skip it.
>
> > }
> >
> > #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */
<snipped>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list