<!DOCTYPE html>
<html data-lt-installed="true">
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body style="padding-bottom: 1px;">
    <p>Hi, Maxim,</p>
    <p><br>
    </p>
    <p>thanks for the patch! See comments below.<br>
    </p>
    <div class="moz-cite-prefix">On 12.09.2024 13:21, mandesero--- via
      Tarantool-patches wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:20240912102153.163481-2-mandesero@gmail.com">
      <pre class="moz-quote-pre" wrap="">From: Maksim Tiushev <a class="moz-txt-link-rfc2396E" href="mailto:mandesero@gmail.com"><mandesero@gmail.com></a>

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)
+</pre>
    </blockquote>
    Excess empty line, please remove.<br>
    <blockquote type="cite"
      cite="mid:20240912102153.163481-2-mandesero@gmail.com">
      <pre class="moz-quote-pre" wrap="">
+  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.")</pre>
    </blockquote>
    <p>realloc -> realloc()</p>
    <p>to highlight that realloc is a function<br>
    </p>
    <blockquote type="cite"
      cite="mid:20240912102153.163481-2-mandesero@gmail.com">
      <pre class="moz-quote-pre" wrap="">
+  endif()
+
+  set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")</pre>
    </blockquote>
    I would add a prefix VALGRIND_ and put variable outside of
    condition. Feel free to ignore.<br>
    <blockquote type="cite"
      cite="mid:20240912102153.163481-2-mandesero@gmail.com">
      <pre class="moz-quote-pre" wrap="">
+  set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
+
+  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")</pre>
    </blockquote>
    <p>In the commit message, you say that VALGRIND_OPTIONS is an env
      variable.</p>
    <p>However, it is not:</p>
    <p><br>
    </p>
    <p>$ VALGRIND_OPTIONS=XXXX ctest -R
      test/tarantool-tests/arm64-ccall-fp-convention.test.lua
      --verbose          <br>
    </p>
    <p><snipped><br>
    </p>
    <p>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"      
       <br>
      69: Working Directory:
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/test/tarantool-tests                              
       <br>
      69: Environment variables:  <br>
    </p>
    <p><snipped></p>
    <p><br>
    </p>
    <p>Also, replace "valgrind" in LUAJIT_TEST_COMMAND by
      ${VALGRIND_BIN} and</p>
    <p>put "find_program(VALGRIND valgrind)" before:</p>
    <p><br>
    </p>
    <p>--- a/test/CMakeLists.txt<br>
      +++ b/test/CMakeLists.txt<br>
      @@ -83,10 +83,11 @@ if(LUAJIT_USE_VALGRIND)<br>
             " realloc usually doesn't return addresses in the right
      address range.")<br>
         endif()<br>
       <br>
      +  find_program(VALGRIND valgrind)<br>
         set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")<br>
         set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE}
      $ENV{VALGRIND_OPTIONS}")<br>
       <br>
      -  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS}
      ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")<br>
      +  set(LUAJIT_TEST_COMMAND "${VALGRIND} ${VALGRIND_OPTIONS}
      ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")<br>
       else()<br>
         set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
      dofile[[${LUAJIT_TEST_INIT}]]")<br>
       endif()<br>
    </p>
    <blockquote type="cite"
      cite="mid:20240912102153.163481-2-mandesero@gmail.com">
      <pre class="moz-quote-pre" wrap="">
+else()
+  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")</pre>
    </blockquote>
    <p>I would add default set() with "LUAJIT_TEST_COMMAND" and remove a
      second branch:</p>
    <p><br>
    </p>
    <p>set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
      dofile[[${LUAJIT_TEST_INIT}]]")<br>
      if(LUAJIT_USE_VALGRIND)<br>
    </p>
    <p><snipped><br>
    </p>
    <p>set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS}
      ${LUAJIT_TEST_COMMAND}")<br>
    </p>
    <p>endif()<br>
    </p>
    <blockquote type="cite"
      cite="mid:20240912102153.163481-2-mandesero@gmail.com">
      <pre class="moz-quote-pre" wrap="">
+endif()
+
 separate_arguments(LUAJIT_TEST_COMMAND)
 
 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
</pre>
    </blockquote>
  </body>
  <lt-container></lt-container>
</html>