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

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

Hi, Maksim!
Thanks for the patch!
Please consider my comments below.

On 12.09.24, mandesero@gmail.com 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

<snipped>

> 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)
> +
> +  if (NOT LUAJIT_USE_SYSMALLOC)
> +    message(FATAL_ERROR

It is better to be a warning by analog with the ASAN build.

> +      "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)

Why do we need this condition?

> +    message(FATAL_ERROR

Ditto.

> +      "LUAJIT_USE_SYSMALLOC cannot be enabled on x64 without GC64, since"

I don't get this comment. Why valgrind depends on x64, and GC64 as well?
I suppose we can drop the comment and the check above.

> +      " realloc usually doesn't return addresses in the right address range.")
> +  endif()

> +
> +  set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")

Please use `LUAJIT_SOURCE_DIR` instead.

> +  set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")

Line length is more than 80 symbols.

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

Since there are no skip conditions for this build some tests will fail
or never finish on this commit. It is better to squash this commit with
the last one.

It is better to use a name LUAJIT_TEST_VALGRIND_OPTS to be consistent
with other names (LUAJIT_TEST_BINARY LUAJIT_TEST_INIT).

Line length is more than 80 symbols.

> +else()
> +  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
> +endif()
> +
>  separate_arguments(LUAJIT_TEST_COMMAND)
>  
>  set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun