[Tarantool-patches] [PATCH v2 luajit 1/3] cmake: run tests with Valgrind
Sergey Bronnikov
sergeyb at tarantool.org
Thu Sep 12 21:52:13 MSK 2024
Hi, Maxim,
thanks for the patch! See comments below.
On 12.09.2024 13:21, mandesero--- via Tarantool-patches wrote:
> From: Maksim Tiushev<mandesero at 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")
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20240912/27dac2f7/attachment.htm>
More information about the Tarantool-patches
mailing list