Thanks for the fixes! LGTM BTW, I've discovered that CMake has integration with memory checking tools, see [1][2]. [1]: https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-memcheck-step [2]: https://cmake.org/cmake/help/latest/variable/CTEST_MEMORYCHECK_TYPE.html#variable:CTEST_MEMORYCHECK_TYPE On 17.12.2024 15:17, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the comments! > Fixed them and force-pushed the branch. > > On 17.12.24, Sergey Bronnikov wrote: >> 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 > Replaced. > >>> | >>> | 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 >>> > > >>> ) >>> + # 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. > Enriched the comment: > =================================================================== > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index a5075f05..87e4b907 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -88,8 +88,10 @@ 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. > + # When set to the default value (zero), the return value from > + # Valgrind will always be the return value of the process being > + # simulated. 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 > =================================================================== > >>> set(LUAJIT_TEST_COMMAND >>> "${VALGRIND} --error-exitcode=1 " >>> "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") >>> =================================================================== >>> >>>>> + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > > >>>>> 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 > Added 2 addtional spaces: > > =================================================================== > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index fcd697c9..a5075f05 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -91,9 +91,9 @@ 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}) > + ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) > set(LUAJIT_TEST_COMMAND > - "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") > + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") > endif() > > separate_arguments(LUAJIT_TEST_COMMAND) > =================================================================== > >>> endif() >>> >>> separate_arguments(LUAJIT_TEST_COMMAND) > >