Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v4 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow
@ 2024-12-02 13:52 mandesero--- via Tarantool-patches
  2024-12-02 13:52 ` [Tarantool-patches] [PATCH v4 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
  2024-12-02 13:52 ` [Tarantool-patches] [PATCH v4 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
  0 siblings, 2 replies; 4+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-12-02 13:52 UTC (permalink / raw)
  To: tarantool-patches, skaplun, m.kokryashkin; +Cc: Maksim Tyushev

From: Maksim Tyushev <mandesero@gmail.com>

Changes in v4:
- Replaced the `VALGRIND_OPTIONS` variable with `VALGRIND_OPTS`, which
  is provided by Valgrind.
- Enabled running `tarantool-c-tests` under Valgrind:
  * Disabled the `tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test` due
    to timeout.
- Renamed the variable for skipping tests to `LUAJIT_TEST_USE_VALGRIND`
  and setted it via `LUA_TEST_ENV_MORE`.
- Added the `--error-exitcode=1` flag to `VALGRIND_OPTS` in CI for stricter
  error handling.

Notes:

> From: Sergey Kaplun
> 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.

<snipped>

> Same for PUC-Rio-Lua-5.1-tests, lua-Harness-test.
> ...

I noticed that Valgrind generates a significant number of reports, but it
seems these are merely warnings or informational messages. At the end, the
errors appear to be suppressed, as indicated in the output.

| 1/2 Test #1: LuaJIT-tests-deps ................   Passed    0.50 sec
| test 2
|   Start 2: test/LuaJIT-tests
|
| 2: Test command: /usr/bin/valgrind "--suppressions=/home/mandesero/myforks/luajit/src/lj.supp" "/home/mandesero/myforks/luajit/build/src/luajit" "-e" "dofile[[/home/mandesero/myforks/luajit/test/luajit-test-init.lua]]" "/home/mandesero/myforks/luajit/test/LuaJIT-tests/test.lua" "+slow" "+ffi" "+bit" "+jit"
|
| 2: --6813-- used_suppression:  62656 Optimized string compare /home/mandesero/myforks/luajit/src/lj.supp:38
| 2: ==6813==
| 2: ==6813== ERROR SUMMARY: 4707 errors from 27 contexts (suppressed: 62656 from 111)

Same for others tests.

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

Enabling the `--track-fds` option for the CI sounds like a good idea.
However, I think it would be better to do this in a separate patch set.

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

--
Best regards,
Maksim Tiushev


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       | 99 +++++++++++++++++++
 CMakeLists.txt                                |  5 +
 test/CMakeLists.txt                           | 16 +++
 test/tarantool-c-tests/CMakeLists.txt         |  9 +-
 .../gh-8594-sysprof-ffunc-crash.test.c        |  7 ++
 .../gh-7745-oom-on-trace.test.lua             |  1 +
 .../lj-1034-tabov-error-frame.test.lua        |  1 +
 .../lj-512-profiler-hook-finalizers.test.lua  |  5 +-
 .../lj-726-profile-flush-close.test.lua       |  5 +-
 .../profilers/gh-5688-tool-cli-flag.test.lua  |  2 +
 ...4-add-proto-trace-sysprof-default.test.lua |  2 +
 .../profilers/misclib-sysprof-lapi.test.lua   |  2 +
 14 files changed, 175 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] 4+ messages in thread

* [Tarantool-patches] [PATCH v4 luajit 1/2] cmake: run tests with Valgrind
  2024-12-02 13:52 [Tarantool-patches] [PATCH v4 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow mandesero--- via Tarantool-patches
@ 2024-12-02 13:52 ` mandesero--- via Tarantool-patches
  2024-12-02 13:52 ` [Tarantool-patches] [PATCH v4 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
  1 sibling, 0 replies; 4+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-12-02 13:52 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. There is a
`VALGRIND_OPTS` variable [1] that we can set -- it makes the usage of
valgrind more flexible -- we can define any necessary flags in
the command line (not at the building stage). By default, the
suppression file is set to `src/lj.supp`.

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

Disabled due tarantool/tarantool#10803:
  - tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
  - tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
  - tarantool-tests/lj-726-profile-flush-close.test.lua
  - tarantool-tests/misclib-sysprof-lapi.test.lua
  - tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua

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

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

Part of tarantool/tarantool#3705
---
 CMakeLists.txt                                   |  5 +++++
 test/CMakeLists.txt                              | 16 ++++++++++++++++
 test/tarantool-c-tests/CMakeLists.txt            |  9 ++++++++-
 .../gh-8594-sysprof-ffunc-crash.test.c           |  7 +++++++
 .../gh-7745-oom-on-trace.test.lua                |  1 +
 .../lj-1034-tabov-error-frame.test.lua           |  1 +
 .../lj-512-profiler-hook-finalizers.test.lua     |  5 ++++-
 .../lj-726-profile-flush-close.test.lua          |  5 ++++-
 .../profilers/gh-5688-tool-cli-flag.test.lua     |  2 ++
 ...7264-add-proto-trace-sysprof-default.test.lua |  2 ++
 .../profilers/misclib-sysprof-lapi.test.lua      |  2 ++
 11 files changed, 52 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..b93cd0f6 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_SUPP "--suppressions=${LUAJIT_SOURCE_DIR}/lj.supp")
+  set(LUAJIT_TEST_COMMAND
+    "${VALGRIND} ${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
+  list(APPEND LUA_TEST_ENV_MORE LUAJIT_TEST_USE_VALGRIND=1)
+endif()
+
 separate_arguments(LUAJIT_TEST_COMMAND)
 
 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index 30d174bb..264f6740 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -56,10 +56,17 @@ foreach(test_source ${tests})
 
   # Generate CMake tests.
   set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}")
+  set(test_command ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX})
+
+  if(LUAJIT_USE_VALGRIND)
+    set(test_command ${VALGRIND} ${LUAJIT_TEST_VALGRIND_SUPP} ${test_command})
+  endif()
+
   add_test(NAME ${test_title}
-    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}
+    COMMAND ${test_command}
     WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
   )
+
   set_tests_properties(${test_title} PROPERTIES
     LABELS ${TEST_SUITE_NAME}
     DEPENDS tarantool-c-tests-deps
diff --git a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
index cf1d815a..569a2e80 100644
--- a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
+++ b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
@@ -46,6 +46,8 @@
  * * https://github.com/tarantool/tarantool/issues/9387
  */
 
+#define UNUSED(x) ((void)(x))
+
 #define MESSAGE "Canary is alive"
 #define LUACALL "local a = tostring('" MESSAGE "') return a"
 
@@ -248,6 +250,10 @@ static int tracer(pid_t chpid)
 
 static int test_tostring_call(void *ctx)
 {
+#if LUAJIT_USE_VALGRIND
+	UNUSED(ctx);
+	return skip("Disabled with Valgrind (Timeout)");
+#else
 	pid_t chpid = fork();
 	switch(chpid) {
 	case -1:
@@ -264,6 +270,7 @@ static int test_tostring_call(void *ctx)
 	default:
 		return tracer(chpid);
 	}
+#endif
 }
 
 #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */
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..c481da24 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("LUAJIT_TEST_USE_VALGRIND"),
 })
 
 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..de63b6d2 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("LUAJIT_TEST_USE_VALGRIND"),
 })
 
 -- 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..32b12c65 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("LUAJIT_TEST_USE_VALGRIND"),
+})
 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..b26f0603 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("LUAJIT_TEST_USE_VALGRIND"),
+})
 test:plan(1)
 
 local TEST_FILE = 'lj-726-profile-flush-close.profile'
diff --git a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
index f935dc5b..ad0d6caf 100644
--- a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
+++ b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
@@ -7,6 +7,8 @@ local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
   ['No profile tools CLI option integration'] = _TARANTOOL,
   -- 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("LUAJIT_TEST_USE_VALGRIND"),
 })
 
 test:plan(3)
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..d9a3080e 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("LUAJIT_TEST_USE_VALGRIND"),
 })
 
 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..605ff91c 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("LUAJIT_TEST_USE_VALGRIND"),
 })
 
 test:plan(19)
-- 
2.34.1


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

* [Tarantool-patches] [PATCH v4 luajit 2/2] ci: add Valgrind testing workflow
  2024-12-02 13:52 [Tarantool-patches] [PATCH v4 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow mandesero--- via Tarantool-patches
  2024-12-02 13:52 ` [Tarantool-patches] [PATCH v4 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
@ 2024-12-02 13:52 ` mandesero--- via Tarantool-patches
  2024-12-03 15:44   ` Sergey Kaplun via Tarantool-patches
  1 sibling, 1 reply; 4+ messages in thread
From: mandesero--- via Tarantool-patches @ 2024-12-02 13:52 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`.

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 tarantool/tarantool#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..50d01a45
--- /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_OPTS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose --error-exitcode=1
+            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_OPTS: --leak-check=no --malloc-fill=0x00 --free-fill=0x00 --verbose --error-exitcode=1
+            JOB_POSTFIX: "malloc/free-fill: 0x00"
+          - VALGRIND_SCENARIO: malloc-free-fill-0xff
+            VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff --verbose --error-exitcode=1
+            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_OPTS: ${{ matrix.VALGRIND_OPTS }}
+        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] 4+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 luajit 2/2] ci: add Valgrind testing workflow
  2024-12-02 13:52 ` [Tarantool-patches] [PATCH v4 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
@ 2024-12-03 15:44   ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 4+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-12-03 15:44 UTC (permalink / raw)
  To: mandesero; +Cc: tarantool-patches

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

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

<snipped>

> +          - VALGRIND_SCENARIO: full
> +            VALGRIND_OPTS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose --error-exitcode=1
> +            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_OPTS: --leak-check=no --malloc-fill=0x00 --free-fill=0x00 --verbose --error-exitcode=1
> +            JOB_POSTFIX: "malloc/free-fill: 0x00"
> +          - VALGRIND_SCENARIO: malloc-free-fill-0xff
> +            VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff --verbose --error-exitcode=1

I suppose that common options for all suites may be placed inside the
`${test_command}` during CMake configuration. Thus we don't forget about
them, and it is more convenient for local testing -- I shouldn't add
VALGRIND_OPTS="--error-exitcode=1 " each time. And this approach is more
robust (see the comment below).

> +            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_OPTS: ${{ matrix.VALGRIND_OPTS }}

I suppose this env should be provided for test step, so these options
are actually used [1]:

| Run cmake --build . --parallel --target LuaJIT-test
|   cmake --build . --parallel --target LuaJIT-test
|   shell: /usr/bin/bash -e {0}
|   env:
|     BUILDDIR: /opt/actions-runner/_work/_temp/build-12086657821
|     CMAKE_BUILD_PARALLEL_LEVEL: 9

Locally I've got errors related to each of our test suites (except
tarantool-c-tests):

| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L LuaJIT-tests
| ...
| 2: ==16791== ERROR SUMMARY: 4749 errors from 69 contexts (suppressed: 62676 from 164)
| 2/2 Test #2: test/LuaJIT-tests ................***Failed   64.23 sec
|
| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L lua-Harness-tests
| ..
| 22% tests passed, 40 tests failed out of 51
|
| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L PUC-Rio-Lua-5.1-tests
| ...
| 4: ==18453== ERROR SUMMARY: 78 errors from 14 contexts (suppressed: 363053 from 81)
| 2/2 Test #4: test/PUC-Rio-Lua-5.1-tests .......***Failed  150.28 sec
|
| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L tarantool-tests
| ...
| 85% tests passed, 27 tests failed out of 175

I suppose that most of these errors are fixed with additional
suppressions in the <src/lj.supp> since they are related to
`str_fastcmp`, etc. Also, there are some of them that require
investigation.

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

[1]: https://github.com/tarantool/luajit/actions/runs/12086657821/job/33706418395#step:6:1

-- 
Best regards,
Sergey Kaplun

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

end of thread, other threads:[~2024-12-03 15:45 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-02 13:52 [Tarantool-patches] [PATCH v4 luajit 0/2] Enable running tests with Valgrind, add CI Valgrind testing workflow mandesero--- via Tarantool-patches
2024-12-02 13:52 ` [Tarantool-patches] [PATCH v4 luajit 1/2] cmake: run tests with Valgrind mandesero--- via Tarantool-patches
2024-12-02 13:52 ` [Tarantool-patches] [PATCH v4 luajit 2/2] ci: add Valgrind testing workflow mandesero--- via Tarantool-patches
2024-12-03 15:44   ` Sergey Kaplun 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