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

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:

<snipped>

| 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.

please replace dot with colon, because below the enumeration of disabled 
tests

Replaced.

|
| 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

<snipped>

    )
+  # 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}")

<snipped>

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)

<snipped>