Hi, Maxim,


thanks for the patch! See comments below.

On 12.09.2024 13:21, mandesero--- via Tarantool-patches wrote:
From: Maksim Tiushev <mandesero@gmail.com>

This patch adds the ability to run tests using Valgrind. Custom
Valgrind testing options can be set via the environment variable
`VALGRIND_OPTIONS`. By default, only the suppression file is set
to `src/lj.supp`.
---
 CMakeLists.txt      |  5 +++++
 test/CMakeLists.txt | 24 +++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index aa2103b3..854b3613 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -289,6 +289,11 @@ endif()
 # ASan enabled.
 option(LUAJIT_USE_ASAN "Build LuaJIT with AddressSanitizer" OFF)
 if(LUAJIT_USE_ASAN)
+  if(LUAJIT_USE_VALGRIND)
+    message(FATAL_ERROR
+      "AddressSanitizer and Valgrind cannot be used simultaneously."
+    )
+  endif()
   if(NOT LUAJIT_USE_SYSMALLOC)
     message(WARNING
       "Unfortunately, internal LuaJIT memory allocator is not instrumented yet,"
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 0db2dd8b..280f0042 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -68,7 +68,29 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
   ${PROJECT_NAME}-codespell
 )
 
-set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+if(LUAJIT_USE_VALGRIND)
+
Excess empty line, please remove.
+  if (NOT LUAJIT_USE_SYSMALLOC)
+    message(FATAL_ERROR
+      "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()
+
+  if (NOT LUAJIT_ENABLE_GC64)
+    message(FATAL_ERROR
+      "LUAJIT_USE_SYSMALLOC cannot be enabled on x64 without GC64, since"
+      " realloc usually doesn't return addresses in the right address range.")

realloc -> realloc()

to highlight that realloc is a function

+  endif()
+
+  set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")
I would add a prefix VALGRIND_ and put variable outside of condition. Feel free to ignore.
+  set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
+
+  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")

In the commit message, you say that VALGRIND_OPTIONS is an env variable.

However, it is not:


$ VALGRIND_OPTIONS=XXXX ctest -R test/tarantool-tests/arm64-ccall-fp-convention.test.lua --verbose         

<snipped>

69: Test command: /bin/valgrind "--suppressions=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj.supp" "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/src/luajit" "-e" "dofile[[/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/luajit-test-init.lua]]" "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/arm64-ccall-fp-convention.test.lua"        
69: Working Directory: /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/test/tarantool-tests                                
69: Environment variables: 

<snipped>


Also, replace "valgrind" in LUAJIT_TEST_COMMAND by ${VALGRIND_BIN} and

put "find_program(VALGRIND valgrind)" before:


--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -83,10 +83,11 @@ if(LUAJIT_USE_VALGRIND)
       " realloc usually doesn't return addresses in the right address range.")
   endif()
 
+  find_program(VALGRIND valgrind)
   set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")
   set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
 
-  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+  set(LUAJIT_TEST_COMMAND "${VALGRIND} ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 else()
   set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 endif()

+else()
+  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")

I would add default set() with "LUAJIT_TEST_COMMAND" and remove a second branch:


set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
if(LUAJIT_USE_VALGRIND)

<snipped>

set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_COMMAND}")

endif()

+endif()
+
 separate_arguments(LUAJIT_TEST_COMMAND)
 
 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")