<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Maxim,</p>
<p><br>
</p>
<p>thanks for the patch! See comments below.<br>
</p>
<div class="moz-cite-prefix">On 12.09.2024 13:21, mandesero--- via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20240912102153.163481-2-mandesero@gmail.com">
<pre class="moz-quote-pre" wrap="">From: Maksim Tiushev <a class="moz-txt-link-rfc2396E" href="mailto:mandesero@gmail.com"><mandesero@gmail.com></a>
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)
+</pre>
</blockquote>
Excess empty line, please remove.<br>
<blockquote type="cite"
cite="mid:20240912102153.163481-2-mandesero@gmail.com">
<pre class="moz-quote-pre" wrap="">
+ 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.")</pre>
</blockquote>
<p>realloc -> realloc()</p>
<p>to highlight that realloc is a function<br>
</p>
<blockquote type="cite"
cite="mid:20240912102153.163481-2-mandesero@gmail.com">
<pre class="moz-quote-pre" wrap="">
+ endif()
+
+ set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")</pre>
</blockquote>
I would add a prefix VALGRIND_ and put variable outside of
condition. Feel free to ignore.<br>
<blockquote type="cite"
cite="mid:20240912102153.163481-2-mandesero@gmail.com">
<pre class="moz-quote-pre" wrap="">
+ set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
+
+ set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")</pre>
</blockquote>
<p>In the commit message, you say that VALGRIND_OPTIONS is an env
variable.</p>
<p>However, it is not:</p>
<p><br>
</p>
<p>$ VALGRIND_OPTIONS=XXXX ctest -R
test/tarantool-tests/arm64-ccall-fp-convention.test.lua
--verbose <br>
</p>
<p><snipped><br>
</p>
<p>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"
<br>
69: Working Directory:
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/test/tarantool-tests
<br>
69: Environment variables: <br>
</p>
<p><snipped></p>
<p><br>
</p>
<p>Also, replace "valgrind" in LUAJIT_TEST_COMMAND by
${VALGRIND_BIN} and</p>
<p>put "find_program(VALGRIND valgrind)" before:</p>
<p><br>
</p>
<p>--- a/test/CMakeLists.txt<br>
+++ b/test/CMakeLists.txt<br>
@@ -83,10 +83,11 @@ if(LUAJIT_USE_VALGRIND)<br>
" realloc usually doesn't return addresses in the right
address range.")<br>
endif()<br>
<br>
+ find_program(VALGRIND valgrind)<br>
set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")<br>
set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE}
$ENV{VALGRIND_OPTIONS}")<br>
<br>
- set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS}
${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")<br>
+ set(LUAJIT_TEST_COMMAND "${VALGRIND} ${VALGRIND_OPTIONS}
${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")<br>
else()<br>
set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
dofile[[${LUAJIT_TEST_INIT}]]")<br>
endif()<br>
</p>
<blockquote type="cite"
cite="mid:20240912102153.163481-2-mandesero@gmail.com">
<pre class="moz-quote-pre" wrap="">
+else()
+ set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")</pre>
</blockquote>
<p>I would add default set() with "LUAJIT_TEST_COMMAND" and remove a
second branch:</p>
<p><br>
</p>
<p>set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
dofile[[${LUAJIT_TEST_INIT}]]")<br>
if(LUAJIT_USE_VALGRIND)<br>
</p>
<p><snipped><br>
</p>
<p>set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS}
${LUAJIT_TEST_COMMAND}")<br>
</p>
<p>endif()<br>
</p>
<blockquote type="cite"
cite="mid:20240912102153.163481-2-mandesero@gmail.com">
<pre class="moz-quote-pre" wrap="">
+endif()
+
separate_arguments(LUAJIT_TEST_COMMAND)
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>