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

Thanks for the fixes! LGTM

BTW, I've discovered that CMake has integration with memory checking 
tools, see [1][2].


[1]: 
https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-memcheck-step

[2]: 
https://cmake.org/cmake/help/latest/variable/CTEST_MEMORYCHECK_TYPE.html#variable:CTEST_MEMORYCHECK_TYPE

On 17.12.2024 15:17, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the comments!
> Fixed them and force-pushed the branch.
>
> On 17.12.24, Sergey Bronnikov wrote:
>> 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
> Replaced.
>
>>> |
>>> | 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
>>>
> <snipped>
>
>>>      )
>>> +  # 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.
> Enriched the comment:
> ===================================================================
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index a5075f05..87e4b907 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -88,8 +88,10 @@ 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.
> +  # When set to the default value (zero), the return value from
> +  # Valgrind will always be the return value of the process being
> +  # simulated. 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})
>     set(LUAJIT_TEST_COMMAND
> ===================================================================
>
>>>      set(LUAJIT_TEST_COMMAND
>>>        "${VALGRIND} --error-exitcode=1 "
>>>        "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
>>> ===================================================================
>>>
>>>>> +    "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
> <snipped>
>
>>>>> 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
> Added 2 addtional spaces:
>
> ===================================================================
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index fcd697c9..a5075f05 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -91,9 +91,9 @@ 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})
> +      ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP})
>     set(LUAJIT_TEST_COMMAND
> -    "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
> +      "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
>   endif()
>   
>   separate_arguments(LUAJIT_TEST_COMMAND)
> ===================================================================
>
>>>    endif()
>>>    
>>>    separate_arguments(LUAJIT_TEST_COMMAND)
> <snipped>
>

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