Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 13992 bytes --]

Hi, Sergey,

thanks for the patch! Please see comments below

On 11.12.2024 16:21, Sergey Kaplun wrote:
> 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
s/valgrind/Valgrind/
> 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).

>
> Disabled 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
this test is missed in a ticket description, please add
>
> 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 alsohttps://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 alsohttps://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 alsohttps://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 alsohttps://github.com/LuaJIT/LuaJIT/issues/606.
>     ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
> +  -- See alsohttps://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 alsohttps://github.com/LuaJIT/LuaJIT/issues/606.
>     ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
> +  -- See alsohttps://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 alsohttps://github.com/LuaJIT/LuaJIT/issues/606.
>     ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
> +  -- See alsohttps://github.com/tarantool/tarantool/issues/10803.
> +  ['Disabled due to #10803'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
>   })
>   
>   test:plan(19)

[-- Attachment #2: Type: text/html, Size: 17363 bytes --]