* [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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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 2024-11-29 15:35 ` mandesero--- via Tarantool-patches 0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* [Tarantool-patches] [PATCH v3 luajit 1/2] cmake: run tests with Valgrind 2024-11-14 15:54 ` Sergey Kaplun via Tarantool-patches @ 2024-11-29 15:35 ` mandesero--- via Tarantool-patches 0 siblings, 0 replies; 11+ messages in thread From: mandesero--- via Tarantool-patches @ 2024-11-29 15:35 UTC (permalink / raw) To: tarantool-patches; +Cc: mandesero From: Maksim Tiushev <mandesero@gmail.com> Hi, Sergey! Thanks for the review! I rewrote the commit message for this patch. See the updated version below. > 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). I have removed the `VALGRIND_OPTIONS` environment variable and replaced it with the `VALGRIND_OPTS` variable provided by Valgrind. I tested this change and verified that everything works correctly. > Therefore, `LUAJIT_TEST_VALGRIND_OPTS` may be renamed to > `LUAJIT_TEST_VALGRIND_SUPP`. Agreed. Since this variable specifically handles suppression files (unlike general Valgrind options), renaming it to `LUAJIT_TEST_VALGRIND_SUPP` is a good idea. > 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`. Done, See the updated version below. > I also get the following error locally: > | test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua (SIGPROF) This test is now disabled with Valgrind. > 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, I confused with the Command content, > It looks like the valgrind with its options is missed. I'm also puzzled by the your command content. As for my test command, as you can see, it works correctly. > I suppose that we need to set `--error-exitcode` option as well. I have added the `--error-exitcode` option in the next patch. > Also, it would be nice to run all tarantool-c-tests under valgrind. Done. `tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test` was disabled due to timeout. Branch: https://github.com/tarantool/luajit/tree/mandesero/lj-3705-turn-off-strcmp-opt-in-debug -- Best regards, Maksim Tiushev ======================================================================== 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2024-11-29 15:35 UTC | newest] Thread overview: 11+ 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-11-29 15:35 ` 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 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