* [Tarantool-patches] [PATCH v5 luajit 0/3] Valgrind testing
@ 2024-12-11 13:21 Sergey Kaplun via Tarantool-patches
2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions Sergey Kaplun via Tarantool-patches
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-12-11 13:21 UTC (permalink / raw)
To: Maksim Tiushev, Sergey Bronnikov; +Cc: tarantool-patches
Changes in v5:
- Backported patch to fix "uninitalised value" warning during vmevent
processing of the `IR_NOP`.
- Added the additional suppression file to avoid false positives in
string comparisons not covered by the original suppression file.
- Remove the `--verbose` flag, since it makes the output too messy.
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.
Branch: https://github.com/tarantool/luajit/tree/mandesero/lj-3705-turn-off-strcmp-opt-in-debug
Related issues:
* https://github.com/tarantool/tarantool/issues/10709
* https://github.com/tarantool/tarantool/issues/3705
* https://github.com/LuaJIT/LuaJIT/issues/624
Maksim Tiushev (2):
cmake: run tests with Valgrind
ci: add Valgrind testing workflow
Mike Pall (1):
Ensure full init of IR_NOP instructions.
.github/actions/setup-valgrind/README.md | 12 +++
.github/actions/setup-valgrind/action.yml | 12 +++
.github/workflows/valgrind-testing.yaml | 102 ++++++++++++++++++
CMakeLists.txt | 5 +
src/lj_asm.c | 2 +-
src/lj_extra.supp | 19 ++++
src/lj_ir.h | 8 ++
src/lj_opt_dce.c | 5 +-
src/lj_opt_mem.c | 25 +----
test/CMakeLists.txt | 19 ++++
test/tarantool-c-tests/CMakeLists.txt | 11 +-
.../gh-8594-sysprof-ffunc-crash.test.c | 9 ++
test/tarantool-tests/CMakeLists.txt | 6 ++
.../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 +
20 files changed, 225 insertions(+), 28 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
create mode 100644 src/lj_extra.supp
--
2.47.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions. 2024-12-11 13:21 [Tarantool-patches] [PATCH v5 luajit 0/3] Valgrind testing Sergey Kaplun via Tarantool-patches @ 2024-12-11 13:21 ` Sergey Kaplun via Tarantool-patches 2024-12-13 12:54 ` Sergey Bronnikov via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind Sergey Kaplun via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow Sergey Kaplun via Tarantool-patches 2 siblings, 1 reply; 14+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-12-11 13:21 UTC (permalink / raw) To: Maksim Tiushev, Sergey Bronnikov; +Cc: tarantool-patches From: Mike Pall <mike> (cherry picked from commit de6b1a11dd1a3349179084578c5d533be1c30234) Before this patch, Valgrind produces tons of warnings during the VMevent calls since the `IR_NOP` instruction isn't fully initialized. Hence, any parsing operations for it in handlers during `jit.dump()` leads to the "uninitialised value" error. This patch fixes the issue by the proper init of such IRs. Sergey Kaplun: * added the description for the problem Needed for tarantool/tarantool#3705 Part of tarantool/tarantool#10709 --- src/lj_asm.c | 2 +- src/lj_ir.h | 8 ++++++++ src/lj_opt_dce.c | 5 +---- src/lj_opt_mem.c | 25 +++++-------------------- 4 files changed, 15 insertions(+), 25 deletions(-) diff --git a/src/lj_asm.c b/src/lj_asm.c index 5137d05c..f7540165 100644 --- a/src/lj_asm.c +++ b/src/lj_asm.c @@ -2377,7 +2377,7 @@ void lj_asm_trace(jit_State *J, GCtrace *T) /* Ensure an initialized instruction beyond the last one for HIOP checks. */ /* This also allows one RENAME to be added without reallocating curfinal. */ as->orignins = lj_ir_nextins(J); - J->cur.ir[as->orignins].o = IR_NOP; + lj_ir_nop(&J->cur.ir[as->orignins]); /* Setup initial state. Copy some fields to reduce indirections. */ as->J = J; diff --git a/src/lj_ir.h b/src/lj_ir.h index 27c66f63..46e7413e 100644 --- a/src/lj_ir.h +++ b/src/lj_ir.h @@ -587,4 +587,12 @@ static LJ_AINLINE int ir_sideeff(IRIns *ir) LJ_STATIC_ASSERT((int)IRT_GUARD == (int)IRM_W); +/* Replace IR instruction with NOP. */ +static LJ_AINLINE void lj_ir_nop(IRIns *ir) +{ + ir->ot = IRT(IR_NOP, IRT_NIL); + ir->op1 = ir->op2 = 0; + ir->prev = 0; +} + #endif diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c index 6948179c..41e36d99 100644 --- a/src/lj_opt_dce.c +++ b/src/lj_opt_dce.c @@ -46,10 +46,7 @@ static void dce_propagate(jit_State *J) irt_clearmark(ir->t); } else if (!ir_sideeff(ir)) { *pchain[ir->o] = ir->prev; /* Reroute original instruction chain. */ - ir->t.irt = IRT_NIL; - ir->o = IR_NOP; /* Replace instruction with NOP. */ - ir->op1 = ir->op2 = 0; - ir->prev = 0; + lj_ir_nop(ir); continue; } pchain[ir->o] = &ir->prev; diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c index c9f1216c..a6e9a072 100644 --- a/src/lj_opt_mem.c +++ b/src/lj_opt_mem.c @@ -375,10 +375,7 @@ TRef LJ_FASTCALL lj_opt_dse_ahstore(jit_State *J) goto doemit; /* No elimination possible. */ /* Remove redundant store from chain and replace with NOP. */ *refp = store->prev; - store->o = IR_NOP; - store->t.irt = IRT_NIL; - store->op1 = store->op2 = 0; - store->prev = 0; + lj_ir_nop(store); /* Now emit the new store instead. */ } goto doemit; @@ -479,10 +476,7 @@ TRef LJ_FASTCALL lj_opt_dse_ustore(jit_State *J) goto doemit; /* No elimination possible. */ /* Remove redundant store from chain and replace with NOP. */ *refp = store->prev; - store->o = IR_NOP; - store->t.irt = IRT_NIL; - store->op1 = store->op2 = 0; - store->prev = 0; + lj_ir_nop(store); if (ref+1 < J->cur.nins && store[1].o == IR_OBAR && store[1].op1 == xref) { IRRef1 *bp = &J->chain[IR_OBAR]; @@ -491,10 +485,7 @@ TRef LJ_FASTCALL lj_opt_dse_ustore(jit_State *J) bp = &obar->prev; /* Remove OBAR, too. */ *bp = obar->prev; - obar->o = IR_NOP; - obar->t.irt = IRT_NIL; - obar->op1 = obar->op2 = 0; - obar->prev = 0; + lj_ir_nop(obar); } /* Now emit the new store instead. */ } @@ -585,10 +576,7 @@ TRef LJ_FASTCALL lj_opt_dse_fstore(jit_State *J) goto doemit; /* No elimination possible. */ /* Remove redundant store from chain and replace with NOP. */ *refp = store->prev; - store->o = IR_NOP; - store->t.irt = IRT_NIL; - store->op1 = store->op2 = 0; - store->prev = 0; + lj_ir_nop(store); /* Now emit the new store instead. */ } goto doemit; @@ -839,10 +827,7 @@ TRef LJ_FASTCALL lj_opt_dse_xstore(jit_State *J) goto doemit; /* No elimination possible. */ /* Remove redundant store from chain and replace with NOP. */ *refp = store->prev; - store->o = IR_NOP; - store->t.irt = IRT_NIL; - store->op1 = store->op2 = 0; - store->prev = 0; + lj_ir_nop(store); /* Now emit the new store instead. */ } goto doemit; -- 2.47.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions. 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions Sergey Kaplun via Tarantool-patches @ 2024-12-13 12:54 ` Sergey Bronnikov via Tarantool-patches 2024-12-16 11:24 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 14+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-12-13 12:54 UTC (permalink / raw) To: Sergey Kaplun, Maksim Tiushev; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 4937 bytes --] Hello, Sergey, thanks for the patch! LGTM with a minor comment. On 11.12.2024 16:21, Sergey Kaplun wrote: > From: Mike Pall <mike> > > (cherry picked from commit de6b1a11dd1a3349179084578c5d533be1c30234) > > Before this patch, Valgrind produces tons of warnings during the VMevent > calls since the `IR_NOP` instruction isn't fully initialized. Hence, any > parsing operations for it in handlers during `jit.dump()` leads to the > "uninitialised value" error. This patch fixes the issue by the proper > init of such IRs. Please add to a comment that with these tests the problem could be reproduced: test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua test/tarantool-tests/fix-jit-dump-ir-conv.test.lua test/tarantool-tests/unit-jit-parse.test.lua > > Sergey Kaplun: > * added the description for the problem > > Needed for tarantool/tarantool#3705 > Part of tarantool/tarantool#10709 > --- > src/lj_asm.c | 2 +- > src/lj_ir.h | 8 ++++++++ > src/lj_opt_dce.c | 5 +---- > src/lj_opt_mem.c | 25 +++++-------------------- > 4 files changed, 15 insertions(+), 25 deletions(-) > > diff --git a/src/lj_asm.c b/src/lj_asm.c > index 5137d05c..f7540165 100644 > --- a/src/lj_asm.c > +++ b/src/lj_asm.c > @@ -2377,7 +2377,7 @@ void lj_asm_trace(jit_State *J, GCtrace *T) > /* Ensure an initialized instruction beyond the last one for HIOP checks. */ > /* This also allows one RENAME to be added without reallocating curfinal. */ > as->orignins = lj_ir_nextins(J); > - J->cur.ir[as->orignins].o = IR_NOP; > + lj_ir_nop(&J->cur.ir[as->orignins]); > > /* Setup initial state. Copy some fields to reduce indirections. */ > as->J = J; > diff --git a/src/lj_ir.h b/src/lj_ir.h > index 27c66f63..46e7413e 100644 > --- a/src/lj_ir.h > +++ b/src/lj_ir.h > @@ -587,4 +587,12 @@ static LJ_AINLINE int ir_sideeff(IRIns *ir) > > LJ_STATIC_ASSERT((int)IRT_GUARD == (int)IRM_W); > > +/* Replace IR instruction with NOP. */ > +static LJ_AINLINE void lj_ir_nop(IRIns *ir) > +{ > + ir->ot = IRT(IR_NOP, IRT_NIL); > + ir->op1 = ir->op2 = 0; > + ir->prev = 0; > +} > + > #endif > diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c > index 6948179c..41e36d99 100644 > --- a/src/lj_opt_dce.c > +++ b/src/lj_opt_dce.c > @@ -46,10 +46,7 @@ static void dce_propagate(jit_State *J) > irt_clearmark(ir->t); > } else if (!ir_sideeff(ir)) { > *pchain[ir->o] = ir->prev; /* Reroute original instruction chain. */ > - ir->t.irt = IRT_NIL; > - ir->o = IR_NOP; /* Replace instruction with NOP. */ > - ir->op1 = ir->op2 = 0; > - ir->prev = 0; > + lj_ir_nop(ir); > continue; > } > pchain[ir->o] = &ir->prev; > diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c > index c9f1216c..a6e9a072 100644 > --- a/src/lj_opt_mem.c > +++ b/src/lj_opt_mem.c > @@ -375,10 +375,7 @@ TRef LJ_FASTCALL lj_opt_dse_ahstore(jit_State *J) > goto doemit; /* No elimination possible. */ > /* Remove redundant store from chain and replace with NOP. */ > *refp = store->prev; > - store->o = IR_NOP; > - store->t.irt = IRT_NIL; > - store->op1 = store->op2 = 0; > - store->prev = 0; > + lj_ir_nop(store); > /* Now emit the new store instead. */ > } > goto doemit; > @@ -479,10 +476,7 @@ TRef LJ_FASTCALL lj_opt_dse_ustore(jit_State *J) > goto doemit; /* No elimination possible. */ > /* Remove redundant store from chain and replace with NOP. */ > *refp = store->prev; > - store->o = IR_NOP; > - store->t.irt = IRT_NIL; > - store->op1 = store->op2 = 0; > - store->prev = 0; > + lj_ir_nop(store); > if (ref+1 < J->cur.nins && > store[1].o == IR_OBAR && store[1].op1 == xref) { > IRRef1 *bp = &J->chain[IR_OBAR]; > @@ -491,10 +485,7 @@ TRef LJ_FASTCALL lj_opt_dse_ustore(jit_State *J) > bp = &obar->prev; > /* Remove OBAR, too. */ > *bp = obar->prev; > - obar->o = IR_NOP; > - obar->t.irt = IRT_NIL; > - obar->op1 = obar->op2 = 0; > - obar->prev = 0; > + lj_ir_nop(obar); > } > /* Now emit the new store instead. */ > } > @@ -585,10 +576,7 @@ TRef LJ_FASTCALL lj_opt_dse_fstore(jit_State *J) > goto doemit; /* No elimination possible. */ > /* Remove redundant store from chain and replace with NOP. */ > *refp = store->prev; > - store->o = IR_NOP; > - store->t.irt = IRT_NIL; > - store->op1 = store->op2 = 0; > - store->prev = 0; > + lj_ir_nop(store); > /* Now emit the new store instead. */ > } > goto doemit; > @@ -839,10 +827,7 @@ TRef LJ_FASTCALL lj_opt_dse_xstore(jit_State *J) > goto doemit; /* No elimination possible. */ > /* Remove redundant store from chain and replace with NOP. */ > *refp = store->prev; > - store->o = IR_NOP; > - store->t.irt = IRT_NIL; > - store->op1 = store->op2 = 0; > - store->prev = 0; > + lj_ir_nop(store); > /* Now emit the new store instead. */ > } > goto doemit; [-- Attachment #2: Type: text/html, Size: 5563 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions. 2024-12-13 12:54 ` Sergey Bronnikov via Tarantool-patches @ 2024-12-16 11:24 ` Sergey Kaplun via Tarantool-patches 2024-12-17 11:08 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 14+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-12-16 11:24 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: Maksim Tiushev, tarantool-patches Hi, Sergey! Thanks for the review! On 13.12.24, Sergey Bronnikov wrote: > Hello, Sergey, > > thanks for the patch! LGTM with a minor comment. > > On 11.12.2024 16:21, Sergey Kaplun wrote: > > From: Mike Pall <mike> > > > > (cherry picked from commit de6b1a11dd1a3349179084578c5d533be1c30234) > > > > Before this patch, Valgrind produces tons of warnings during the VMevent > > calls since the `IR_NOP` instruction isn't fully initialized. Hence, any > > parsing operations for it in handlers during `jit.dump()` leads to the > > "uninitialised value" error. This patch fixes the issue by the proper > > init of such IRs. > > Please add to a comment that with these tests the problem could > > be reproduced: > > test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua > > test/tarantool-tests/fix-jit-dump-ir-conv.test.lua > > test/tarantool-tests/unit-jit-parse.test.lua Updated. The new commit message is the following: | Ensure full init of IR_NOP instructions. | | (cherry picked from commit de6b1a11dd1a3349179084578c5d533be1c30234) | | Before this patch, Valgrind produces tons of warnings during the VMevent | calls since the `IR_NOP` instruction isn't fully initialized. Hence, any | parsing operations for it in handlers during `jit.dump()` leads to the | "uninitialised value" error. | | This can be reproduced with the following tests from the | tarantool-tests suite: | * fix-jit-dump-ir-conv.test.lua | * lj-1128-table-new-opt-tnew.test.lua | * lj-981-folding-0.test.lua | * unit-jit-parse.test.lua | | This patch fixes the issue by the proper init of such IRs. | | Sergey Kaplun: | * added the description for the problem | | Needed for tarantool/tarantool#3705 | Part of tarantool/tarantool#10709 > > > > > Sergey Kaplun: > > * added the description for the problem > > > > Needed for tarantool/tarantool#3705 > > Part of tarantool/tarantool#10709 > > --- <snipped> -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions. 2024-12-16 11:24 ` Sergey Kaplun via Tarantool-patches @ 2024-12-17 11:08 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 0 replies; 14+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-12-17 11:08 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Maksim Tiushev, tarantool-patches [-- Attachment #1: Type: text/plain, Size: 2012 bytes --] Hello, LGTM On 16.12.2024 14:24, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the review! > > On 13.12.24, Sergey Bronnikov wrote: >> Hello, Sergey, >> >> thanks for the patch! LGTM with a minor comment. >> >> On 11.12.2024 16:21, Sergey Kaplun wrote: >>> From: Mike Pall <mike> >>> >>> (cherry picked from commit de6b1a11dd1a3349179084578c5d533be1c30234) >>> >>> Before this patch, Valgrind produces tons of warnings during the VMevent >>> calls since the `IR_NOP` instruction isn't fully initialized. Hence, any >>> parsing operations for it in handlers during `jit.dump()` leads to the >>> "uninitialised value" error. This patch fixes the issue by the proper >>> init of such IRs. >> Please add to a comment that with these tests the problem could >> >> be reproduced: >> >> test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua >> >> test/tarantool-tests/fix-jit-dump-ir-conv.test.lua >> >> test/tarantool-tests/unit-jit-parse.test.lua > Updated. The new commit message is the following: > > | Ensure full init of IR_NOP instructions. > | > | (cherry picked from commit de6b1a11dd1a3349179084578c5d533be1c30234) > | > | Before this patch, Valgrind produces tons of warnings during the VMevent > | calls since the `IR_NOP` instruction isn't fully initialized. Hence, any > | parsing operations for it in handlers during `jit.dump()` leads to the > | "uninitialised value" error. > | > | This can be reproduced with the following tests from the > | tarantool-tests suite: > | * fix-jit-dump-ir-conv.test.lua > | * lj-1128-table-new-opt-tnew.test.lua > | * lj-981-folding-0.test.lua > | * unit-jit-parse.test.lua > | > | This patch fixes the issue by the proper init of such IRs. > | > | Sergey Kaplun: > | * added the description for the problem > | > | Needed for tarantool/tarantool#3705 > | Part of tarantool/tarantool#10709 > >>> Sergey Kaplun: >>> * added the description for the problem >>> >>> Needed for tarantool/tarantool#3705 >>> Part of tarantool/tarantool#10709 >>> --- > <snipped> > [-- Attachment #2: Type: text/html, Size: 2896 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind 2024-12-11 13:21 [Tarantool-patches] [PATCH v5 luajit 0/3] Valgrind testing Sergey Kaplun via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions Sergey Kaplun via Tarantool-patches @ 2024-12-11 13:21 ` Sergey Kaplun via Tarantool-patches 2024-12-13 13:18 ` Sergey Bronnikov via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow Sergey Kaplun via Tarantool-patches 2 siblings, 1 reply; 14+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-12-11 13:21 UTC (permalink / raw) To: Maksim Tiushev, Sergey Bronnikov; +Cc: tarantool-patches 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 files are set to <src/lj.supp> (original suppression file in LuaJIT) and an additional one <src/lj_extra.supp> (maintained by us). 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 +++++ src/lj_extra.supp | 19 +++++++++++++++++++ test/CMakeLists.txt | 19 +++++++++++++++++++ test/tarantool-c-tests/CMakeLists.txt | 11 ++++++++++- .../gh-8594-sysprof-ffunc-crash.test.c | 9 +++++++++ test/tarantool-tests/CMakeLists.txt | 6 ++++++ .../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 ++ 13 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 src/lj_extra.supp 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/src/lj_extra.supp b/src/lj_extra.supp new file mode 100644 index 00000000..ac205f03 --- /dev/null +++ b/src/lj_extra.supp @@ -0,0 +1,19 @@ +# Valgrind suppression file maintained by the Tarantool team. + +# Missed from the original suppression file. +# Unaligned access to the string, see <lj_str.c> for the details. +{ + Optimized string compare + Memcheck:Addr4 + fun:lj_getu32 + fun:str_fastcmp +} + +# Missed from the original suppression file. +# `lj_str_cmp()` may read up to 3 extra bytes from the end of the +# string. It is OK due to the aligned allocations. +{ + Optimized string compare + Memcheck:Cond + fun:lj_str_cmp +} diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0db2dd8b..bda85ec1 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -69,6 +69,25 @@ 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) + list(APPEND LUAJIT_TEST_VALGRIND_SUPP + --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp + --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp + ) + set(LUAJIT_TEST_COMMAND + "${VALGRIND} --error-exitcode=1 " + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") +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..5f6c45da 100644 --- a/test/tarantool-c-tests/CMakeLists.txt +++ b/test/tarantool-c-tests/CMakeLists.txt @@ -56,10 +56,19 @@ 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} --error-exitcode=1 ${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..35108e77 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,12 @@ static int tracer(pid_t chpid) static int test_tostring_call(void *ctx) { +#if LUAJIT_USE_VALGRIND + UNUSED(ctx); + UNUSED(tracer); + UNUSED(tracee); + return skip("Disabled with Valgrind (Timeout)"); +#else pid_t chpid = fork(); switch(chpid) { case -1: @@ -264,6 +272,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/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt index 8bdb2cf3..848c4cab 100644 --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -89,6 +89,12 @@ if(LUAJIT_ENABLE_TABLE_BUMP) list(APPEND LUA_TEST_ENV_MORE LUAJIT_TABLE_BUMP=1) endif() +# Needed to skip some long tests or tests using signals. +# See also https://github.com/tarantool/tarantool/issues/10803. +if(LUAJIT_USE_VALGRIND) + list(APPEND LUA_TEST_ENV_MORE LUAJIT_TEST_USE_VALGRIND=1) +endif() + set(TEST_SUITE_NAME "tarantool-tests") # XXX: The call produces both test and target 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.47.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind Sergey Kaplun via Tarantool-patches @ 2024-12-13 13:18 ` Sergey Bronnikov via Tarantool-patches 2024-12-16 16:40 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 14+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-12-13 13:18 UTC (permalink / raw) To: Sergey Kaplun, Maksim Tiushev; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 13992 bytes --] Hi, Sergey, thanks for the patch! Please see comments below On 11.12.2024 16:21, Sergey Kaplun wrote: > 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 s/valgrind/Valgrind/ > command line (not at the building stage). By default, the suppression There is also a config file .valgrindrc [1], why env variable is preffered? [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts > files are set to <src/lj.supp> (original suppression file in LuaJIT) and > an additional one <src/lj_extra.supp> (maintained by us). > > Also, this patch disables the following tests when running with Valgrind > due to failures: Please add a ticket url to commit message (https://github.com/tarantool/tarantool/issues/10803). > > 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 this test is missed in a ticket description, please add > > 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 +++++ > src/lj_extra.supp | 19 +++++++++++++++++++ > test/CMakeLists.txt | 19 +++++++++++++++++++ > test/tarantool-c-tests/CMakeLists.txt | 11 ++++++++++- > .../gh-8594-sysprof-ffunc-crash.test.c | 9 +++++++++ > test/tarantool-tests/CMakeLists.txt | 6 ++++++ > .../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 ++ > 13 files changed, 84 insertions(+), 3 deletions(-) > create mode 100644 src/lj_extra.supp > > 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/src/lj_extra.supp b/src/lj_extra.supp > new file mode 100644 > index 00000000..ac205f03 > --- /dev/null > +++ b/src/lj_extra.supp > @@ -0,0 +1,19 @@ > +# Valgrind suppression file maintained by the Tarantool team. > + > +# Missed from the original suppression file. > +# Unaligned access to the string, see <lj_str.c> for the details. > +{ > + Optimized string compare > + Memcheck:Addr4 > + fun:lj_getu32 > + fun:str_fastcmp > +} > + > +# Missed from the original suppression file. > +# `lj_str_cmp()` may read up to 3 extra bytes from the end of the > +# string. It is OK due to the aligned allocations. > +{ > + Optimized string compare > + Memcheck:Cond > + fun:lj_str_cmp > +} > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 0db2dd8b..bda85ec1 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -69,6 +69,25 @@ 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) Please check that VALGRIND variable is not empty and put this condition before "if (NOT LUAJIT_USE_SYSMALLOC)" > + list(APPEND LUAJIT_TEST_VALGRIND_SUPP > + --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp > + --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp > + ) > + set(LUAJIT_TEST_COMMAND > + "${VALGRIND} --error-exitcode=1 " please add a comment why --error-exitcode is used. From a manual page: --error-exitcode=<number> [default: 0] Specifies an alternative exit code to return if Valgrind reported any errors in the run. When set to the default value (zero), the return value from Valgrind will always be the return value of the process being simulated. When set to a nonzero value, that value is returned instead, if Valgrind detects any errors. This is useful for using Valgrind as part of an automated test suite, since it makes it easy to detect test cases for which Valgrind has reported errors, just by inspecting return codes. > + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") With "list(APPEND BENCHMARK_CMAKE_FLAGS "AA" "BB")" you don't need trailing whitespaces. Please replace "set()" with "list(APPEND ... )" like above. > +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..5f6c45da 100644 > --- a/test/tarantool-c-tests/CMakeLists.txt > +++ b/test/tarantool-c-tests/CMakeLists.txt > @@ -56,10 +56,19 @@ 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} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP} The same already defined in test/CMakeLists.txt, could you reuse already defined value? > + ${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..35108e77 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,12 @@ static int tracer(pid_t chpid) > > static int test_tostring_call(void *ctx) > { > +#if LUAJIT_USE_VALGRIND > + UNUSED(ctx); > + UNUSED(tracer); > + UNUSED(tracee); > + return skip("Disabled with Valgrind (Timeout)"); > +#else > pid_t chpid = fork(); > switch(chpid) { > case -1: > @@ -264,6 +272,7 @@ static int test_tostring_call(void *ctx) > default: > return tracer(chpid); > } > +#endif I propose to use a testsuite specific timeout for tarantool-c-tests and increase it's value instead of disabling tests. Hint: we can set increased timeout under option LUAJIT_USE_VALGRIND only. > } > > #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */ > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > index 8bdb2cf3..848c4cab 100644 > --- a/test/tarantool-tests/CMakeLists.txt > +++ b/test/tarantool-tests/CMakeLists.txt > @@ -89,6 +89,12 @@ if(LUAJIT_ENABLE_TABLE_BUMP) > list(APPEND LUA_TEST_ENV_MORE LUAJIT_TABLE_BUMP=1) > endif() > > +# Needed to skip some long tests or tests using signals. > +# See alsohttps://github.com/tarantool/tarantool/issues/10803. > +if(LUAJIT_USE_VALGRIND) > + list(APPEND LUA_TEST_ENV_MORE LUAJIT_TEST_USE_VALGRIND=1) > +endif() > + > set(TEST_SUITE_NAME "tarantool-tests") > > # XXX: The call produces both test and target > 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 alsohttps://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 alsohttps://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 alsohttps://github.com/LuaJIT/LuaJIT/issues/606. > ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'), > + -- See alsohttps://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 alsohttps://github.com/LuaJIT/LuaJIT/issues/606. > ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'), > + -- See alsohttps://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 alsohttps://github.com/LuaJIT/LuaJIT/issues/606. > ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"), > + -- See alsohttps://github.com/tarantool/tarantool/issues/10803. > + ['Disabled due to #10803'] = os.getenv("LUAJIT_TEST_USE_VALGRIND"), > }) > > test:plan(19) [-- Attachment #2: Type: text/html, Size: 17363 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind 2024-12-13 13:18 ` Sergey Bronnikov via Tarantool-patches @ 2024-12-16 16:40 ` Sergey Kaplun via Tarantool-patches 2024-12-17 11:42 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 14+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-12-16 16:40 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: Maksim Tiushev, tarantool-patches Hi, Sergey! Thansk for the review! See my answers below, all patches are force-pushed to the branch. The branch is rebased on master. On 13.12.24, Sergey Bronnikov wrote: > Hi, Sergey, > > thanks for the patch! Please see comments below > > On 11.12.2024 16:21, Sergey Kaplun wrote: > > 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 > s/valgrind/Valgrind/ Fixed, thanks! > > command line (not at the building stage). By default, the suppression > > There is also a config file .valgrindrc [1], why env variable is preffered? > > [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts It is more flexible. For example, we may easily test different configurations locally for the single build without changing any sources. > > > files are set to <src/lj.supp> (original suppression file in LuaJIT) and > > an additional one <src/lj_extra.supp> (maintained by us). > > > > Also, this patch disables the following tests when running with Valgrind > > due to failures: > > Please add a ticket url to commit message > (https://github.com/tarantool/tarantool/issues/10803). It is already mentioned (see the line below). > > > > > 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 Updated the commit message regarding this test. > > - 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 > this test is missed in a ticket description, please add Added. > > > > 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 | cmake: run tests with Valgrind | | 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 | files are set to <src/lj.supp> (original suppression file in LuaJIT) and | an additional one <src/lj_extra.supp> (maintained by us). | | Also, this patch disables the following tests when running with Valgrind | due to failures. | | The <tarantool-tests/lj-512-profiler-hook-finalizers.test.lua> test is | disabled due to its time sensitivity (it is not run the expected amount | of time with Valgrind). | | These tests from the tarantool-tests suite are disabled due to | tarantool/tarantool#10803: | - lj-726-profile-flush-close.test.lua | - profilers/gh-5688-tool-cli-flag.test.lua | - profilers/gh-7264-add-proto-trace-sysprof-default.test.lua | - profilers/misclib-sysprof-lapi.test.lua | | Timed out due to running under Valgrind: | - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test | - tarantool-tests/gh-7745-oom-on-trace.test.lua | - tarantool-tests/lj-1034-tabov-error-frame.test.lua | | [1]: https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts | | Part of tarantool/tarantool#3705 > > --- > > CMakeLists.txt | 5 +++++ > > src/lj_extra.supp | 19 +++++++++++++++++++ > > test/CMakeLists.txt | 19 +++++++++++++++++++ > > test/tarantool-c-tests/CMakeLists.txt | 11 ++++++++++- > > .../gh-8594-sysprof-ffunc-crash.test.c | 9 +++++++++ > > test/tarantool-tests/CMakeLists.txt | 6 ++++++ > > .../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 ++ > > 13 files changed, 84 insertions(+), 3 deletions(-) > > create mode 100644 src/lj_extra.supp > > <snipped> > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > > index 0db2dd8b..bda85ec1 100644 > > --- a/test/CMakeLists.txt > > +++ b/test/CMakeLists.txt > > @@ -69,6 +69,25 @@ 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) Fixed indentation. =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index bda85ec1..4590e065 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -71,7 +71,7 @@ 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) + 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" =================================================================== > > + 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) > > Please check that VALGRIND variable is not empty > > and put this condition before "if (NOT LUAJIT_USE_SYSMALLOC)" Added, now the error is raised in that case: =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 4590e065..d9cab615 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -71,6 +71,12 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") if(LUAJIT_USE_VALGRIND) + find_program(VALGRIND valgrind) + if(NOT VALGRIND) + message(FATAL_ERROR "`valgrind' not found when LUAJIT_USE_VALGRIND option " + "is enabled.") + endif() + if(NOT LUAJIT_USE_SYSMALLOC) message(WARNING "LUAJIT_USE_SYSMALLOC option is mandatory for Valgrind's memcheck tool" @@ -78,7 +84,6 @@ if(LUAJIT_USE_VALGRIND) " architectures.") endif() - find_program(VALGRIND valgrind) list(APPEND LUAJIT_TEST_VALGRIND_SUPP --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp =================================================================== > > > + list(APPEND LUAJIT_TEST_VALGRIND_SUPP > > + --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp > > + --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp > > + ) > > + set(LUAJIT_TEST_COMMAND > > + "${VALGRIND} --error-exitcode=1 " > > please add a comment why --error-exitcode is used. From a manual page: > > --error-exitcode=<number> [default: 0] > Specifies an alternative exit code to return if Valgrind > reported any errors in the run. When set to the default value (zero), > the return value from Valgrind will always be the return value of the > process being simulated. When set to a nonzero value, that value is > returned instead, if Valgrind detects any errors. This is useful for > using Valgrind as part of an automated test suite, since it makes it > easy to detect test cases for which Valgrind has reported errors, just > by inspecting return codes. Added the comment. =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index d9cab615..9685c494 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -88,6 +88,8 @@ if(LUAJIT_USE_VALGRIND) --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp ) + # Set the exit code to non-zero to automatically detect + # failing tests. set(LUAJIT_TEST_COMMAND "${VALGRIND} --error-exitcode=1 " "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") =================================================================== > > > + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > > With "list(APPEND BENCHMARK_CMAKE_FLAGS "AA" "BB")" you don't need > trailing whitespaces. > > Please replace "set()" with "list(APPEND ... )" like above. I suppose you mean `list(PREPEND ...)` [1] (since `APPEND` inserts elements into the end of the list), but this is available only from CMake version 3.15, while the minimum supported version is 2.8. So, ignored for now. [1]: https://cmake.org/cmake/help/latest/command/list.html#prepend > > > +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..5f6c45da 100644 > > --- a/test/tarantool-c-tests/CMakeLists.txt > > +++ b/test/tarantool-c-tests/CMakeLists.txt > > @@ -56,10 +56,19 @@ 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} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP} > The same already defined in test/CMakeLists.txt, could you reuse already > defined value? Yes, good catch! =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 9685c494..fcd697c9 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -90,9 +90,10 @@ if(LUAJIT_USE_VALGRIND) ) # Set the exit code to non-zero to automatically detect # failing tests. + set(LUAJIT_TEST_VALGRIND_COMMAND + ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) set(LUAJIT_TEST_COMMAND - "${VALGRIND} --error-exitcode=1 " - "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") endif() separate_arguments(LUAJIT_TEST_COMMAND) diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt index 5f6c45da..c4a402d0 100644 --- a/test/tarantool-c-tests/CMakeLists.txt +++ b/test/tarantool-c-tests/CMakeLists.txt @@ -59,9 +59,7 @@ foreach(test_source ${tests}) set(test_command ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}) if(LUAJIT_USE_VALGRIND) - set(test_command ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP} - ${test_command} - ) + set(test_command ${LUAJIT_TEST_VALGRIND_COMMAND} ${test_command}) endif() add_test(NAME ${test_title} =================================================================== > > + ${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..35108e77 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,12 @@ static int tracer(pid_t chpid) > > > > static int test_tostring_call(void *ctx) > > { > > +#if LUAJIT_USE_VALGRIND > > + UNUSED(ctx); > > + UNUSED(tracer); > > + UNUSED(tracee); > > + return skip("Disabled with Valgrind (Timeout)"); > > +#else > > pid_t chpid = fork(); > > switch(chpid) { > > case -1: > > @@ -264,6 +272,7 @@ static int test_tostring_call(void *ctx) > > default: > > return tracer(chpid); > > } > > +#endif > > I propose to use a testsuite specific timeout for tarantool-c-tests > > and increase it's value instead of disabling tests. > > Hint: we can set increased timeout under option LUAJIT_USE_VALGRIND only. The problem is that this test will run for too long, so it is just easier to skip it. > > > } > > > > #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */ <snipped> -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind 2024-12-16 16:40 ` Sergey Kaplun via Tarantool-patches @ 2024-12-17 11:42 ` Sergey Bronnikov via Tarantool-patches 2024-12-17 12:17 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 14+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-12-17 11:42 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Maksim Tiushev, tarantool-patches [-- Attachment #1: Type: text/plain, Size: 8373 bytes --] Hello, Thanks for fixes! LGTM after fixing minor comments below. On 16.12.2024 19:40, Sergey Kaplun wrote: > Hi, Sergey! > Thansk for the review! > See my answers below, all patches are force-pushed to the branch. > The branch is rebased on master. > > On 13.12.24, Sergey Bronnikov wrote: > <snipped> > | cmake: run tests with Valgrind > | > | 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 > | files are set to <src/lj.supp> (original suppression file in LuaJIT) and > | an additional one <src/lj_extra.supp> (maintained by us). > | > | Also, this patch disables the following tests when running with Valgrind > | due to failures. please replace dot with colon, because below the enumeration of disabled tests > | > | The <tarantool-tests/lj-512-profiler-hook-finalizers.test.lua> test is > | disabled due to its time sensitivity (it is not run the expected amount > | of time with Valgrind). > | > | These tests from the tarantool-tests suite are disabled due to > | tarantool/tarantool#10803: > | - lj-726-profile-flush-close.test.lua > | - profilers/gh-5688-tool-cli-flag.test.lua > | - profilers/gh-7264-add-proto-trace-sysprof-default.test.lua > | - profilers/misclib-sysprof-lapi.test.lua > | > | Timed out due to running under Valgrind: > | - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test > | - tarantool-tests/gh-7745-oom-on-trace.test.lua > | - tarantool-tests/lj-1034-tabov-error-frame.test.lua > | > | [1]:https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts > | > | Part of tarantool/tarantool#3705 > >>> --- >>> CMakeLists.txt | 5 +++++ >>> src/lj_extra.supp | 19 +++++++++++++++++++ >>> test/CMakeLists.txt | 19 +++++++++++++++++++ >>> test/tarantool-c-tests/CMakeLists.txt | 11 ++++++++++- >>> .../gh-8594-sysprof-ffunc-crash.test.c | 9 +++++++++ >>> test/tarantool-tests/CMakeLists.txt | 6 ++++++ >>> .../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 ++ >>> 13 files changed, 84 insertions(+), 3 deletions(-) >>> create mode 100644 src/lj_extra.supp >>> <snipped> > Added the comment. > =================================================================== > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index d9cab615..9685c494 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -88,6 +88,8 @@ if(LUAJIT_USE_VALGRIND) > --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp > --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp > ) > + # Set the exit code to non-zero to automatically detect > + # failing tests. because "when set to the default value (zero), the return value from Valgrind will always be the return value of the process being simulated.", I believe it is an important thing, and it's worth to add to comment. > set(LUAJIT_TEST_COMMAND > "${VALGRIND} --error-exitcode=1 " > "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > =================================================================== > >>> + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") >> With "list(APPEND BENCHMARK_CMAKE_FLAGS "AA" "BB")" you don't need >> trailing whitespaces. >> >> Please replace "set()" with "list(APPEND ... )" like above. > I suppose you mean `list(PREPEND ...)` [1] (since `APPEND` inserts > elements into the end of the list), but this is available only from > CMake version 3.15, while the minimum supported version is 2.8. So, > ignored for now. > > [1]:https://cmake.org/cmake/help/latest/command/list.html#prepend > >>> +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..5f6c45da 100644 >>> --- a/test/tarantool-c-tests/CMakeLists.txt >>> +++ b/test/tarantool-c-tests/CMakeLists.txt >>> @@ -56,10 +56,19 @@ 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} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP} >> The same already defined in test/CMakeLists.txt, could you reuse already >> defined value? > Yes, good catch! > =================================================================== > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 9685c494..fcd697c9 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -90,9 +90,10 @@ if(LUAJIT_USE_VALGRIND) > ) > # Set the exit code to non-zero to automatically detect > # failing tests. > + set(LUAJIT_TEST_VALGRIND_COMMAND > + ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) bad indentation > set(LUAJIT_TEST_COMMAND > - "${VALGRIND} --error-exitcode=1 " > - "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") bad indentation > endif() > > separate_arguments(LUAJIT_TEST_COMMAND) > diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt > index 5f6c45da..c4a402d0 100644 > --- a/test/tarantool-c-tests/CMakeLists.txt > +++ b/test/tarantool-c-tests/CMakeLists.txt > @@ -59,9 +59,7 @@ foreach(test_source ${tests}) > set(test_command ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}) > > if(LUAJIT_USE_VALGRIND) > - set(test_command ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP} > - ${test_command} > - ) > + set(test_command ${LUAJIT_TEST_VALGRIND_COMMAND} ${test_command}) > endif() > > add_test(NAME ${test_title} > =================================================================== > >>> + ${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..35108e77 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,12 @@ static int tracer(pid_t chpid) >>> >>> static int test_tostring_call(void *ctx) >>> { >>> +#if LUAJIT_USE_VALGRIND >>> + UNUSED(ctx); >>> + UNUSED(tracer); >>> + UNUSED(tracee); >>> + return skip("Disabled with Valgrind (Timeout)"); >>> +#else >>> pid_t chpid = fork(); >>> switch(chpid) { >>> case -1: >>> @@ -264,6 +272,7 @@ static int test_tostring_call(void *ctx) >>> default: >>> return tracer(chpid); >>> } >>> +#endif >> I propose to use a testsuite specific timeout for tarantool-c-tests >> >> and increase it's value instead of disabling tests. >> >> Hint: we can set increased timeout under option LUAJIT_USE_VALGRIND only. > The problem is that this test will run for too long, so it is just > easier to skip it. > Okay. >>> } >>> >>> #else /* LUAJIT_OS == LUAJIT_OS_LINUX && LUAJIT_TARGET == LUAJIT_ARCH_X64 */ > <snipped> > [-- Attachment #2: Type: text/html, Size: 10835 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind 2024-12-17 11:42 ` Sergey Bronnikov via Tarantool-patches @ 2024-12-17 12:17 ` Sergey Kaplun via Tarantool-patches 2024-12-17 19:31 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 14+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-12-17 12:17 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: Maksim Tiushev, tarantool-patches Hi, Sergey! Thanks for the comments! Fixed them and force-pushed the branch. On 17.12.24, Sergey Bronnikov wrote: > Hello, > > Thanks for fixes! > > LGTM after fixing minor comments below. > > On 16.12.2024 19:40, Sergey Kaplun wrote: > > Hi, Sergey! > > Thansk for the review! > > See my answers below, all patches are force-pushed to the branch. > > The branch is rebased on master. > > > > On 13.12.24, Sergey Bronnikov wrote: > > > <snipped> > > | cmake: run tests with Valgrind > > | > > | 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 > > | files are set to <src/lj.supp> (original suppression file in LuaJIT) and > > | an additional one <src/lj_extra.supp> (maintained by us). > > | > > | Also, this patch disables the following tests when running with Valgrind > > | due to failures. > please replace dot with colon, because below the enumeration of disabled > tests Replaced. > > | > > | The <tarantool-tests/lj-512-profiler-hook-finalizers.test.lua> test is > > | disabled due to its time sensitivity (it is not run the expected amount > > | of time with Valgrind). > > | > > | These tests from the tarantool-tests suite are disabled due to > > | tarantool/tarantool#10803: > > | - lj-726-profile-flush-close.test.lua > > | - profilers/gh-5688-tool-cli-flag.test.lua > > | - profilers/gh-7264-add-proto-trace-sysprof-default.test.lua > > | - profilers/misclib-sysprof-lapi.test.lua > > | > > | Timed out due to running under Valgrind: > > | - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test > > | - tarantool-tests/gh-7745-oom-on-trace.test.lua > > | - tarantool-tests/lj-1034-tabov-error-frame.test.lua > > | > > | [1]:https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts > > | > > | Part of tarantool/tarantool#3705 > > <snipped> > > ) > > + # Set the exit code to non-zero to automatically detect > > + # failing tests. > because "when set to the default value (zero), > the return value from Valgrind will always be the return value of the > process being simulated.", I believe it is an important thing, and it's > worth to add to comment. Enriched the comment: =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a5075f05..87e4b907 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -88,8 +88,10 @@ if(LUAJIT_USE_VALGRIND) --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp ) - # Set the exit code to non-zero to automatically detect - # failing tests. + # When set to the default value (zero), the return value from + # Valgrind will always be the return value of the process being + # simulated. Set the exit code to non-zero to automatically + # detect failing tests. set(LUAJIT_TEST_VALGRIND_COMMAND ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) set(LUAJIT_TEST_COMMAND =================================================================== > > set(LUAJIT_TEST_COMMAND > > "${VALGRIND} --error-exitcode=1 " > > "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > > =================================================================== > > > >>> + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") <snipped> > >>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt > >>> index 30d174bb..5f6c45da 100644 > >>> --- a/test/tarantool-c-tests/CMakeLists.txt > >>> +++ b/test/tarantool-c-tests/CMakeLists.txt > >>> @@ -56,10 +56,19 @@ 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} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP} > >> The same already defined in test/CMakeLists.txt, could you reuse already > >> defined value? > > Yes, good catch! > > =================================================================== > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > > index 9685c494..fcd697c9 100644 > > --- a/test/CMakeLists.txt > > +++ b/test/CMakeLists.txt > > @@ -90,9 +90,10 @@ if(LUAJIT_USE_VALGRIND) > > ) > > # Set the exit code to non-zero to automatically detect > > # failing tests. > > + set(LUAJIT_TEST_VALGRIND_COMMAND > > + ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) > bad indentation > > set(LUAJIT_TEST_COMMAND > > - "${VALGRIND} --error-exitcode=1 " > > - "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > > + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") > bad indentation Added 2 addtional spaces: =================================================================== diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index fcd697c9..a5075f05 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -91,9 +91,9 @@ if(LUAJIT_USE_VALGRIND) # Set the exit code to non-zero to automatically detect # failing tests. set(LUAJIT_TEST_VALGRIND_COMMAND - ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) + ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) set(LUAJIT_TEST_COMMAND - "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") endif() separate_arguments(LUAJIT_TEST_COMMAND) =================================================================== > > endif() > > > > separate_arguments(LUAJIT_TEST_COMMAND) <snipped> > > -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind 2024-12-17 12:17 ` Sergey Kaplun via Tarantool-patches @ 2024-12-17 19:31 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 0 replies; 14+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-12-17 19:31 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Maksim Tiushev, tarantool-patches [-- Attachment #1: Type: text/plain, Size: 6356 bytes --] Thanks for the fixes! LGTM BTW, I've discovered that CMake has integration with memory checking tools, see [1][2]. [1]: https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-memcheck-step [2]: https://cmake.org/cmake/help/latest/variable/CTEST_MEMORYCHECK_TYPE.html#variable:CTEST_MEMORYCHECK_TYPE On 17.12.2024 15:17, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the comments! > Fixed them and force-pushed the branch. > > On 17.12.24, Sergey Bronnikov wrote: >> Hello, >> >> Thanks for fixes! >> >> LGTM after fixing minor comments below. >> >> On 16.12.2024 19:40, Sergey Kaplun wrote: >>> Hi, Sergey! >>> Thansk for the review! >>> See my answers below, all patches are force-pushed to the branch. >>> The branch is rebased on master. >>> >>> On 13.12.24, Sergey Bronnikov wrote: >>> >> <snipped> >>> | cmake: run tests with Valgrind >>> | >>> | 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 >>> | files are set to <src/lj.supp> (original suppression file in LuaJIT) and >>> | an additional one <src/lj_extra.supp> (maintained by us). >>> | >>> | Also, this patch disables the following tests when running with Valgrind >>> | due to failures. >> please replace dot with colon, because below the enumeration of disabled >> tests > Replaced. > >>> | >>> | The <tarantool-tests/lj-512-profiler-hook-finalizers.test.lua> test is >>> | disabled due to its time sensitivity (it is not run the expected amount >>> | of time with Valgrind). >>> | >>> | These tests from the tarantool-tests suite are disabled due to >>> | tarantool/tarantool#10803: >>> | - lj-726-profile-flush-close.test.lua >>> | - profilers/gh-5688-tool-cli-flag.test.lua >>> | - profilers/gh-7264-add-proto-trace-sysprof-default.test.lua >>> | - profilers/misclib-sysprof-lapi.test.lua >>> | >>> | Timed out due to running under Valgrind: >>> | - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test >>> | - tarantool-tests/gh-7745-oom-on-trace.test.lua >>> | - tarantool-tests/lj-1034-tabov-error-frame.test.lua >>> | >>> | [1]:https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts >>> | >>> | Part of tarantool/tarantool#3705 >>> > <snipped> > >>> ) >>> + # Set the exit code to non-zero to automatically detect >>> + # failing tests. >> because "when set to the default value (zero), >> the return value from Valgrind will always be the return value of the >> process being simulated.", I believe it is an important thing, and it's >> worth to add to comment. > Enriched the comment: > =================================================================== > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index a5075f05..87e4b907 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -88,8 +88,10 @@ if(LUAJIT_USE_VALGRIND) > --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp > --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp > ) > - # Set the exit code to non-zero to automatically detect > - # failing tests. > + # When set to the default value (zero), the return value from > + # Valgrind will always be the return value of the process being > + # simulated. Set the exit code to non-zero to automatically > + # detect failing tests. > set(LUAJIT_TEST_VALGRIND_COMMAND > ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) > set(LUAJIT_TEST_COMMAND > =================================================================== > >>> set(LUAJIT_TEST_COMMAND >>> "${VALGRIND} --error-exitcode=1 " >>> "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") >>> =================================================================== >>> >>>>> + "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") > <snipped> > >>>>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt >>>>> index 30d174bb..5f6c45da 100644 >>>>> --- a/test/tarantool-c-tests/CMakeLists.txt >>>>> +++ b/test/tarantool-c-tests/CMakeLists.txt >>>>> @@ -56,10 +56,19 @@ 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} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP} >>>> The same already defined in test/CMakeLists.txt, could you reuse already >>>> defined value? >>> Yes, good catch! >>> =================================================================== >>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt >>> index 9685c494..fcd697c9 100644 >>> --- a/test/CMakeLists.txt >>> +++ b/test/CMakeLists.txt >>> @@ -90,9 +90,10 @@ if(LUAJIT_USE_VALGRIND) >>> ) >>> # Set the exit code to non-zero to automatically detect >>> # failing tests. >>> + set(LUAJIT_TEST_VALGRIND_COMMAND >>> + ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) >> bad indentation >>> set(LUAJIT_TEST_COMMAND >>> - "${VALGRIND} --error-exitcode=1 " >>> - "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}") >>> + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") >> bad indentation > Added 2 addtional spaces: > > =================================================================== > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index fcd697c9..a5075f05 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -91,9 +91,9 @@ if(LUAJIT_USE_VALGRIND) > # Set the exit code to non-zero to automatically detect > # failing tests. > set(LUAJIT_TEST_VALGRIND_COMMAND > - ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) > + ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}) > set(LUAJIT_TEST_COMMAND > - "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") > + "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}") > endif() > > separate_arguments(LUAJIT_TEST_COMMAND) > =================================================================== > >>> endif() >>> >>> separate_arguments(LUAJIT_TEST_COMMAND) > <snipped> > [-- Attachment #2: Type: text/html, Size: 9126 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow 2024-12-11 13:21 [Tarantool-patches] [PATCH v5 luajit 0/3] Valgrind testing Sergey Kaplun via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions Sergey Kaplun via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind Sergey Kaplun via Tarantool-patches @ 2024-12-11 13:21 ` Sergey Kaplun via Tarantool-patches 2024-12-13 13:23 ` Sergey Bronnikov via Tarantool-patches 2024-12-22 7:44 ` mandesero--- via Tarantool-patches 2 siblings, 2 replies; 14+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-12-11 13:21 UTC (permalink / raw) To: Maksim Tiushev, Sergey Bronnikov; +Cc: tarantool-patches From: Maksim Tiushev <mandesero@gmail.com> This patch adds CI testing with Valgrind in three scenarios: - Full leak 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 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 | 102 ++++++++++++++++++++++ 3 files changed, 126 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..e6606478 --- /dev/null +++ b/.github/workflows/valgrind-testing.yaml @@ -0,0 +1,102 @@ +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 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 + 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 + JOB_POSTFIX: "malloc/free-fill: 0x00" + - VALGRIND_SCENARIO: malloc-free-fill-0xff + VALGRIND_OPTS: --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, the internal + # LuaJIT memory allocator is not instrumented yet, so to + # find any memory errors, it's better to build LuaJIT with + # the 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. + 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 + env: + VALGRIND_OPTS: ${{ matrix.VALGRIND_OPTS }} + run: cmake --build . --parallel --target LuaJIT-test + working-directory: ${{ env.BUILDDIR }} -- 2.47.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow Sergey Kaplun via Tarantool-patches @ 2024-12-13 13:23 ` Sergey Bronnikov via Tarantool-patches 2024-12-22 7:44 ` mandesero--- via Tarantool-patches 1 sibling, 0 replies; 14+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-12-13 13:23 UTC (permalink / raw) To: Sergey Kaplun, Maksim Tiushev; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 7101 bytes --] Hi, Sergey, thanks for the patch! LGTM On 11.12.2024 16:21, Sergey Kaplun wrote: > From: Maksim Tiushev<mandesero@gmail.com> > > This patch adds CI testing with Valgrind in three scenarios: > - Full leak 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 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 | 102 ++++++++++++++++++++++ > 3 files changed, 126 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..e6606478 > --- /dev/null > +++ b/.github/workflows/valgrind-testing.yaml > @@ -0,0 +1,102 @@ > +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 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 > + 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 > + JOB_POSTFIX: "malloc/free-fill: 0x00" > + - VALGRIND_SCENARIO: malloc-free-fill-0xff > + VALGRIND_OPTS: --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, the internal > + # LuaJIT memory allocator is not instrumented yet, so to > + # find any memory errors, it's better to build LuaJIT with > + # the 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. > + 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 > + env: > + VALGRIND_OPTS: ${{ matrix.VALGRIND_OPTS }} > + run: cmake --build . --parallel --target LuaJIT-test > + working-directory: ${{ env.BUILDDIR }} [-- Attachment #2: Type: text/html, Size: 7342 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow Sergey Kaplun via Tarantool-patches 2024-12-13 13:23 ` Sergey Bronnikov via Tarantool-patches @ 2024-12-22 7:44 ` mandesero--- via Tarantool-patches 1 sibling, 0 replies; 14+ messages in thread From: mandesero--- via Tarantool-patches @ 2024-12-22 7:44 UTC (permalink / raw) To: tarantool-patches; +Cc: mandesero Hi, Sergey! Thanks for the patch! I see no problem, LGTM ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-12-22 7:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-12-11 13:21 [Tarantool-patches] [PATCH v5 luajit 0/3] Valgrind testing Sergey Kaplun via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions Sergey Kaplun via Tarantool-patches 2024-12-13 12:54 ` Sergey Bronnikov via Tarantool-patches 2024-12-16 11:24 ` Sergey Kaplun via Tarantool-patches 2024-12-17 11:08 ` Sergey Bronnikov via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind Sergey Kaplun via Tarantool-patches 2024-12-13 13:18 ` Sergey Bronnikov via Tarantool-patches 2024-12-16 16:40 ` Sergey Kaplun via Tarantool-patches 2024-12-17 11:42 ` Sergey Bronnikov via Tarantool-patches 2024-12-17 12:17 ` Sergey Kaplun via Tarantool-patches 2024-12-17 19:31 ` Sergey Bronnikov via Tarantool-patches 2024-12-11 13:21 ` [Tarantool-patches] [PATCH v5 luajit 3/3] ci: add Valgrind testing workflow Sergey Kaplun via Tarantool-patches 2024-12-13 13:23 ` Sergey Bronnikov via Tarantool-patches 2024-12-22 7:44 ` mandesero--- 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