Hi, Maxim,
thanks for the patch! See comments below.
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")