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

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

Hi, Maksim!
Thanks for the patch!
Please consider my comments below.

On 11.11.24, mandesero@gmail.com 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`.

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).

> 
> Also this patch disables the following tests when running with Valgrind

Typo: s/Also/Also,/

> due to failures:
> 
> Disabled due #10803:

Typo: s<#10803><tarantool/tarantool#10803>

>   - test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua

Nit: Commit line width is more than 72 symbols.

>   - 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
> 
> Timed out due to running under Valgrind:
>   - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
>   - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> 
> Part of #3705

Typo: s<#3705><tarantool/tarantool#3705>

> ---
>  CMakeLists.txt                                   |  5 +++++
>  test/CMakeLists.txt                              | 16 ++++++++++++++++
>  test/tarantool-tests/CMakeLists.txt              |  3 ++-
>  .../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 ++++-
>  ...7264-add-proto-trace-sysprof-default.test.lua |  2 ++
>  .../profilers/misclib-sysprof-lapi.test.lua      |  2 ++
>  9 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index aa2103b3..854b3613 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt

<snipped>

> 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}")

I suppose that we can drop $ENV{VALGRIND_OPTIONS}, since 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).

Therefore, `LUAJIT_TEST_VALGRIND_OPTS` may be renamed to
`LUAJIT_TEST_VALGRIND_SUPP`.

> +  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 8bdb2cf3..39562f35 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -110,7 +110,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_TEST_ENV_MORE}"
> +    ENVIRONMENT "LUA_PATH=${LUA_PATH};${LUA_TEST_ENV_MORE};\
> +LJ_USE_VALGRIND=${LUAJIT_USE_VALGRIND}"

It is better to set this variable to the `LUA_TEST_ENV_MORE` list.
Also, I suggest rename it to the `LUAJIT_TEST_USE_VALGRIND`.

>      LABELS ${TEST_SUITE_NAME}
>      DEPENDS tarantool-tests-deps
>    )
> 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',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  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',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  -- 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..8f32b9ef 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

Nit: Missed dot at the end of the sentence.

> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

> +})
>  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..27eb3751 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

Nit: Missed dot at the end of the sentence.

> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

> +})
>  test:plan(1)
>  
>  local TEST_FILE = 'lj-726-profile-flush-close.profile'
> 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..e6c853a1 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

Nit: Missed dot at the end of the sentence.

> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  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..2210d668 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"),
>
Nit: Missed dot at the end of the sentence.

 +  -- See also https://github.com/tarantool/tarantool/issues/10803
> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  test:plan(19)
> -- 
> 2.34.1
> 

[1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts

-- 
Best regards,
Sergey Kaplun