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