From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 87148467D17; Mon, 16 Dec 2024 19:41:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 87148467D17 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1734367306; bh=XwPs2f1fcq+3savKs4hhD9Qv4alCeH1yb1bq5/YH7gs=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=ojDFzONRPQmSl7749LmuNS/oWoraUPNuaSmT/iiKnLzTnnOb3SEqtMr9zLyeUTnPX e6NBGEI3MM/uUgw7n9mVOTIRpXLYuIE9sGdBoAWfn3YYmizUmhCeRrXytIj74vbA0U WmUNHePuQaUNlm3LM/pof2t8LGpNZpn09u7dT/AQ= Received: from send59.i.mail.ru (send59.i.mail.ru [89.221.237.154]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9E498467D17 for ; Mon, 16 Dec 2024 19:41:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9E498467D17 Received: by exim-smtp-84dbd655b7-785kd with esmtpa (envelope-from ) id 1tNEAC-00000000A0n-2MYu; Mon, 16 Dec 2024 19:41:45 +0300 Date: Mon, 16 Dec 2024 19:40:44 +0300 To: Sergey Bronnikov Cc: Maksim Tiushev , tarantool-patches@dev.tarantool.org Message-ID: References: <7f8dea571105b6780b05d633df0d75b8c1a8a3c1.1733909411.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9D0F949AE8EC90782B7D4D4D7C1B378948806ECB2900E9D91182A05F5380850404C228DA9ACA6FE27B2F6EA0C7AB5CECC3DE06ABAFEAF670593DFD93E61612AD6D736CC3054EF672FFFC59631B34BE54A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE721D130CF676D2164EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637416EC277BB71E4518638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D818F4DA8D43F797BAEE9DB705B7A98D042E5F1CB802964F04CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F3E38EE449E3E2AE389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC82FFDA4F57982C5F4F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2A336C651863509103F1AB874ED890284AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3D22EEC73CD09A32ABA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166176DF2183F8FC7C04E672349037D5FA5725E5C173C3A84C3335407143AA9223635872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5B5A7F9A60B5484315002B1117B3ED696308B47A460F391A8C81EEE05487B0209823CB91A9FED034534781492E4B8EEADA3FB0D9844EF8EC5BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFCBD7989CBCBFC8B038C9915755D75B22341890BE20667553161B093B97BA29E2FB9773CE30426B0E0B0B9839AE4C716C64E844B351B6CF947D7070541D0283E0270049D67BCDEC465F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojaQ2NfKPcDQMBp09E8WYR2g== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B59A91CD7272BD00C623DE06ABAFEAF670593DFD93E61612AD6B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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: > Hi, Sergey, > > thanks for the patch! Please see comments below > > On 11.12.2024 16:21, Sergey Kaplun wrote: > > From: Maksim Tiushev > > > > 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 > s/valgrind/Valgrind/ Fixed, thanks! > > command line (not at the building stage). By default, the suppression > > There is also a config file .valgrindrc [1], why env variable is preffered? > > [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts It is more flexible. For example, we may easily test different configurations locally for the single build without changing any sources. > > > files are set to (original suppression file in LuaJIT) and > > an additional one (maintained by us). > > > > Also, this patch disables the following tests when running with Valgrind > > due to failures: > > Please add a ticket url to commit message > (https://github.com/tarantool/tarantool/issues/10803). It is already mentioned (see the line below). > > > > > Disabled due tarantool/tarantool#10803: > > - tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua > > - tarantool-tests/lj-512-profiler-hook-finalizers.test.lua Updated the commit message regarding this test. > > - tarantool-tests/lj-726-profile-flush-close.test.lua > > - tarantool-tests/misclib-sysprof-lapi.test.lua > > - tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua > this test is missed in a ticket description, please add Added. > > > > Timed out due to running under Valgrind: > > - tarantool-tests/gh-7745-oom-on-trace.test.lua > > - tarantool-tests/lj-1034-tabov-error-frame.test.lua > > - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test > > [1]:https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts > > > > Part of tarantool/tarantool#3705 | 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 (original suppression file in LuaJIT) and | an additional one (maintained by us). | | Also, this patch disables the following tests when running with Valgrind | due to failures. | | The 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 > > > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > > index 0db2dd8b..bda85ec1 100644 > > --- a/test/CMakeLists.txt > > +++ b/test/CMakeLists.txt > > @@ -69,6 +69,25 @@ 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) Fixed indentation. =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index bda85ec1..4590e065 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -71,7 +71,7 @@ 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) + 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" =================================================================== > > + 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) > > Please check that VALGRIND variable is not empty > > and put this condition before "if (NOT LUAJIT_USE_SYSMALLOC)" Added, now the error is raised in that case: =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 4590e065..d9cab615 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -71,6 +71,12 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") if(LUAJIT_USE_VALGRIND) + find_program(VALGRIND valgrind) + if(NOT VALGRIND) + message(FATAL_ERROR "`valgrind' not found when LUAJIT_USE_VALGRIND option " + "is enabled.") + endif() + if(NOT LUAJIT_USE_SYSMALLOC) message(WARNING "LUAJIT_USE_SYSMALLOC option is mandatory for Valgrind's memcheck tool" @@ -78,7 +84,6 @@ if(LUAJIT_USE_VALGRIND) " architectures.") endif() - find_program(VALGRIND valgrind) list(APPEND LUAJIT_TEST_VALGRIND_SUPP --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp =================================================================== > > > + list(APPEND LUAJIT_TEST_VALGRIND_SUPP > > + --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp > > + --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp > > + ) > > + set(LUAJIT_TEST_COMMAND > > + "${VALGRIND} --error-exitcode=1 " > > please add a comment why --error-exitcode is used. From a manual page: > >        --error-exitcode= [default: 0] >            Specifies an alternative exit code to return if Valgrind > reported any errors in the run. When set to the default value (zero), > the return value from Valgrind will always be the return value of the > process being simulated. When set to a nonzero value, that value is > returned instead, if Valgrind detects any errors. This is useful for > using Valgrind as part of an automated test suite, since it makes it > easy to detect test cases for which Valgrind has reported errors, just > by inspecting return codes. 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. 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}) set(LUAJIT_TEST_COMMAND - "${VALGRIND} --error-exitcode=1 " - "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") 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. > > > } > > > > #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */ -- Best regards, Sergey Kaplun