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

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

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

Hi, Maxim!

thanks for the patch!


On 18.09.2024 11:50, mandesero--- via Tarantool-patches wrote:
> From: Maksim Tiushev<mandesero@gmail.com>
>
> This patch enables running tests with Valgrind. Custom Valgrind testing
> options can be configured by setting the `VALGRIND_OPTIONS` variable.
> Please note that this environment variable must be set before building,
> and any updates to it will require a project rebuild. By default, the
> suppression file is set to `src/lj.supp`.
>
> Also this patch disables the following tests when running with Valgrind
> due to failures:
>
> For SIGPROF:

s/SIGPROF/sysprof/?

>    - test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
>    - test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>    - test/tarantool-tests/lj-726-profile-flush-close.test.lua
>    - test/tarantool-tests/misclib-sysprof-lapi.test.lua
>
> For TIMEOUT:

It is not clear what do you mean with "for TIMEOUT".

Whether these tests timed out due to running under Valgrind or not?

>    - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
>    - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua


I think we need followup issues for these tests and put a link to the 
issue to the

commit message. Otherwise, these tests will remain unfixed. It is 
desirable to

describe details of failures under Valgrind in that issue.

> ---
>   CMakeLists.txt                                   |  5 +++++
>   test/CMakeLists.txt                              | 16 ++++++++++++++++
>   test/tarantool-tests/CMakeLists.txt              |  3 ++-
>   ...7264-add-proto-trace-sysprof-default.test.lua |  1 +
>   .../gh-7745-oom-on-trace.test.lua                |  1 +
>   .../lj-1034-tabov-error-frame.test.lua           |  1 +
>   .../lj-512-profiler-hook-finalizers.test.lua     |  4 +++-
>   .../lj-726-profile-flush-close.test.lua          |  4 +++-
>   .../misclib-sysprof-lapi.test.lua                |  1 +
>   9 files changed, 33 insertions(+), 3 deletions(-)
>
> 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..3cc0ddbc 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -69,6 +69,22 @@ 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)
> +  set(LUAJIT_TEST_VALGRIND_OPTS
> +    "--suppressions=${LUAJIT_SOURCE_DIR}/lj.supp $ENV{VALGRIND_OPTIONS}")
> +  set(LUAJIT_TEST_COMMAND
> +    "${VALGRIND} ${LUAJIT_TEST_VALGRIND_OPTS} ${LUAJIT_TEST_COMMAND}")
> +endif()
> +
>   separate_arguments(LUAJIT_TEST_COMMAND)
>   
>   set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index e3750bf3..2becebb7 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -130,7 +130,8 @@ foreach(test_path ${tests})
>       # LUA_CPATH and LD_LIBRARY_PATH variables and also
>       # dependencies list with libraries are set in scope of
>       # BuildTestLib macro.
> -    ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}"
> +    ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE};\
> +LJ_USE_VALGRIND=${LUAJIT_USE_VALGRIND}"
>       LABELS ${TEST_SUITE_NAME}
>       DEPENDS tarantool-tests-deps
>     )
> diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> index c1d68e3c..f700b414 100644
> --- a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> +++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> @@ -4,6 +4,7 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
>     ['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
>                                                  jit.arch ~= 'x64',
>     ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
> +  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

" (SIGPROF)" looks useless, please remove. Here and below.

I would add a number of the issue instead.

>   })
>   
>   test:plan(2)
> 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..b4ab8872 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("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   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..5e75973b 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("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   -- 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..fc2f43b7 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,9 @@
>   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({
> +  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
> +})
>   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..6d09658f 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,8 @@
>   local tap = require('tap')
>   
> -local test = tap.test('lj-726-profile-flush-close')
> +local test = tap.test('lj-726-profile-flush-close'):skipcond({
> +  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
> +})
>   test:plan(1)
>   
>   local TEST_FILE = 'lj-726-profile-flush-close.profile'
> diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> index fdaed46a..c5e2516b 100644
> --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> @@ -3,6 +3,7 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
>     ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
>                                                  jit.arch ~= "x64",
>     ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
> +  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   test:plan(19)

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