Hello, Thanks for fixes! LGTM after fixing minor comments below. On 16.12.2024 19:40, Sergey Kaplun wrote: > 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: > > | 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 (original suppression file in LuaJIT) and > | an additional one (maintained by us). > | > | Also, this patch disables the following tests when running with Valgrind > | due to failures. please replace dot with colon, because below the enumeration of disabled tests > | > | The 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 >>> > 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), the return value from Valgrind will always be the return value of the process being simulated.", I believe it is an important thing, and it's worth to add to comment. > 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}) bad indentation > set(LUAJIT_TEST_COMMAND > - "${VALGRIND} --error-exitcode=1 " > - "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") bad indentation > 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. > Okay. >>> } >>> >>> #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */ > >