<!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>Thanks for the fixes! LGTM<br>
</p>
<p>BTW, I've discovered that CMake has integration with memory
checking tools, see [1][2].</p>
<p><br>
</p>
<p>[1]:
<a class="moz-txt-link-freetext" href="https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-memcheck-step">https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-memcheck-step</a></p>
<p>[2]:
<a class="moz-txt-link-freetext" href="https://cmake.org/cmake/help/latest/variable/CTEST_MEMORYCHECK_TYPE.html#variable:CTEST_MEMORYCHECK_TYPE">https://cmake.org/cmake/help/latest/variable/CTEST_MEMORYCHECK_TYPE.html#variable:CTEST_MEMORYCHECK_TYPE</a><br>
</p>
<div class="moz-cite-prefix">On 17.12.2024 15:17, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z2Fr7JYJNvJGQO_8@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the comments!
Fixed them and force-pushed the branch.
On 17.12.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hello,
Thanks for fixes!
LGTM after fixing minor comments below.
On 16.12.2024 19:40, Sergey Kaplun wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">| 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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">please replace dot with colon, because below the enumeration of disabled
tests
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Replaced.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">|
| 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]:<a class="moz-txt-link-freetext" href="https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts">https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts</a>
|
| Part of tarantool/tarantool#3705
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> )
+ # Set the exit code to non-zero to automatically detect
+ # failing tests.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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
===================================================================
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> set(LUAJIT_TEST_COMMAND
"${VALGRIND} --error-exitcode=1 "
"${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
===================================================================
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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}
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">The same already defined in test/CMakeLists.txt, could you reuse already
defined value?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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})
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">bad indentation
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> set(LUAJIT_TEST_COMMAND
- "${VALGRIND} --error-exitcode=1 "
- "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
+ "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">bad indentation
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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)
===================================================================
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> endif()
separate_arguments(LUAJIT_TEST_COMMAND)
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>