Hi, Sergey,
thanks for the patch! Please see comments below
s/valgrind/Valgrind/From: Maksim Tiushev <mandesero@gmail.com> This patch enables running tests with Valgrind. There is a `VALGRIND_OPTS` variable [1] that we can set -- it makes the usage of valgrind more flexible -- we can define any necessary flags in the
command line (not at the building stage). By default, the suppression
There is also a config file .valgrindrc [1], why env variable is preffered?
[1]:
https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts
files are set to <src/lj.supp> (original suppression file in LuaJIT) and an additional one <src/lj_extra.supp> (maintained by us). Also, this patch disables the following tests when running with Valgrind due to failures:
Please add a ticket url to commit message
(https://github.com/tarantool/tarantool/issues/10803).
this test is missed in a ticket description, please addDisabled due tarantool/tarantool#10803: - tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua - tarantool-tests/lj-512-profiler-hook-finalizers.test.lua - tarantool-tests/lj-726-profile-flush-close.test.lua - tarantool-tests/misclib-sysprof-lapi.test.lua - tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
Timed out due to running under Valgrind: - tarantool-tests/gh-7745-oom-on-trace.test.lua - tarantool-tests/lj-1034-tabov-error-frame.test.lua - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test
[1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts
Part of tarantool/tarantool#3705
---
CMakeLists.txt | 5 +++++
src/lj_extra.supp | 19 +++++++++++++++++++
test/CMakeLists.txt | 19 +++++++++++++++++++
test/tarantool-c-tests/CMakeLists.txt | 11 ++++++++++-
.../gh-8594-sysprof-ffunc-crash.test.c | 9 +++++++++
test/tarantool-tests/CMakeLists.txt | 6 ++++++
.../gh-7745-oom-on-trace.test.lua | 1 +
.../lj-1034-tabov-error-frame.test.lua | 1 +
.../lj-512-profiler-hook-finalizers.test.lua | 5 ++++-
.../lj-726-profile-flush-close.test.lua | 5 ++++-
.../profilers/gh-5688-tool-cli-flag.test.lua | 2 ++
...4-add-proto-trace-sysprof-default.test.lua | 2 ++
.../profilers/misclib-sysprof-lapi.test.lua | 2 ++
13 files changed, 84 insertions(+), 3 deletions(-)
create mode 100644 src/lj_extra.supp
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/src/lj_extra.supp b/src/lj_extra.supp
new file mode 100644
index 00000000..ac205f03
--- /dev/null
+++ b/src/lj_extra.supp
@@ -0,0 +1,19 @@
+# Valgrind suppression file maintained by the Tarantool team.
+
+# Missed from the original suppression file.
+# Unaligned access to the string, see <lj_str.c> for the details.
+{
+ Optimized string compare
+ Memcheck:Addr4
+ fun:lj_getu32
+ fun:str_fastcmp
+}
+
+# Missed from the original suppression file.
+# `lj_str_cmp()` may read up to 3 extra bytes from the end of the
+# string. It is OK due to the aligned allocations.
+{
+ Optimized string compare
+ Memcheck:Cond
+ fun:lj_str_cmp
+}
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 0db2dd8b..bda85ec1 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -69,6 +69,25 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
)
set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+
+if(LUAJIT_USE_VALGRIND)
+ if (NOT LUAJIT_USE_SYSMALLOC)
+ message(WARNING
+ "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()
+
+ find_program(VALGRIND valgrind)
Please check that VALGRIND variable is not empty
and put this condition before "if (NOT LUAJIT_USE_SYSMALLOC)"
+ list(APPEND LUAJIT_TEST_VALGRIND_SUPP
+ --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
+ --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
+ )
+ set(LUAJIT_TEST_COMMAND
+ "${VALGRIND} --error-exitcode=1 "
please add a comment why --error-exitcode is used. From a manual page:
--error-exitcode=<number> [default: 0]
Specifies an alternative exit code to return if
Valgrind reported any errors in the run. When set to the default
value (zero), the return value from Valgrind will always be the
return value of the process being simulated. When set to a nonzero
value, that value is returned instead, if Valgrind detects any
errors. This is useful for using Valgrind as part of an automated
test suite, since it makes it easy to detect test cases for which
Valgrind has reported errors, just by inspecting return codes.
+ "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
With "list(APPEND BENCHMARK_CMAKE_FLAGS "AA" "BB")" you don't need trailing whitespaces.
Please replace "set()" with "list(APPEND ... )" like above.
+endif()
+
separate_arguments(LUAJIT_TEST_COMMAND)
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index 30d174bb..5f6c45da 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -56,10 +56,19 @@ foreach(test_source ${tests})
# Generate CMake tests.
set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}")
+ set(test_command ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX})
+
+ if(LUAJIT_USE_VALGRIND)
+ set(test_command ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}
The same already defined in test/CMakeLists.txt, could you reuse
already defined value?
+ ${test_command}
+ )
+ endif()
+
add_test(NAME ${test_title}
- COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}
+ COMMAND ${test_command}
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
)
+
set_tests_properties(${test_title} PROPERTIES
LABELS ${TEST_SUITE_NAME}
DEPENDS tarantool-c-tests-deps
diff --git a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
index cf1d815a..35108e77 100644
--- a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
+++ b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
@@ -46,6 +46,8 @@
* * https://github.com/tarantool/tarantool/issues/9387
*/
+#define UNUSED(x) ((void)(x))
+
#define MESSAGE "Canary is alive"
#define LUACALL "local a = tostring('" MESSAGE "') return a"
@@ -248,6 +250,12 @@ static int tracer(pid_t chpid)
static int test_tostring_call(void *ctx)
{
+#if LUAJIT_USE_VALGRIND
+ UNUSED(ctx);
+ UNUSED(tracer);
+ UNUSED(tracee);
+ return skip("Disabled with Valgrind (Timeout)");
+#else
pid_t chpid = fork();
switch(chpid) {
case -1:
@@ -264,6 +272,7 @@ static int test_tostring_call(void *ctx)
default:
return tracer(chpid);
}
+#endif
I propose to use a testsuite specific timeout for tarantool-c-tests
and increase it's value instead of disabling tests.
Hint: we can set increased timeout under option
LUAJIT_USE_VALGRIND only.
}
#else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 8bdb2cf3..848c4cab 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -89,6 +89,12 @@ if(LUAJIT_ENABLE_TABLE_BUMP)
list(APPEND LUA_TEST_ENV_MORE LUAJIT_TABLE_BUMP=1)
endif()
+# Needed to skip some long tests or tests using signals.
+# See also https://github.com/tarantool/tarantool/issues/10803.
+if(LUAJIT_USE_VALGRIND)
+ list(APPEND LUA_TEST_ENV_MORE LUAJIT_TEST_USE_VALGRIND=1)
+endif()
+
set(TEST_SUITE_NAME "tarantool-tests")
# XXX: The call produces both test and target
diff --git a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
index fe320251..c481da24 100644
--- a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
+++ b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
@@ -6,6 +6,7 @@ local test = tap.test('OOM on trace'):skipcond({
(jit.os == 'OSX'),
['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
['Test requires JIT enabled'] = not jit.status(),
+ ['Disabled with Valgrind (Timeout)'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
})
test:plan(1)
diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
index 0e23fdb2..de63b6d2 100644
--- a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
+++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
@@ -4,6 +4,7 @@ local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
['Test requires JIT enabled'] = not jit.status(),
['Test requires GC64 mode enabled'] = not ffi.abi('gc64'),
['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
+ ['Disabled with Valgrind (Timeout)'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
})
-- XXX: The test for the problem uses the table of GC
diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
index f02bd05f..32b12c65 100644
--- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
+++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
@@ -1,7 +1,10 @@
local tap = require('tap')
local profile = require('jit.profile')
-local test = tap.test('lj-512-profiler-hook-finalizers')
+local test = tap.test('lj-512-profiler-hook-finalizers'):skipcond({
+ -- See also https://github.com/tarantool/tarantool/issues/10803.
+ ['Disabled due to #10803'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
+})
test:plan(1)
-- Sampling interval in ms.
diff --git a/test/tarantool-tests/lj-726-profile-flush-close.test.lua b/test/tarantool-tests/lj-726-profile-flush-close.test.lua
index 36cca43d..b26f0603 100644
--- a/test/tarantool-tests/lj-726-profile-flush-close.test.lua
+++ b/test/tarantool-tests/lj-726-profile-flush-close.test.lua
@@ -1,6 +1,9 @@
local tap = require('tap')
-local test = tap.test('lj-726-profile-flush-close')
+local test = tap.test('lj-726-profile-flush-close'):skipcond({
+ -- See also https://github.com/tarantool/tarantool/issues/10803.
+ ['Disabled due to #10803'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
+})
test:plan(1)
local TEST_FILE = 'lj-726-profile-flush-close.profile'
diff --git a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
index f935dc5b..ad0d6caf 100644
--- a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
+++ b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
@@ -7,6 +7,8 @@ local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
['No profile tools CLI option integration'] = _TARANTOOL,
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
+ -- See also https://github.com/tarantool/tarantool/issues/10803.
+ ['Disabled due to #10803'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
})
test:plan(3)
diff --git a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
index 176c1a15..d9a3080e 100644
--- a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
+++ b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
@@ -6,6 +6,8 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
+ -- See also https://github.com/tarantool/tarantool/issues/10803.
+ ['Disabled due to #10803'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
})
test:plan(2)
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 26c277cd..605ff91c 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -5,6 +5,8 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
+ -- See also https://github.com/tarantool/tarantool/issues/10803.
+ ['Disabled due to #10803'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
})
test:plan(19)