<!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>Hello,</p>
<p>Thanks for fixes!</p>
<p>LGTM after fixing minor comments below.<br>
</p>
<div class="moz-cite-prefix">On 16.12.2024 19:40, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<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>
<br>
</blockquote>
<snipped><br>
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<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>
please replace dot with colon, because below the enumeration of
disabled tests<br>
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<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 type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
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
</pre>
</blockquote>
</blockquote>
</blockquote>
<p><br>
</p>
<p><snipped><br>
</p>
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
because "when set to the default value (zero), <br>
the return value from Valgrind will always be the return value of
the <br>
process being simulated.", I believe it is an important thing, and
it's worth to add to comment.<br>
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<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">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
With "list(APPEND BENCHMARK_CMAKE_FLAGS "AA" "BB")" you don't need
trailing whitespaces.
Please replace "set()" with "list(APPEND ... )" like above.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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]: <a class="moz-txt-link-freetext" href="https://cmake.org/cmake/help/latest/command/list.html#prepend">https://cmake.org/cmake/help/latest/command/list.html#prepend</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+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}
</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>
bad indentation<br>
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<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>
bad indentation
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<pre class="moz-quote-pre" wrap="">
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}
===================================================================
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ ${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 @@
* *<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/9387">https://github.com/tarantool/tarantool/issues/9387</a>
*/
+#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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
The problem is that this test will run for too long, so it is just
easier to skip it.
</pre>
</blockquote>
Okay.<br>
<br>
<blockquote type="cite" cite="mid:Z2BYDOdYJwy6Vupx@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> }
#else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>