Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow
@ 2024-09-18  8:50 mandesero--- via Tarantool-patches
  2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-09-18  8:50 UTC (permalink / raw)
  To: tarantool-patches, skaplun, m.kokryashkin; +Cc: Maksim Tyushev

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. Please note that this environment variable must
be set before building, and any updates to it will require a project
rebuild. The patchset 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://githb.com/tarantool/luajit/tree/mandesero/lj-3705-turn-off-strcmp-opt-in-debug
Issue: https://github.com/tarantool/tarantool/issues/3705

Changes in v3:
 - Squashed commits 'run tests with Valgrind (1/3 v2)' and
   'disable failed tests (3/3 v2)'.
 - Simplified `.github/actions/setup-valgrind`, now dependents
   on `.github/actions/setup-linux`.
 - Updated `test/CMakeLists.txt` with minor adjustments.

Maksim Tiushev (2):
  cmake: run tests with Valgrind
  ci: add Valgrind testing workflow

 .github/actions/setup-valgrind/README.md      | 12 +++
 .github/actions/setup-valgrind/action.yml     | 12 +++
 .github/workflows/valgrind-testing.yaml       | 95 +++++++++++++++++++
 CMakeLists.txt                                |  5 +
 test/CMakeLists.txt                           | 16 ++++
 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, 152 insertions(+), 3 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind
  2024-09-18  8:50 [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow mandesero--- via Tarantool-patches
@ 2024-09-18  8:50 ` mandesero--- via Tarantool-patches
  2024-11-01 14:05   ` Sergey Bronnikov via Tarantool-patches
  2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
  2024-11-01 12:51 ` [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI " Sergey Bronnikov via Tarantool-patches
  2 siblings, 1 reply; 10+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-09-18  8:50 UTC (permalink / raw)
  To: tarantool-patches, skaplun, m.kokryashkin; +Cc: Maksim Tiushev

From: Maksim Tiushev <mandesero@gmail.com>

This patch enables running tests with Valgrind. Custom Valgrind testing
options can be configured by setting the `VALGRIND_OPTIONS` variable.
Please note that this environment variable must be set before building,
and any updates to it will require a project rebuild. By default, the
suppression file is set to `src/lj.supp`.

Also 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
  - test/tarantool-tests/misclib-sysprof-lapi.test.lua

For TIMEOUT:
  - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
  - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
---
 CMakeLists.txt                                   |  5 +++++
 test/CMakeLists.txt                              | 16 ++++++++++++++++
 test/tarantool-tests/CMakeLists.txt              |  3 ++-
 ...7264-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 +
 9 files changed, 33 insertions(+), 3 deletions(-)

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")
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..c5e2516b 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 (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
 })
 
 test:plan(19)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow
  2024-09-18  8:50 [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow mandesero--- via Tarantool-patches
  2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
@ 2024-09-18  8:50 ` mandesero--- via Tarantool-patches
  2024-11-01 14:17   ` Sergey Bronnikov via Tarantool-patches
  2024-11-01 12:51 ` [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI " Sergey Bronnikov via Tarantool-patches
  2 siblings, 1 reply; 10+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-09-18  8:50 UTC (permalink / raw)
  To: tarantool-patches, skaplun, m.kokryashkin; +Cc: Maksim Tiushev

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 | 12 +++
 .github/workflows/valgrind-testing.yaml   | 95 +++++++++++++++++++++++
 3 files changed, 119 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..e7d66a3a
--- /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-valgrind
+  if: ${{ matrix.OS == 'Linux' }}
+```
\ No newline at end of file
diff --git a/.github/actions/setup-valgrind/action.yml b/.github/actions/setup-valgrind/action.yml
new file mode 100644
index 00000000..5c11fdaa
--- /dev/null
+++ b/.github/actions/setup-valgrind/action.yml
@@ -0,0 +1,12 @@
+name: Setup CI environment with Valgrind on Linux
+description: Extend setup-linux with Valgrind installation
+
+runs:
+  using: composite
+  steps:
+    - name: Setup CI environment (Linux)
+      uses: ./.github/actions/setup-linux
+    - name: Install Valgrind
+      run: |
+        apt -y install valgrind
+      shell: bash
diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
new file mode 100644
index 00000000..15038092
--- /dev/null
+++ b/.github/workflows/valgrind-testing.yaml
@@ -0,0 +1,95 @@
+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
+        # We don't test on Linux/ARM64 because the address returned by the
+        # system allocator may exceed 47 bits. As a result, we are unable to
+        # allocate memory for `lua_State`. Therefore, testing on this platform
+        # is currently disabled.
+        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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow
  2024-09-18  8:50 [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow mandesero--- via Tarantool-patches
  2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
  2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
@ 2024-11-01 12:51 ` Sergey Bronnikov via Tarantool-patches
  2 siblings, 0 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-11-01 12:51 UTC (permalink / raw)
  To: mandesero, tarantool-patches, skaplun, m.kokryashkin

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

Hi, Maxim!

thanks for new iteration!

Please see my comments.

On 18.09.2024 11:50, mandesero--- via Tarantool-patches wrote:
> 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. Please note that this environment variable must
> be set before building, and any updates to it will require a project
> rebuild. The patchset 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
s/SIGPROF/sysprof/?
> disabled when `LUAJIT_USE_VALGRIND=ON`.
>
> Branch:https://githb.com/tarantool/luajit/tree/mandesero/lj-3705-turn-off-strcmp-opt-in-debug
> Issue:https://github.com/tarantool/tarantool/issues/3705
>
> Changes in v3:
>   - Squashed commits 'run tests with Valgrind (1/3 v2)' and
>     'disable failed tests (3/3 v2)'.
>   - Simplified `.github/actions/setup-valgrind`, now dependents
>     on `.github/actions/setup-linux`.
>   - Updated `test/CMakeLists.txt` with minor adjustments.
>
> Maksim Tiushev (2):
>    cmake: run tests with Valgrind
>    ci: add Valgrind testing workflow
>
>   .github/actions/setup-valgrind/README.md      | 12 +++
>   .github/actions/setup-valgrind/action.yml     | 12 +++
>   .github/workflows/valgrind-testing.yaml       | 95 +++++++++++++++++++
>   CMakeLists.txt                                |  5 +
>   test/CMakeLists.txt                           | 16 ++++
>   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, 152 insertions(+), 3 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
>

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind
  2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
@ 2024-11-01 14:05   ` Sergey Bronnikov via Tarantool-patches
  2024-11-11 11:03     ` mandesero--- via Tarantool-patches
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-11-01 14:05 UTC (permalink / raw)
  To: mandesero, tarantool-patches, skaplun, m.kokryashkin

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

Hi, Maxim!

thanks for the patch!


On 18.09.2024 11:50, mandesero--- via Tarantool-patches wrote:
> From: Maksim Tiushev<mandesero@gmail.com>
>
> This patch enables running tests with Valgrind. Custom Valgrind testing
> options can be configured by setting the `VALGRIND_OPTIONS` variable.
> Please note that this environment variable must be set before building,
> and any updates to it will require a project rebuild. By default, the
> suppression file is set to `src/lj.supp`.
>
> Also 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
>    - test/tarantool-tests/misclib-sysprof-lapi.test.lua
>
> For TIMEOUT:

It is not clear what do you mean with "for TIMEOUT".

Whether these tests timed out due to running under Valgrind or not?

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


I think we need followup issues for these tests and put a link to the 
issue to the

commit message. Otherwise, these tests will remain unfixed. It is 
desirable to

describe details of failures under Valgrind in that issue.

> ---
>   CMakeLists.txt                                   |  5 +++++
>   test/CMakeLists.txt                              | 16 ++++++++++++++++
>   test/tarantool-tests/CMakeLists.txt              |  3 ++-
>   ...7264-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 +
>   9 files changed, 33 insertions(+), 3 deletions(-)
>
> 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")
> 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',

" (SIGPROF)" looks useless, please remove. Here and below.

I would add a number of the issue instead.

>   })
>   
>   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..c5e2516b 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 (SIGPROF)'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
>   })
>   
>   test:plan(19)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow
  2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
@ 2024-11-01 14:17   ` Sergey Bronnikov via Tarantool-patches
  2024-11-11 13:04     ` mandesero--- via Tarantool-patches
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-11-01 14:17 UTC (permalink / raw)
  To: mandesero, tarantool-patches, skaplun, m.kokryashkin

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

Hi, Maxim,

thanks for the patch! See my comments below.


On 18.09.2024 11:50, mandesero--- via Tarantool-patches wrote:
> 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`.

Motivation behind chosen configurations is not clear.

And what is a difference for "--malloc-fill=0x00"/"--free-fill=0x00"

and "--malloc-fill=0xFF"/"--free-fill=0xFF". Why the fill value make 
sense here?


Also, we usually mention an issue number in commit messages.

Cover letter says that the issue behind these patches is

https://github.com/tarantool/tarantool/issues/3705

but commit messages for  both patches doesn't mention the issue.

I propose to add "Part of #3705" to the first patch and

"Closes #3705" to the second patch.

> ---
>   .github/actions/setup-valgrind/README.md  | 12 +++
>   .github/actions/setup-valgrind/action.yml | 12 +++
>   .github/workflows/valgrind-testing.yaml   | 95 +++++++++++++++++++++++
>   3 files changed, 119 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..e7d66a3a
> --- /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-valgrind
> +  if: ${{ matrix.OS == 'Linux' }}
> +```
> \ No newline at end of file
> diff --git a/.github/actions/setup-valgrind/action.yml b/.github/actions/setup-valgrind/action.yml
> new file mode 100644
> index 00000000..5c11fdaa
> --- /dev/null
> +++ b/.github/actions/setup-valgrind/action.yml
> @@ -0,0 +1,12 @@
> +name: Setup CI environment with Valgrind on Linux
> +description: Extend setup-linux with Valgrind installation
> +
> +runs:
> +  using: composite
> +  steps:
> +    - name: Setup CI environment (Linux)
> +      uses: ./.github/actions/setup-linux
> +    - name: Install Valgrind
> +      run: |
> +        apt -y install valgrind
> +      shell: bash
> diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
> new file mode 100644
> index 00000000..15038092
> --- /dev/null
> +++ b/.github/workflows/valgrind-testing.yaml
> @@ -0,0 +1,95 @@
> +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
> +        # We don't test on Linux/ARM64 because the address returned by the
> +        # system allocator may exceed 47 bits. As a result, we are unable to
> +        # allocate memory for `lua_State`. Therefore, testing on this platform
> +        # is currently disabled.
> +        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
This config doesn't have VALGRIND_OPTIONS, is it intentional?
> +          - BUILDTYPE: Release
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
Why JOB_POSTFIX is empty here?
> +          - VALGRIND_SCENARIO: full
> +            VALGRIND_OPTIONS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose
Please explain a meaning of specified options.
> +            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: 8344 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind
  2024-11-01 14:05   ` Sergey Bronnikov via Tarantool-patches
@ 2024-11-11 11:03     ` mandesero--- via Tarantool-patches
  2024-11-14 14:04       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 10+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-11-11 11:03 UTC (permalink / raw)
  To: tarantool-patches; +Cc: mandesero

From: Maksim Tiushev <mandesero@gmail.com>

This patch enables running tests with Valgrind. Custom Valgrind testing
options can be configured by setting the `VALGRIND_OPTIONS` variable.
Please note that this environment variable must be set before building,
and any updates to it will require a project rebuild. By default, the
suppression file is set to `src/lj.supp`.

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

Disabled due #10803:
  - 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
  - test/tarantool-tests/misclib-sysprof-lapi.test.lua

Timed out due to running under Valgrind:
  - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
  - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua

Part of #3705
---
 CMakeLists.txt                                   |  5 +++++
 test/CMakeLists.txt                              | 16 ++++++++++++++++
 test/tarantool-tests/CMakeLists.txt              |  3 ++-
 .../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 ++++-
 ...7264-add-proto-trace-sysprof-default.test.lua |  2 ++
 .../profilers/misclib-sysprof-lapi.test.lua      |  2 ++
 9 files changed, 37 insertions(+), 3 deletions(-)

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")
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 8bdb2cf3..39562f35 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -110,7 +110,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_TEST_ENV_MORE}"
+    ENVIRONMENT "LUA_PATH=${LUA_PATH};${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-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..8f32b9ef 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,10 @@
 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({
+  -- See also https://github.com/tarantool/tarantool/issues/10803
+  ['Disabled due to #10803'] = 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..27eb3751 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,9 @@
 local tap = require('tap')
 
-local test = tap.test('lj-726-profile-flush-close')
+local test = tap.test('lj-726-profile-flush-close'):skipcond({
+  -- See also https://github.com/tarantool/tarantool/issues/10803
+  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
+})
 test:plan(1)
 
 local TEST_FILE = 'lj-726-profile-flush-close.profile'
diff --git a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
index 176c1a15..e6c853a1 100644
--- a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
+++ b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
@@ -6,6 +6,8 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
   ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
   -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
   ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
+  -- See also https://github.com/tarantool/tarantool/issues/10803
+  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
 })
 
 test:plan(2)
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 26c277cd..2210d668 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -5,6 +5,8 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
   ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
   -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
   ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
+  -- See also https://github.com/tarantool/tarantool/issues/10803
+  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',
 })
 
 test:plan(19)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow
  2024-11-01 14:17   ` Sergey Bronnikov via Tarantool-patches
@ 2024-11-11 13:04     ` mandesero--- via Tarantool-patches
  0 siblings, 0 replies; 10+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-11-11 13:04 UTC (permalink / raw)
  To: tarantool-patches; +Cc: mandesero

From: Maksim Tiushev <mandesero@gmail.com>

Hi, Sergey!

Thanks for the review!

I updated the commit message, see below.

> > +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> This config doesn't have VALGRIND_OPTIONS, is it intentional?

Yes, according to the previous patch, `VALGRIND_OPTIONS` is an
environment variable and must be set before building. It is not
a CMake option. It is setted in the `configure` step in this
workflow.

> > +          - BUILDTYPE: Release
> > +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> Why JOB_POSTFIX is empty here?

`JOB_POSTFIX` describes Valgrind options, not the build type
or CMake options.

---------------------------------------------------------------

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

The use of `0x00` and `0xFF` for memory fill helps to detect dirty
reads. `0x00` mimics zero-initialized memory, which can mask some
uninitialized memory usage. `0xFF` fills memory with a non-zero
values to make such errors easier to spot.

Closes #3705
---
 .github/actions/setup-valgrind/README.md  | 12 +++
 .github/actions/setup-valgrind/action.yml | 12 +++
 .github/workflows/valgrind-testing.yaml   | 99 +++++++++++++++++++++++
 3 files changed, 123 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..e7d66a3a
--- /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-valgrind
+  if: ${{ matrix.OS == 'Linux' }}
+```
\ No newline at end of file
diff --git a/.github/actions/setup-valgrind/action.yml b/.github/actions/setup-valgrind/action.yml
new file mode 100644
index 00000000..5c11fdaa
--- /dev/null
+++ b/.github/actions/setup-valgrind/action.yml
@@ -0,0 +1,12 @@
+name: Setup CI environment with Valgrind on Linux
+description: Extend setup-linux with Valgrind installation
+
+runs:
+  using: composite
+  steps:
+    - name: Setup CI environment (Linux)
+      uses: ./.github/actions/setup-linux
+    - name: Install Valgrind
+      run: |
+        apt -y install valgrind
+      shell: bash
diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
new file mode 100644
index 00000000..44056fcd
--- /dev/null
+++ b/.github/workflows/valgrind-testing.yaml
@@ -0,0 +1,99 @@
+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
+        # We don't test on Linux/ARM64 because the address returned by the
+        # system allocator may exceed 47 bits. As a result, we are unable to
+        # allocate memory for `lua_State`. Therefore, testing on this platform
+        # is currently disabled.
+        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"
+          # The use of `0x00` and `0xFF` for memory fill helps to detect dirty
+          #  reads. `0x00` mimics zero-initialized memory, which can mask some
+          #  uninitialized memory usage. `0xFF` fills memory with a non-zero
+          #  values to make such errors easier to spot.
+          - 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind
  2024-11-11 11:03     ` mandesero--- via Tarantool-patches
@ 2024-11-14 14:04       ` Sergey Kaplun via Tarantool-patches
  2024-11-14 15:54         ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-11-14 14:04 UTC (permalink / raw)
  To: mandesero; +Cc: tarantool-patches

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

On 11.11.24, mandesero@gmail.com wrote:
> From: Maksim Tiushev <mandesero@gmail.com>
> 
> This patch enables running tests with Valgrind. Custom Valgrind testing
> options can be configured by setting the `VALGRIND_OPTIONS` variable.
> Please note that this environment variable must be set before building,
> and any updates to it will require a project rebuild. By default, the
> suppression file is set to `src/lj.supp`.

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

> 
> Also this patch disables the following tests when running with Valgrind

Typo: s/Also/Also,/

> due to failures:
> 
> Disabled due #10803:

Typo: s<#10803><tarantool/tarantool#10803>

>   - test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua

Nit: Commit line width is more than 72 symbols.

>   - test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>   - test/tarantool-tests/lj-726-profile-flush-close.test.lua
>   - test/tarantool-tests/misclib-sysprof-lapi.test.lua
> 
> Timed out due to running under Valgrind:
>   - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
>   - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> 
> Part of #3705

Typo: s<#3705><tarantool/tarantool#3705>

> ---
>  CMakeLists.txt                                   |  5 +++++
>  test/CMakeLists.txt                              | 16 ++++++++++++++++
>  test/tarantool-tests/CMakeLists.txt              |  3 ++-
>  .../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 ++++-
>  ...7264-add-proto-trace-sysprof-default.test.lua |  2 ++
>  .../profilers/misclib-sysprof-lapi.test.lua      |  2 ++
>  9 files changed, 37 insertions(+), 3 deletions(-)
> 
> 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..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}")

I suppose that we can drop $ENV{VALGRIND_OPTIONS}, since 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).

Therefore, `LUAJIT_TEST_VALGRIND_OPTS` may be renamed to
`LUAJIT_TEST_VALGRIND_SUPP`.

> +  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")
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 8bdb2cf3..39562f35 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -110,7 +110,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_TEST_ENV_MORE}"
> +    ENVIRONMENT "LUA_PATH=${LUA_PATH};${LUA_TEST_ENV_MORE};\
> +LJ_USE_VALGRIND=${LUAJIT_USE_VALGRIND}"

It is better to set this variable to the `LUA_TEST_ENV_MORE` list.
Also, I suggest rename it to the `LUAJIT_TEST_USE_VALGRIND`.

>      LABELS ${TEST_SUITE_NAME}
>      DEPENDS tarantool-tests-deps
>    )
> 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',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  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',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  -- 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..8f32b9ef 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,10 @@
>  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({
> +  -- See also https://github.com/tarantool/tarantool/issues/10803

Nit: Missed dot at the end of the sentence.

> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

> +})
>  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..27eb3751 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,9 @@
>  local tap = require('tap')
>  
> -local test = tap.test('lj-726-profile-flush-close')
> +local test = tap.test('lj-726-profile-flush-close'):skipcond({
> +  -- See also https://github.com/tarantool/tarantool/issues/10803

Nit: Missed dot at the end of the sentence.

> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

> +})
>  test:plan(1)
>  
>  local TEST_FILE = 'lj-726-profile-flush-close.profile'
> diff --git a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
> index 176c1a15..e6c853a1 100644
> --- a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
> +++ b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
> @@ -6,6 +6,8 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
>    ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
>    -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
>    ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
> +  -- See also https://github.com/tarantool/tarantool/issues/10803

Nit: Missed dot at the end of the sentence.

> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  test:plan(2)
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> index 26c277cd..2210d668 100644
> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> @@ -5,6 +5,8 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
>    ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
>    -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
>    ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
>
Nit: Missed dot at the end of the sentence.

 +  -- See also https://github.com/tarantool/tarantool/issues/10803
> +  ['Disabled due to #10803'] = os.getenv("LJ_USE_VALGRIND") == 'ON',

Minor: there is no need to check the custom value.
If the variable isn't set `os.getenv()` returns `nil`.

>  })
>  
>  test:plan(19)
> -- 
> 2.34.1
> 

[1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind
  2024-11-14 14:04       ` Sergey Kaplun via Tarantool-patches
@ 2024-11-14 15:54         ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-11-14 15:54 UTC (permalink / raw)
  To: mandesero, tarantool-patches

On 14.11.24, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Maksim!
> Thanks for the patch!
> Please consider my comments below.
> 
> On 11.11.24, mandesero@gmail.com wrote:

<snipped>

> >   - test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
> >   - test/tarantool-tests/lj-726-profile-flush-close.test.lua
> >   - test/tarantool-tests/misclib-sysprof-lapi.test.lua

I also get the following error locally:

| test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua (SIGPROF)

So, it looks like all sysprof's tests should be disabled.

> > 
> > Timed out due to running under Valgrind:
> >   - test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> >   - test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> > 

Also, I've found that tests from LuaJIT-tests suite reports tons of the
assertions, but still the suite passes. This should be fixed:

I suppose that we need to set `--error-exitcode` option as well.

The option `--exit-on-first-error` may be also considered.

| ctest --verbose -L LuaJIT-tests
| ...
| 2: ==20374== Command: /home/burii/reviews/luajit/valgrind-ci/src/luajit -e dofile[[/home/burii/reviews/luajit/valgrind-ci/test/luajit-test-init.lua]] /home/burii/reviews/luajit/valgrind-ci/test/LuaJIT-tests/test.lua +slow +ffi +bit +jit
| 2: ==20374==
| 2: ==20374== Invalid read of size 4
| 2: ==20374==    at 0x137059: lj_getu32 (lj_def.h:244)

Same for PUC-Rio-Lua-5.1-tests, lua-Harness-test.
Also, I see these warnings for tarantool-tests suite as well, see
| ctest --verbose -R test/tarantool-tests/fix-jit-dump-ir-conv.test.lua
for example:
| 82: ==25641== Conditional jump or move depends on uninitialised value(s)
| 82: ==25641==    at 0x1BCD8F: lj_BC_ADDVN (/home/burii/reviews/luajit/valgrind-ci/src/lj_vm.S:252)
| 82: ==25641==    by 0x1445F3: lj_vmevent_call (lj_vmevent.c:46)
| 82: ==25641==    by 0x186EB5: trace_stop (lj_trace.c:555)
| 82: ==25641==    by 0x184422: trace_state (lj_trace.c:726)
| 82: ==25641==    by 0x1BE97D: lj_vm_cpcall (/home/burii/reviews/luajit/valgrind-ci/src/lj_vm.S:1266)
| 82: ==25641==    by 0x183CA1: lj_trace_ins (lj_trace.c:757)
| 82: ==25641==    by 0x1263C4: lj_dispatch_ins (lj_dispatch.c:430)
| 82: ==25641==    by 0x1C0D69: lj_vm_inshook (/home/burii/reviews/luajit/valgrind-ci/src/lj_vm.S:2649)
| 82: ==25641==    by 0x11FCCA: lua_pcall (lj_api.c:1173)
| 82: ==25641==    by 0x112A84: docall (luajit.c:133)
| 82: ==25641==    by 0x112620: handle_script (luajit.c:304)
| 82: ==25641==    by 0x111B9C: pmain (luajit.c:604)

Also, I confused with the Command content,
It looks like the valgrind with its options is missed.

Also, it would be nice to run all tarantool-c-tests under valgrind.

Also, what do you think about enabling option `--track-fds` for the CI
(maybe in the separate patch set).

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-11-14 15:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-18  8:50 [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow mandesero--- via Tarantool-patches
2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
2024-11-01 14:05   ` Sergey Bronnikov via Tarantool-patches
2024-11-11 11:03     ` mandesero--- via Tarantool-patches
2024-11-14 14:04       ` Sergey Kaplun via Tarantool-patches
2024-11-14 15:54         ` Sergey Kaplun via Tarantool-patches
2024-09-18  8:50 ` [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
2024-11-01 14:17   ` Sergey Bronnikov via Tarantool-patches
2024-11-11 13:04     ` mandesero--- via Tarantool-patches
2024-11-01 12:51 ` [Tarantool-patches] [PATCH v3 luajit 0/2] Enable running tests with Valgrind, add CI " Sergey Bronnikov via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox