[Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind
Sergey Bronnikov
sergeyb at tarantool.org
Tue Dec 17 14:42:04 MSK 2024
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>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20241217/a0a91d11/attachment.htm>
More information about the Tarantool-patches
mailing list