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 450ADE408F6; Thu, 12 Sep 2024 21:52:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 450ADE408F6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1726167137; bh=bYBNlm3M7JcZlg8i4gXuAyEfyMMLFUAOObT1nBHwyyo=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=k56xSFFmqs24NggKytkdBzECWtMX0wYqXqGnZnmOgfemKnJEYzhvXbaXOJ0OzLxV/ Pk60H3tq0LJVtFjoQJfDt1SHJb+buyZ640tmSK/FHBPFzPHxkqS9ma5+cMJgPVUm6c 8TkLhUjAKw9AAcYQ7LzV1bffCGhW9kMaCnfX1dEA= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (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 C8ABD63C9AC for ; Thu, 12 Sep 2024 21:52:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C8ABD63C9AC Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1soovO-00000003yHH-469Y; Thu, 12 Sep 2024 21:52:15 +0300 Content-Type: multipart/alternative; boundary="------------E03Q7G2qzbM4lF5kiJnM5O2D" Message-ID: <262398e1-121b-4d2e-a5bd-cc92bc56eef3@tarantool.org> Date: Thu, 12 Sep 2024 21:52:13 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: mandesero@gmail.com, tarantool-patches@dev.tarantool.org, skaplun@tarantool.org, m.kokryashkin@tarantool.org References: <20240912102153.163481-1-mandesero@gmail.com> <20240912102153.163481-2-mandesero@gmail.com> In-Reply-To: <20240912102153.163481-2-mandesero@gmail.com> X-Mailru-Src: smtp X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9350BA8AF986552F125C65B6EE9BE2695CE600C76FAA8F598182A05F5380850404C228DA9ACA6FE27936DE8A34A798C51A6D5EE0DB6E1EC8D06244248AE497E6DF74A38ABFC07646D96BB91BDF9958CC6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AD2F2D6F6013FF7FC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE76B1ACD3B92AC1F27EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B043BF0FB74779F36E2A77C4DB2385B2D8A8E08062544A71B19C630481E338D0DA471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE7C4D20244F70839727B076A6E789B0E97A8DF7F3B2552694AD5FFEEA1DED7F25D49FD398EE364050F9647ADFADE5905B12D242C3BD2E3F4C6C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F790063747074E5DE1517656EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A59F05476691851FDD5002B1117B3ED6969EC9734C38847191CCE9A60C8CB01D7C823CB91A9FED034534781492E4B8EEAD47A3109F1ACFD409BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742D3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF5ABCD861C434D83DA631A2A1227D37B4697CCA581ED990A7D7CD164936C729DB86260C86DC1AF146BA3E0C39EEA44CB3DE1E67B48AFE3F5E090246BFB57BBAE4437E340106E05A3AC226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojBIBNH8BG++docTKu696qYA== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F258845849A7EA5D6683ADBE95D4C5B18C328FC0A30AAA5A6E1FAC2167DF199C8C3DFBD9645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 luajit 1/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. --------------E03Q7G2qzbM4lF5kiJnM5O2D Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Maxim, thanks for the patch! See comments below. On 12.09.2024 13:21, mandesero--- via Tarantool-patches wrote: > From: Maksim Tiushev > > This patch adds the ability to run tests using Valgrind. Custom > Valgrind testing options can be set via the environment variable > `VALGRIND_OPTIONS`. By default, only the suppression file is set > to `src/lj.supp`. > --- > CMakeLists.txt | 5 +++++ > test/CMakeLists.txt | 24 +++++++++++++++++++++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/CMakeLists.txt b/CMakeLists.txt > index aa2103b3..854b3613 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -289,6 +289,11 @@ endif() > # ASan enabled. > option(LUAJIT_USE_ASAN "Build LuaJIT with AddressSanitizer" OFF) > if(LUAJIT_USE_ASAN) > + if(LUAJIT_USE_VALGRIND) > + message(FATAL_ERROR > + "AddressSanitizer and Valgrind cannot be used simultaneously." > + ) > + endif() > if(NOT LUAJIT_USE_SYSMALLOC) > message(WARNING > "Unfortunately, internal LuaJIT memory allocator is not instrumented yet," > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 0db2dd8b..280f0042 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -68,7 +68,29 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS > ${PROJECT_NAME}-codespell > ) > > -set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") > +if(LUAJIT_USE_VALGRIND) > + Excess empty line, please remove. > + if (NOT LUAJIT_USE_SYSMALLOC) > + message(FATAL_ERROR > + "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() > + > + if (NOT LUAJIT_ENABLE_GC64) > + message(FATAL_ERROR > + "LUAJIT_USE_SYSMALLOC cannot be enabled on x64 without GC64, since" > + " realloc usually doesn't return addresses in the right address range.") realloc -> realloc() to highlight that realloc is a function > + endif() > + > + set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp") I would add a prefix VALGRIND_ and put variable outside of condition. Feel free to ignore. > + set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}") > + > + set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") In the commit message, you say that VALGRIND_OPTIONS is an env variable. However, it is not: $ VALGRIND_OPTIONS=XXXX ctest -R test/tarantool-tests/arm64-ccall-fp-convention.test.lua --verbose 69: Test command: /bin/valgrind "--suppressions=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj.supp" "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/src/luajit" "-e" "dofile[[/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/luajit-test-init.lua]]" "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/arm64-ccall-fp-convention.test.lua" 69: Working Directory: /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/test/tarantool-tests 69: Environment variables: Also, replace "valgrind" in LUAJIT_TEST_COMMAND by ${VALGRIND_BIN} and put "find_program(VALGRIND valgrind)" before: --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -83,10 +83,11 @@ if(LUAJIT_USE_VALGRIND)        " realloc usually doesn't return addresses in the right address range.")    endif() +  find_program(VALGRIND valgrind)    set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")    set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}") -  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") +  set(LUAJIT_TEST_COMMAND "${VALGRIND} ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")  else()    set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")  endif() > +else() > + set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") I would add default set() with "LUAJIT_TEST_COMMAND" and remove a second branch: set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") if(LUAJIT_USE_VALGRIND) set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_COMMAND}") endif() > +endif() > + > separate_arguments(LUAJIT_TEST_COMMAND) > > set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") --------------E03Q7G2qzbM4lF5kiJnM5O2D Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Maxim,


thanks for the patch! See comments below.

On 12.09.2024 13:21, mandesero--- via Tarantool-patches wrote:
From: Maksim Tiushev <mandesero@gmail.com>

This patch adds the ability to run tests using Valgrind. Custom
Valgrind testing options can be set via the environment variable
`VALGRIND_OPTIONS`. By default, only the suppression file is set
to `src/lj.supp`.
---
 CMakeLists.txt      |  5 +++++
 test/CMakeLists.txt | 24 +++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index aa2103b3..854b3613 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -289,6 +289,11 @@ endif()
 # ASan enabled.
 option(LUAJIT_USE_ASAN "Build LuaJIT with AddressSanitizer" OFF)
 if(LUAJIT_USE_ASAN)
+  if(LUAJIT_USE_VALGRIND)
+    message(FATAL_ERROR
+      "AddressSanitizer and Valgrind cannot be used simultaneously."
+    )
+  endif()
   if(NOT LUAJIT_USE_SYSMALLOC)
     message(WARNING
       "Unfortunately, internal LuaJIT memory allocator is not instrumented yet,"
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 0db2dd8b..280f0042 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -68,7 +68,29 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
   ${PROJECT_NAME}-codespell
 )
 
-set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+if(LUAJIT_USE_VALGRIND)
+
Excess empty line, please remove.
+  if (NOT LUAJIT_USE_SYSMALLOC)
+    message(FATAL_ERROR
+      "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()
+
+  if (NOT LUAJIT_ENABLE_GC64)
+    message(FATAL_ERROR
+      "LUAJIT_USE_SYSMALLOC cannot be enabled on x64 without GC64, since"
+      " realloc usually doesn't return addresses in the right address range.")

realloc -> realloc()

to highlight that realloc is a function

+  endif()
+
+  set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")
I would add a prefix VALGRIND_ and put variable outside of condition. Feel free to ignore.
+  set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
+
+  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")

In the commit message, you say that VALGRIND_OPTIONS is an env variable.

However, it is not:


$ VALGRIND_OPTIONS=XXXX ctest -R test/tarantool-tests/arm64-ccall-fp-convention.test.lua --verbose         

<snipped>

69: Test command: /bin/valgrind "--suppressions=/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj.supp" "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/src/luajit" "-e" "dofile[[/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/luajit-test-init.lua]]" "/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/arm64-ccall-fp-convention.test.lua"        
69: Working Directory: /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/test/tarantool-tests                                
69: Environment variables: 

<snipped>


Also, replace "valgrind" in LUAJIT_TEST_COMMAND by ${VALGRIND_BIN} and

put "find_program(VALGRIND valgrind)" before:


--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -83,10 +83,11 @@ if(LUAJIT_USE_VALGRIND)
       " realloc usually doesn't return addresses in the right address range.")
   endif()
 
+  find_program(VALGRIND valgrind)
   set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")
   set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
 
-  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+  set(LUAJIT_TEST_COMMAND "${VALGRIND} ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 else()
   set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 endif()

+else()
+  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")

I would add default set() with "LUAJIT_TEST_COMMAND" and remove a second branch:


set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
if(LUAJIT_USE_VALGRIND)

<snipped>

set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_COMMAND}")

endif()

+endif()
+
 separate_arguments(LUAJIT_TEST_COMMAND)
 
 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
--------------E03Q7G2qzbM4lF5kiJnM5O2D--