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 0C438DCBF5E; Tue, 17 Dec 2024 22:31:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0C438DCBF5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1734463888; bh=vSSYbthbhvl6OX7ObwOBkZRAIFs5rzmTzbU2tYh4ijo=; 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=bs0v6vqKpDnlwqez06girFlIPCoC8uSI8PxelGpAWxXES/G7apQfgFn08niON46no DoYcTIk+RM7g6lEnxVTXLXQpYO0AWEXwBc6ggrmfmGf8JUQaQIi0jkwkspazA+H8t6 F/KVkLYNluj0SZF7r0EoIgwLhuEr420/L1gJwjY8= Received: from send105.i.mail.ru (send105.i.mail.ru [89.221.237.200]) (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 4470D4363FD for ; Tue, 17 Dec 2024 22:31:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4470D4363FD Received: by exim-smtp-6bb57bdf67-6dgj4 with esmtpa (envelope-from ) id 1tNdHx-00000000AoU-1FvF; Tue, 17 Dec 2024 22:31:25 +0300 Content-Type: multipart/alternative; boundary="------------a0HZrlAfE0XVr9C0E0iDBQSW" Message-ID: <52d1b898-e87c-4fe2-a6e1-5b58afd8db13@tarantool.org> Date: Tue, 17 Dec 2024 22:31:25 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: Maksim Tiushev , tarantool-patches@dev.tarantool.org References: <7f8dea571105b6780b05d633df0d75b8c1a8a3c1.1733909411.git.skaplun@tarantool.org> <0807ccb5-8d41-49a9-a57e-3ab9c6c705f7@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9B1D8B0AA3C5934EEACC8CC571658AAF417B692E3FA1FF7F5182A05F53808504055AD5CEC8FFAE4E63DE06ABAFEAF67052EB915F5925C4C5F347F49F1BB0FA955D19A5A69C2E3A454 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE766DBE83FD69AB645EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006376F127A835590024F8F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB5533756690B632A54461F0DDED8FA9BFC0A9068383DCBFA77B9ED11D13E7EC63C067032E389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0C26CFBAC0749D213D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B2EE5AD8F952D28FBA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC9C5A33288E4BA8F93AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F7900637B4E5EBAFC6973958D81D268191BDAD3D3666184CF4C3C14F3FC91FA280E0CE3D1A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C317B107DEF921CE79089D37D7C0E48F6C8AA50765F7900637D8AFD95DD44FDE86EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5A3A755CD88561D575002B1117B3ED696C5C359C6EA229098ED71F038FC046993823CB91A9FED034534781492E4B8EEAD09F854029C6BD0DABDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D348F1757F8DFBC5C338E57F13230FE6D191227EFF8506A81AAE392DE9FD33D796818428691ABB0CEF21D7E09C32AA3244CB290A247AD86B80E77DD89D51EBB774227B0279767E39D2CEA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojrmbv0RPnqfR8k3rPA1Li0w== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D61405C58B1D65643D8E1C591814E25D11F9F887FA47FC7736A230152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------a0HZrlAfE0XVr9C0E0iDBQSW Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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: >>> >> >>> | 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. >> please replace dot with colon, because below the enumeration of disabled >> tests > Replaced. > >>> | >>> | 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 >>> > > >>> ) >>> + # 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}") > > >>>>> 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) > > --------------a0HZrlAfE0XVr9C0E0iDBQSW Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

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>



        

      

      

    

  
  


--------------a0HZrlAfE0XVr9C0E0iDBQSW--