[Tarantool-patches] [PATCH v2 luajit 0/3] Enable running tests with Valgrind, add CI Valgrind testing workflow

[Tarantool-patches] [PATCH v2 luajit 0/3] Enable running tests with Valgrind, add CI Valgrind testing workflow

[mandesero--- via Tarantool-patches]

From: Maksim Tyushev <mandesero@gmail.com>

This patchset enables running LuaJIT tests under Valgrind, with the
option to set custom Valgrind options using the `VALGRIND_OPTIONS`
environment variable. It also introduces a Valgrind testing workflow
with three scenarios: full checks, and two memory checks without leak
detection, where memory is filled with `0x00` and `0xff`.

Some tests consistently fail under Valgrind due to various reasons,
such as SIGPROF, timeouts, or flaky behavior. These tests are
disabled when `LUAJIT_USE_VALGRIND=ON`.

Branch: https://github.com/tarantool/luajit/tree/mandesero/lj-3705-turn-off-strcmp-opt-in-debug
Issue: https://github.com/tarantool/tarantool/issues/3705

Maksim Tiushev (3):
  cmake: run tests with Valgrind
  ci: add Valgrind testing workflow
  test: disable tests failing with Valgrind

 .github/actions/setup-valgrind/README.md      | 12 +++
 .github/actions/setup-valgrind/action.yml     | 19 ++++
 .github/workflows/valgrind-testing.yaml       | 91 +++++++++++++++++++
 CMakeLists.txt                                |  5 +
 test/CMakeLists.txt                           | 24 ++++-
 test/tarantool-tests/CMakeLists.txt           |  3 +-
 ...4-add-proto-trace-sysprof-default.test.lua |  1 +
 .../gh-7745-oom-on-trace.test.lua             |  1 +
 .../lj-1034-tabov-error-frame.test.lua        |  1 +
 .../lj-512-profiler-hook-finalizers.test.lua  |  4 +-
 .../lj-726-profile-flush-close.test.lua       |  4 +-
 .../misclib-sysprof-lapi.test.lua             |  1 +
 12 files changed, 162 insertions(+), 4 deletions(-)
 create mode 100644 .github/actions/setup-valgrind/README.md
 create mode 100644 .github/actions/setup-valgrind/action.yml
 create mode 100644 .github/workflows/valgrind-testing.yaml

-- 
2.34.1

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

[mandesero--- via Tarantool-patches]

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
@@ -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)
+
+  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.")
+  endif()
+
+  set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")
+  set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
+
+  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+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

[Tarantool-patches] [PATCH v2 luajit 2/3] ci: add Valgrind testing workflow

[mandesero--- via Tarantool-patches]

From: Maksim Tiushev <mandesero@gmail.com>

This patch adds CI testing with Valgrind in three scenarios:
  - Full checks enabled.
  - No leak checks, with memory fill set to `--malloc-fill=0x00`
    and `--free-fill=0x00`.
  - No leak checks, with memory fill set to `--malloc-fill=0xFF`
    and `--free-fill=0xFF`.
---
 .github/actions/setup-valgrind/README.md  | 12 +++
 .github/actions/setup-valgrind/action.yml | 19 +++++
 .github/workflows/valgrind-testing.yaml   | 91 +++++++++++++++++++++++
 3 files changed, 122 insertions(+)
 create mode 100644 .github/actions/setup-valgrind/README.md
 create mode 100644 .github/actions/setup-valgrind/action.yml
 create mode 100644 .github/workflows/valgrind-testing.yaml

diff --git a/.github/actions/setup-valgrind/README.md b/.github/actions/setup-valgrind/README.md
new file mode 100644
index 00000000..fabd5af1
--- /dev/null
+++ b/.github/actions/setup-valgrind/README.md
@@ -0,0 +1,12 @@
+# Setup environment for Valgrind on Linux
+
+Action setups the environment on Linux runners (install requirements, setup the
+workflow environment, etc) for testing with Valgrind.
+
+## How to use Github Action from Github workflow
+
+Add the following code to the running steps before LuaJIT configuration:
+```
+- uses: ./.github/actions/setup-linux
+  if: ${{ matrix.OS == 'Linux' }}
+```
diff --git a/.github/actions/setup-valgrind/action.yml b/.github/actions/setup-valgrind/action.yml
new file mode 100644
index 00000000..4f6cfba4
--- /dev/null
+++ b/.github/actions/setup-valgrind/action.yml
@@ -0,0 +1,19 @@
+name: Setup CI environment for Valgrind on Linux
+description: Common part to tweak Linux CI runner environment
+runs:
+  using: composite
+  steps:
+    - name: Setup CI environment
+      uses: ./.github/actions/setup
+    - name: Set CMAKE_BUILD_PARALLEL_LEVEL
+      run: |
+        # Set CMAKE_BUILD_PARALLEL_LEVEL environment variable to
+        # limit the number of parallel jobs for build/test step.
+        NPROC=$(nproc)
+        echo CMAKE_BUILD_PARALLEL_LEVEL=$(($NPROC + 1)) | tee -a $GITHUB_ENV
+      shell: bash
+    - name: Install build and test dependencies
+      run: |
+        apt -y update
+        apt -y install cmake gcc make ninja-build perl valgrind
+      shell: bash
diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
new file mode 100644
index 00000000..693799ea
--- /dev/null
+++ b/.github/workflows/valgrind-testing.yaml
@@ -0,0 +1,91 @@
+name: Valgrind testing
+
+on:
+  push:
+    branches-ignore:
+      - '**-notest'
+      - 'upstream-**'
+    tags-ignore:
+      - '**'
+
+concurrency:
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool/release/2.11,
+  # tarantool/release/2.10, etc) workflow runs are never canceled.
+  #
+  # We use a trick here: define the concurrency group as 'workflow
+  # run ID' + # 'workflow run attempt' because it is a unique
+  # combination for any run. So it effectively discards grouping.
+  #
+  # XXX: we cannot use `github.sha` as a unique identifier because
+  # pushing a tag may cancel a run that works on a branch push
+  # event.
+  group: ${{ startsWith(github.ref, 'refs/heads/tarantool/')
+    && format('{0}-{1}', github.run_id, github.run_attempt)
+    || format('{0}-{1}', github.workflow, github.ref) }}
+  cancel-in-progress: true
+
+jobs:
+  test-valgrind:
+    strategy:
+      fail-fast: false
+      matrix:
+        # XXX: Let's start with only Linux/x86_64
+        BUILDTYPE: [Debug, Release]
+        VALGRIND_SCENARIO: [full, malloc-free-fill-0x00, malloc-free-fill-0xff]
+        include:
+          - BUILDTYPE: Debug
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
+          - BUILDTYPE: Release
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
+          - VALGRIND_SCENARIO: full
+            VALGRIND_OPTIONS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose
+            JOB_POSTFIX: "leak-check: full"
+          - VALGRIND_SCENARIO: malloc-free-fill-0x00
+            VALGRIND_OPTIONS: --leak-check=no --malloc-fill=0x00 --free-fill=0x00
+            JOB_POSTFIX: "malloc/free-fill: 0x00"
+          - VALGRIND_SCENARIO: malloc-free-fill-0xff
+            VALGRIND_OPTIONS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
+            JOB_POSTFIX: "malloc/free-fill: 0xff"
+    runs-on: [self-hosted, regular, Linux, x86_64]
+    name: >
+      LuaJIT with Valgrind (Linux/x86_64)
+      ${{ matrix.BUILDTYPE }}
+      CC: gcc
+      GC64:ON SYSMALLOC:ON
+      ${{ matrix.JOB_POSTFIX }}
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: setup Linux for Valgrind
+        uses: ./.github/actions/setup-valgrind
+      - name: configure
+        # XXX: LuaJIT configuration requires a couple of tweaks:
+        # LUAJIT_USE_SYSMALLOC=ON: Unfortunately, internal LuaJIT
+        #   memory allocator is not instrumented yet, so to find
+        #   any memory errors it's better to build LuaJIT with
+        #   system provided memory allocator (i.e. run CMake
+        #   configuration phase with -DLUAJIT_USE_SYSMALLOC=ON).
+        #   For more info, see root CMakeLists.txt.
+        # LUAJIT_ENABLE_GC64=ON: LUAJIT_USE_SYSMALLOC cannot be
+        #   enabled on x64 without GC64, since realloc usually
+        #   doesn't return addresses in the right address range.
+        #   For more info, see root CMakeLists.txt.
+        env:
+          VALGRIND_OPTIONS: ${{ matrix.VALGRIND_OPTIONS }}
+        run: >
+          cmake -S . -B ${{ env.BUILDDIR }}
+          -G Ninja
+          ${{ matrix.CMAKEFLAGS }}
+          -DLUAJIT_USE_VALGRIND=ON
+          -DLUAJIT_ENABLE_GC64=ON
+          -DLUAJIT_USE_SYSMALLOC=ON
+      - name: build
+        run: cmake --build . --parallel
+        working-directory: ${{ env.BUILDDIR }}
+      - name: test
+        run: cmake --build . --parallel --target LuaJIT-test
+        working-directory: ${{ env.BUILDDIR }}
-- 
2.34.1

[Tarantool-patches] [PATCH v2 luajit 3/3] test: disable tests failing with Valgrind

[mandesero--- via Tarantool-patches]

From: Maksim Tiushev <mandesero@gmail.com>

This patch disables the following tests when running with Valgrind due to
failures:

For SIGPROF:
  - test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
  - test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
  - test/tarantool-tests/lj-726-profile-flush-close.test.lua

For TIMEOUT:
  - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
  - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua

For FLAKY:
  - test/tarantool-tests/misclib-sysprof-lapi.test.lua
---
 test/tarantool-tests/CMakeLists.txt                           | 3 ++-
 .../gh-7264-add-proto-trace-sysprof-default.test.lua          | 1 +
 test/tarantool-tests/gh-7745-oom-on-trace.test.lua            | 1 +
 test/tarantool-tests/lj-1034-tabov-error-frame.test.lua       | 1 +
 test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua | 4 +++-
 test/tarantool-tests/lj-726-profile-flush-close.test.lua      | 4 +++-
 test/tarantool-tests/misclib-sysprof-lapi.test.lua            | 1 +
 7 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index e3750bf3..2becebb7 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -130,7 +130,8 @@ foreach(test_path ${tests})
     # LUA_CPATH and LD_LIBRARY_PATH variables and also
     # dependencies list with libraries are set in scope of
     # BuildTestLib macro.
-    ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}"
+    ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE};\
+LJ_USE_VALGRIND=${LUAJIT_USE_VALGRIND}"
     LABELS ${TEST_SUITE_NAME}
     DEPENDS tarantool-tests-deps
   )
diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
index c1d68e3c..f700b414 100644
--- a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
+++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
@@ -4,6 +4,7 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
   ['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
                                                jit.arch ~= 'x64',
   ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
+  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
 })
 
 test:plan(2)
diff --git a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
index fe320251..b4ab8872 100644
--- a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
+++ b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
@@ -6,6 +6,7 @@ local test = tap.test('OOM on trace'):skipcond({
                                                    (jit.os == 'OSX'),
   ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
   ['Test requires JIT enabled'] = not jit.status(),
+  ['Disabled with Valgrind (Timeout)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
 })
 
 test:plan(1)
diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
index 0e23fdb2..5e75973b 100644
--- a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
+++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
@@ -4,6 +4,7 @@ local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
   ['Test requires GC64 mode enabled'] = not ffi.abi('gc64'),
   ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
+  ['Disabled with Valgrind (Timeout)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
 })
 
 -- XXX: The test for the problem uses the table of GC
diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
index f02bd05f..fc2f43b7 100644
--- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
+++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
@@ -1,7 +1,9 @@
 local tap = require('tap')
 local profile = require('jit.profile')
 
-local test = tap.test('lj-512-profiler-hook-finalizers')
+local test = tap.test('lj-512-profiler-hook-finalizers'):skipcond({
+  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
+})
 test:plan(1)
 
 -- Sampling interval in ms.
diff --git a/test/tarantool-tests/lj-726-profile-flush-close.test.lua b/test/tarantool-tests/lj-726-profile-flush-close.test.lua
index 36cca43d..6d09658f 100644
--- a/test/tarantool-tests/lj-726-profile-flush-close.test.lua
+++ b/test/tarantool-tests/lj-726-profile-flush-close.test.lua
@@ -1,6 +1,8 @@
 local tap = require('tap')
 
-local test = tap.test('lj-726-profile-flush-close')
+local test = tap.test('lj-726-profile-flush-close'):skipcond({
+  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
+})
 test:plan(1)
 
 local TEST_FILE = 'lj-726-profile-flush-close.profile'
diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
index fdaed46a..79884710 100644
--- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
@@ -3,6 +3,7 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
   ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
                                                jit.arch ~= "x64",
   ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
+  ['Disabled with Valgrind (Flaky)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
 })
 
 test:plan(19)
-- 
2.34.1

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

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4575 bytes --]

Hi, Maxim,


thanks for the patch! See comments below.

On 12.09.2024 13:21, mandesero--- via Tarantool-patches 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
> @@ -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)
> +
Excess empty line, please remove.
> +  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.")

realloc -> realloc()

to highlight that realloc is a function

> +  endif()
> +
> +  set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")
I would add a prefix VALGRIND_ and put variable outside of condition. 
Feel free to ignore.
> +  set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} $ENV{VALGRIND_OPTIONS}")
> +
> +  set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")

In the commit message, you say that VALGRIND_OPTIONS is an env variable.

However, it is not:


$ VALGRIND_OPTIONS=XXXX ctest -R 
test/tarantool-tests/arm64-ccall-fp-convention.test.lua --verbose

<snipped>

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" 

69: Working Directory: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/test/tarantool-tests 

69: Environment variables:

<snipped>


Also, replace "valgrind" in LUAJIT_TEST_COMMAND by ${VALGRIND_BIN} and

put "find_program(VALGRIND valgrind)" before:


--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -83,10 +83,11 @@ if(LUAJIT_USE_VALGRIND)
        " realloc usually doesn't return addresses in the right address 
range.")
    endif()

+  find_program(VALGRIND valgrind)
    set(SUPPRESSIONS_FILE "${CMAKE_SOURCE_DIR}/src/lj.supp")
    set(VALGRIND_OPTIONS "--suppressions=${SUPPRESSIONS_FILE} 
$ENV{VALGRIND_OPTIONS}")

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

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

I would add default set() with "LUAJIT_TEST_COMMAND" and remove a second 
branch:


set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e 
dofile[[${LUAJIT_TEST_INIT}]]")
if(LUAJIT_USE_VALGRIND)

<snipped>

set(LUAJIT_TEST_COMMAND "valgrind ${VALGRIND_OPTIONS} 
${LUAJIT_TEST_COMMAND}")

endif()

> +endif()
> +
>   separate_arguments(LUAJIT_TEST_COMMAND)
>   
>   set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")

[-- Attachment #2: Type: text/html, Size: 6479 bytes --]

Re: [Tarantool-patches] [PATCH v2 luajit 2/3] ci: add Valgrind testing workflow

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 6816 bytes --]

Hi, Maxim,

thanks for the patch! See comments below:

On 12.09.2024 13:21, mandesero--- via Tarantool-patches wrote:
> From: Maksim Tiushev<mandesero@gmail.com>
>
> This patch adds CI testing with Valgrind in three scenarios:
Patch adds a *workflow* with testing under Valgrind...
>    - Full checks enabled.
>    - No leak checks, with memory fill set to `--malloc-fill=0x00`
>      and `--free-fill=0x00`.
>    - No leak checks, with memory fill set to `--malloc-fill=0xFF`
>      and `--free-fill=0xFF`.
> ---
>   .github/actions/setup-valgrind/README.md  | 12 +++
>   .github/actions/setup-valgrind/action.yml | 19 +++++
>   .github/workflows/valgrind-testing.yaml   | 91 +++++++++++++++++++++++
>   3 files changed, 122 insertions(+)
>   create mode 100644 .github/actions/setup-valgrind/README.md
>   create mode 100644 .github/actions/setup-valgrind/action.yml
>   create mode 100644 .github/workflows/valgrind-testing.yaml
>
> diff --git a/.github/actions/setup-valgrind/README.md b/.github/actions/setup-valgrind/README.md
> new file mode 100644
> index 00000000..fabd5af1
> --- /dev/null
> +++ b/.github/actions/setup-valgrind/README.md
> @@ -0,0 +1,12 @@
> +# Setup environment for Valgrind on Linux
> +
> +Action setups the environment on Linux runners (install requirements, setup the
> +workflow environment, etc) for testing with Valgrind.
> +
> +## How to use Github Action from Github workflow
> +
> +Add the following code to the running steps before LuaJIT configuration:
> +```
> +- uses: ./.github/actions/setup-linux
s/setup-linux/setup-valgrind/
> +  if: ${{ matrix.OS == 'Linux' }}
> +```
> diff --git a/.github/actions/setup-valgrind/action.yml b/.github/actions/setup-valgrind/action.yml
> new file mode 100644
> index 00000000..4f6cfba4
> --- /dev/null
> +++ b/.github/actions/setup-valgrind/action.yml
> @@ -0,0 +1,19 @@
> +name: Setup CI environment for Valgrind on Linux
> +description: Common part to tweak Linux CI runner environment
copy-pasted?
> +runs:
> +  using: composite
> +  steps:
> +    - name: Setup CI environment
> +      uses: ./.github/actions/setup
> +    - name: Set CMAKE_BUILD_PARALLEL_LEVEL
> +      run: |
> +        # Set CMAKE_BUILD_PARALLEL_LEVEL environment variable to
> +        # limit the number of parallel jobs for build/test step.
> +        NPROC=$(nproc)
> +        echo CMAKE_BUILD_PARALLEL_LEVEL=$(($NPROC + 1)) | tee -a $GITHUB_ENV
Do you really need this for setup Valgrind?
> +      shell: bash
> +    - name: Install build and test dependencies
> +      run: |
> +        apt -y update
> +        apt -y install cmake gcc make ninja-build perl valgrind
Why do you need gcc/cmake/make etc if an action about Valgrind setup?
> +      shell: bash
> diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
> new file mode 100644
> index 00000000..693799ea
> --- /dev/null
> +++ b/.github/workflows/valgrind-testing.yaml
> @@ -0,0 +1,91 @@
> +name: Valgrind testing
> +
> +on:
> +  push:
> +    branches-ignore:
> +      - '**-notest'
> +      - 'upstream-**'
> +    tags-ignore:
> +      - '**'
> +
> +concurrency:
> +  # An update of a developer branch cancels the previously
> +  # scheduled workflow run for this branch. However, the default
> +  # branch, and long-term branch (tarantool/release/2.11,
> +  # tarantool/release/2.10, etc) workflow runs are never canceled.
> +  #
> +  # We use a trick here: define the concurrency group as 'workflow
> +  # run ID' + # 'workflow run attempt' because it is a unique
> +  # combination for any run. So it effectively discards grouping.
> +  #
> +  # XXX: we cannot use `github.sha` as a unique identifier because
> +  # pushing a tag may cancel a run that works on a branch push
> +  # event.
> +  group: ${{ startsWith(github.ref, 'refs/heads/tarantool/')
> +    && format('{0}-{1}', github.run_id, github.run_attempt)
> +    || format('{0}-{1}', github.workflow, github.ref) }}
> +  cancel-in-progress: true
> +
> +jobs:
> +  test-valgrind:
> +    strategy:
> +      fail-fast: false
> +      matrix:
> +        # XXX: Let's start with only Linux/x86_64
> +        BUILDTYPE: [Debug, Release]
> +        VALGRIND_SCENARIO: [full, malloc-free-fill-0x00, malloc-free-fill-0xff]
> +        include:
> +          - BUILDTYPE: Debug
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> +          - BUILDTYPE: Release
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> +          - VALGRIND_SCENARIO: full
> +            VALGRIND_OPTIONS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose
> +            JOB_POSTFIX: "leak-check: full"
> +          - VALGRIND_SCENARIO: malloc-free-fill-0x00
> +            VALGRIND_OPTIONS: --leak-check=no --malloc-fill=0x00 --free-fill=0x00
> +            JOB_POSTFIX: "malloc/free-fill: 0x00"
> +          - VALGRIND_SCENARIO: malloc-free-fill-0xff
> +            VALGRIND_OPTIONS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
> +            JOB_POSTFIX: "malloc/free-fill: 0xff"
> +    runs-on: [self-hosted, regular, Linux, x86_64]
> +    name: >
> +      LuaJIT with Valgrind (Linux/x86_64)
> +      ${{ matrix.BUILDTYPE }}
> +      CC: gcc
> +      GC64:ON SYSMALLOC:ON
> +      ${{ matrix.JOB_POSTFIX }}
> +    steps:
> +      - uses: actions/checkout@v4
> +        with:
> +          fetch-depth: 0
> +          submodules: recursive
> +      - name: setup Linux for Valgrind
> +        uses: ./.github/actions/setup-valgrind
> +      - name: configure
> +        # XXX: LuaJIT configuration requires a couple of tweaks:
> +        # LUAJIT_USE_SYSMALLOC=ON: Unfortunately, internal LuaJIT
> +        #   memory allocator is not instrumented yet, so to find
> +        #   any memory errors it's better to build LuaJIT with
> +        #   system provided memory allocator (i.e. run CMake
> +        #   configuration phase with -DLUAJIT_USE_SYSMALLOC=ON).
> +        #   For more info, see root CMakeLists.txt.
> +        # LUAJIT_ENABLE_GC64=ON: LUAJIT_USE_SYSMALLOC cannot be
> +        #   enabled on x64 without GC64, since realloc usually
> +        #   doesn't return addresses in the right address range.
> +        #   For more info, see root CMakeLists.txt.
> +        env:
> +          VALGRIND_OPTIONS: ${{ matrix.VALGRIND_OPTIONS }}
> +        run: >
> +          cmake -S . -B ${{ env.BUILDDIR }}
> +          -G Ninja
> +          ${{ matrix.CMAKEFLAGS }}
> +          -DLUAJIT_USE_VALGRIND=ON
> +          -DLUAJIT_ENABLE_GC64=ON
> +          -DLUAJIT_USE_SYSMALLOC=ON
> +      - name: build
> +        run: cmake --build . --parallel
> +        working-directory: ${{ env.BUILDDIR }}
> +      - name: test
> +        run: cmake --build . --parallel --target LuaJIT-test
> +        working-directory: ${{ env.BUILDDIR }}

[-- Attachment #2: Type: text/html, Size: 7875 bytes --]

Re: [Tarantool-patches] [PATCH v2 luajit 3/3] test: disable tests failing with Valgrind

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 6142 bytes --]

Hi, Maxim,

thanks for the patch! See comments below.

On 12.09.2024 13:21, mandesero--- via Tarantool-patches wrote:
> From: Maksim Tiushev<mandesero@gmail.com>
>
> This patch disables the following tests when running with Valgrind due to
> failures:
>
> For SIGPROF:
s/SIGPROF/sysprof/
>    - test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
>    - test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>    - test/tarantool-tests/lj-726-profile-flush-close.test.lua
>
> For TIMEOUT:
>    - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
>    - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
>
> For FLAKY:
>    - test/tarantool-tests/misclib-sysprof-lapi.test.lua
it is a sysprof test, why it is in a separate category?
> ---
>   test/tarantool-tests/CMakeLists.txt                           | 3 ++-
>   .../gh-7264-add-proto-trace-sysprof-default.test.lua          | 1 +
>   test/tarantool-tests/gh-7745-oom-on-trace.test.lua            | 1 +
>   test/tarantool-tests/lj-1034-tabov-error-frame.test.lua       | 1 +
>   test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua | 4 +++-
>   test/tarantool-tests/lj-726-profile-flush-close.test.lua      | 4 +++-
>   test/tarantool-tests/misclib-sysprof-lapi.test.lua            | 1 +
>   7 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index e3750bf3..2becebb7 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -130,7 +130,8 @@ foreach(test_path ${tests})
>       # LUA_CPATH and LD_LIBRARY_PATH variables and also
>       # dependencies list with libraries are set in scope of
>       # BuildTestLib macro.
> -    ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}"
> +    ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE};\
> +LJ_USE_VALGRIND=${LUAJIT_USE_VALGRIND}"
>       LABELS ${TEST_SUITE_NAME}
>       DEPENDS tarantool-tests-deps
>     )
> diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> index c1d68e3c..f700b414 100644
> --- a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> +++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> @@ -4,6 +4,7 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
>     ['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
>                                                  jit.arch ~= 'x64',
>     ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
> +  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   test:plan(2)
> diff --git a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> index fe320251..b4ab8872 100644
> --- a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> +++ b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> @@ -6,6 +6,7 @@ local test = tap.test('OOM on trace'):skipcond({
>                                                      (jit.os == 'OSX'),
>     ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
>     ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled with Valgrind (Timeout)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   test:plan(1)
> diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> index 0e23fdb2..5e75973b 100644
> --- a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> +++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> @@ -4,6 +4,7 @@ local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
>     ['Test requires JIT enabled'] = not jit.status(),
>     ['Test requires GC64 mode enabled'] = not ffi.abi('gc64'),
>     ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
> +  ['Disabled with Valgrind (Timeout)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   -- XXX: The test for the problem uses the table of GC
> diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
> index f02bd05f..fc2f43b7 100644
> --- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
> +++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
> @@ -1,7 +1,9 @@
>   local tap = require('tap')
>   local profile = require('jit.profile')
>   
> -local test = tap.test('lj-512-profiler-hook-finalizers')
> +local test = tap.test('lj-512-profiler-hook-finalizers'):skipcond({
> +  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
> +})
>   test:plan(1)
>   
>   -- Sampling interval in ms.
> diff --git a/test/tarantool-tests/lj-726-profile-flush-close.test.lua b/test/tarantool-tests/lj-726-profile-flush-close.test.lua
> index 36cca43d..6d09658f 100644
> --- a/test/tarantool-tests/lj-726-profile-flush-close.test.lua
> +++ b/test/tarantool-tests/lj-726-profile-flush-close.test.lua
> @@ -1,6 +1,8 @@
>   local tap = require('tap')
>   
> -local test = tap.test('lj-726-profile-flush-close')
> +local test = tap.test('lj-726-profile-flush-close'):skipcond({
> +  ['Disabled with Valgrind (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
> +})
>   test:plan(1)
>   
>   local TEST_FILE = 'lj-726-profile-flush-close.profile'
> diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> index fdaed46a..79884710 100644
> --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> @@ -3,6 +3,7 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
>     ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
>                                                  jit.arch ~= "x64",
>     ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
> +  ['Disabled with Valgrind (Flaky)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   test:plan(19)

[-- Attachment #2: Type: text/html, Size: 6731 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH v2 luajit 2/3] ci: add Valgrind testing workflow

[Sergey Kaplun via Tarantool-patches]

Hi, Maksim!
Thanks for the patch!
Please see my comment below.

On 12.09.24, mandesero@gmail.com wrote:
> From: Maksim Tiushev <mandesero@gmail.com>

<snipped>

> +    runs-on: [self-hosted, regular, Linux, x86_64]

It is worth checking the arm64 architecture too.
It is useful for backporting patches like the following:

* 003f6856 ("FFI: Don't load PC from non-function object in FFI
            continuation.") [1],
* 68ffbd31 ("FFI: Don't load PC from non-function object in FFI
            continuation.") [2].

See the corresponding issue [3].

So, for this patchset, you should backport the aforementioned patches
first and add the valgrind check after.

> +    name: >

<snipped>

> -- 
> 2.34.1
> 

[1]: https://github.com/LuaJIT/LuaJIT/commit/003f6856
[2]: https://github.com/LuaJIT/LuaJIT/commit/68ffbd31
[3]: https://github.com/LuaJIT/LuaJIT/issues/743


-- 
Best regards,
Sergey Kaplun

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

[mandesero--- via Tarantool-patches]

From: Maksim Tiushev <mandesero@gmail.com>

Hi, Sergey!
Thanks for the review!

Changes:
1. Requested by Sergey Bronnikov

> realloc -> realloc()
> to highlight that realloc is a function

 - This message was dropped by Sergey Kaplun.

> In the commit message, you say that VALGRIND_OPTIONS is an env variable.
> However, it is not:

 - You're right, it doesn't work in the scenario you provided. I've
 updated the commit message to be more informative:

"Custom Valgrind testing options can be configured by setting the
`VALGRIND_OPTIONS` variable before running `cmake <..params..>`.
To update the value of this variable, modify the environment variable
`VALGRIND_OPTIONS` and rebuild the project using `cmake <..params..>`
again."

> Also, replace "valgrind" in LUAJIT_TEST_COMMAND by ${VALGRIND_BIN} and
> put "find_program(VALGRIND valgrind)" before:
>
> <snipped>
>
> I would add default set() with "LUAJIT_TEST_COMMAND" and remove a second 
> branch:

 - I kept the default `LUAJIT_TEST_COMMAND` and added a conditional branch
 for `LUAJIT_USE_VALGRIND` with the provided changes.

2. Requested by Sergey Kaplun

> It is better to be a warning by analog with the ASAN build.
> <snipped>
> 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.

 - I changed the message level to warning and removed the last one.

> Please use `LUAJIT_SOURCE_DIR` instead.
 
 - Done.

---
 CMakeLists.txt      |  5 +++++
 test/CMakeLists.txt | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

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..3cc0ddbc 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -69,6 +69,22 @@ 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)
+    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)
+  set(LUAJIT_TEST_VALGRIND_OPTS
+    "--suppressions=${LUAJIT_SOURCE_DIR}/lj.supp $ENV{VALGRIND_OPTIONS}")
+  set(LUAJIT_TEST_COMMAND
+    "${VALGRIND} ${LUAJIT_TEST_VALGRIND_OPTS} ${LUAJIT_TEST_COMMAND}")
+endif()
+
 separate_arguments(LUAJIT_TEST_COMMAND)
 
 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
-- 
2.34.1