Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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:
> Hi, Sergey,
> 
> thanks for the patch! Please see comments below
> 
> On 11.12.2024 16:21, Sergey Kaplun wrote:
> > From: Maksim Tiushev<mandesero@gmail.com>
> >
> > 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
> s/valgrind/Valgrind/

Fixed, thanks!

> > command line (not at the building stage). By default, the suppression
> 
> There is also a config file .valgrindrc [1], why env variable is preffered?
> 
> [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts

It is more flexible. For example, we may easily test different
configurations locally for the single build without changing any
sources.

> 
> > 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 add a ticket url to commit message 
> (https://github.com/tarantool/tarantool/issues/10803).

It is already mentioned (see the line below).

> 
> >
> > Disabled due tarantool/tarantool#10803:
> >    - tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> >    - tarantool-tests/lj-512-profiler-hook-finalizers.test.lua

Updated the commit message regarding this test.

> >    - tarantool-tests/lj-726-profile-flush-close.test.lua
> >    - tarantool-tests/misclib-sysprof-lapi.test.lua
> >    - tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
> this test is missed in a ticket description, please add

Added.

> >
> > Timed out due to running under Valgrind:
> >    - tarantool-tests/gh-7745-oom-on-trace.test.lua
> >    - tarantool-tests/lj-1034-tabov-error-frame.test.lua
> >    - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test
> > [1]:https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts
> >
> > Part of tarantool/tarantool#3705

| 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.
|
| 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

> > ---
> >   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
> >

<snipped>

> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index 0db2dd8b..bda85ec1 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -69,6 +69,25 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
> >   )
> >   
> >   set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
> > +
> > +if(LUAJIT_USE_VALGRIND)
> > +  if (NOT LUAJIT_USE_SYSMALLOC)

Fixed indentation.

===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index bda85ec1..4590e065 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -71,7 +71,7 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
 set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 
 if(LUAJIT_USE_VALGRIND)
-  if (NOT LUAJIT_USE_SYSMALLOC)
+  if(NOT LUAJIT_USE_SYSMALLOC)
     message(WARNING
       "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"
===================================================================

> > +    message(WARNING
> > +      "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()
> > +
> > +  find_program(VALGRIND valgrind)
> 
> Please check that VALGRIND variable is not empty
> 
> and put this condition before "if (NOT LUAJIT_USE_SYSMALLOC)"

Added, now the error is raised in that case:
===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 4590e065..d9cab615 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -71,6 +71,12 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
 set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 
 if(LUAJIT_USE_VALGRIND)
+  find_program(VALGRIND valgrind)
+  if(NOT VALGRIND)
+    message(FATAL_ERROR "`valgrind' not found when LUAJIT_USE_VALGRIND option "
+                        "is enabled.")
+  endif()
+
   if(NOT LUAJIT_USE_SYSMALLOC)
     message(WARNING
       "LUAJIT_USE_SYSMALLOC option is mandatory for Valgrind's memcheck tool"
@@ -78,7 +84,6 @@ if(LUAJIT_USE_VALGRIND)
       " architectures.")
   endif()
 
-  find_program(VALGRIND valgrind)
   list(APPEND LUAJIT_TEST_VALGRIND_SUPP
     --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
     --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
===================================================================

> 
> > +  list(APPEND LUAJIT_TEST_VALGRIND_SUPP
> > +    --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
> > +    --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
> > +  )
> > +  set(LUAJIT_TEST_COMMAND
> > +    "${VALGRIND} --error-exitcode=1 "
> 
> please add a comment why --error-exitcode is used. From a manual page:
> 
>         --error-exitcode=<number> [default: 0]
>             Specifies an alternative exit code to return if Valgrind 
> reported any errors in the run. When set to the default value (zero), 
> the return value from Valgrind will always be the return value of the 
> process being simulated. When set to a nonzero value, that value is 
> returned instead, if Valgrind detects any errors. This is useful for 
> using Valgrind as part of an automated test suite, since it makes it 
> easy to detect test cases for which Valgrind has reported errors, just 
> by inspecting return codes.

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.
   set(LUAJIT_TEST_COMMAND
     "${VALGRIND} --error-exitcode=1 "
     "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
===================================================================

> 
> > +    "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
> 
> With "list(APPEND BENCHMARK_CMAKE_FLAGS "AA" "BB")" you don't need 
> trailing whitespaces.
> 
> Please replace "set()" with "list(APPEND ... )" like above.

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]: https://cmake.org/cmake/help/latest/command/list.html#prepend

> 
> > +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}
> 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})
   set(LUAJIT_TEST_COMMAND
-    "${VALGRIND} --error-exitcode=1 "
-    "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
+    "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
 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}
===================================================================

> > +      ${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 @@
> >    * *https://github.com/tarantool/tarantool/issues/9387
> >    */
> >   
> > +#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
> 
> 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.

The problem is that this test will run for too long, so it is just
easier to skip it.

> 
> >   }
> >   
> >   #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */

<snipped>

-- 
Best regards,
Sergey Kaplun