Hi, Maxim,
thanks for the patch! See comments below.
Excess empty line, please remove.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) +
+ 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
I would add a prefix VALGRIND_ and put variable outside of condition. Feel free to ignore.+ endif() + + 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}]]")
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")