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: 8373 bytes --]

Hello,

Thanks for fixes!

LGTM after fixing minor comments below.

On 16.12.2024 19:40, Sergey Kaplun wrote:
> Hi, Sergey!
> Thansk for the review!
> See my answers below, all patches are force-pushed to the branch.
> The branch is rebased on master.
>
> On 13.12.24, Sergey Bronnikov wrote:
>
<snipped>
> | cmake: run tests with Valgrind
> |
> | 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
> | 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 replace dot with colon, because below the enumeration of disabled 
tests
> |
> | The <tarantool-tests/lj-512-profiler-hook-finalizers.test.lua> test is
> | disabled due to its time sensitivity (it is not run the expected amount
> | of time with Valgrind).
> |
> | These tests from the tarantool-tests suite are disabled due to
> | tarantool/tarantool#10803:
> |   - lj-726-profile-flush-close.test.lua
> |   - profilers/gh-5688-tool-cli-flag.test.lua
> |   - profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
> |   - profilers/misclib-sysprof-lapi.test.lua
> |
> | Timed out due to running under Valgrind:
> |   - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test
> |   - tarantool-tests/gh-7745-oom-on-trace.test.lua
> |   - tarantool-tests/lj-1034-tabov-error-frame.test.lua
> |
> | [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
>>>

<snipped>

> Added the comment.
> ===================================================================
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index d9cab615..9685c494 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -88,6 +88,8 @@ if(LUAJIT_USE_VALGRIND)
>       --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
>       --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
>     )
> +  # Set the exit code to non-zero to automatically detect
> +  # failing tests.
because "when set to the default value (zero),
the return value from Valgrind will always be the return value of the
process being simulated.", I believe it is an important thing, and it's 
worth to add to comment.
>     set(LUAJIT_TEST_COMMAND
>       "${VALGRIND} --error-exitcode=1 "
>       "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
> ===================================================================
>
>>> +    "${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.
> I suppose you mean `list(PREPEND ...)` [1] (since `APPEND` inserts
> elements into the end of the list), but this is available only from
> CMake version 3.15, while the minimum supported version is 2.8. So,
> ignored for now.
>
> [1]:https://cmake.org/cmake/help/latest/command/list.html#prepend
>
>>> +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?
> Yes, good catch!
> ===================================================================
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 9685c494..fcd697c9 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -90,9 +90,10 @@ if(LUAJIT_USE_VALGRIND)
>     )
>     # Set the exit code to non-zero to automatically detect
>     # failing tests.
> +  set(LUAJIT_TEST_VALGRIND_COMMAND
> +    ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP})
bad indentation
>     set(LUAJIT_TEST_COMMAND
> -    "${VALGRIND} --error-exitcode=1 "
> -    "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
> +    "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
bad indentation
>   endif()
>   
>   separate_arguments(LUAJIT_TEST_COMMAND)
> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> index 5f6c45da..c4a402d0 100644
> --- a/test/tarantool-c-tests/CMakeLists.txt
> +++ b/test/tarantool-c-tests/CMakeLists.txt
> @@ -59,9 +59,7 @@ foreach(test_source ${tests})
>     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}
> -      ${test_command}
> -    )
> +    set(test_command ${LUAJIT_TEST_VALGRIND_COMMAND} ${test_command})
>     endif()
>   
>     add_test(NAME ${test_title}
> ===================================================================
>
>>> +      ${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.
> The problem is that this test will run for too long, so it is just
> easier to skip it.
>
Okay.

>>>    }
>>>    
>>>    #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */
> <snipped>
>

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