Hello,
Thanks for fixes!
LGTM after fixing minor comments below.
<snipped>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:
please replace dot with colon, because below the enumeration of disabled tests| 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>
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.
because "when set to the default value (zero), bad indentationset(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}")
bad indentation
Okay.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); } +#endifI 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>