Thanks for updates, LGTM.
On 26 Oct 2022, at 16:42, Sergey Kaplun <skaplun@tarantool.org> wrote:
Hi, Sergos!Thanks for the review!On 25.10.22, sergos wrote:Hi!
Thanks for the patch!
Some comments on the commit message and the test showed flaky behavior,
it failed only 2 times out of 10 runs using the
Tarantool 2.11.0-entrypoint-637-gdd7d46af3 on a
Darwin s-ostanevich2 22.1.0 Darwin Kernel Version 22.1.0
OMG, `tap` in tarantool is slightly different so the test is flaky.Fixes with increasing amount of calls of `fibb()` function.Also, removes `jit.bc.dump()` output.See the iterative patch below.===================================================================diff --git a/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua b/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.luaindex b5146f70..475d9200 100644--- a/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua+++ b/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua@@ -18,11 +18,16 @@ local function fibb(n) return n < 2 and n or fibb(n - 1) + fibb(n - 2)end+local function empty() end-- Compile `jit.bc` functions, that are used in vmevent handler.-require('jit.bc').dump(loadstring(string.dump(fibb)))+require('jit.bc').dump(loadstring(string.dump(fibb)), {+ write = empty,+ close = empty,+ flush = empty,+})-- Here we dump (to /dev/null) info about `fibb()` traces and run-- `jit.bc` functions inside.-test:ok(fibb(0) == 0, 'run compiled function inside vmevent handler')+test:ok(fibb(2) == 1, 'run compiled function inside vmevent handler')os.exit(test:check() and 0 or 1)===================================================================
Regards,
Sergos
On 19 Oct 2022, at 15:37, Sergey Kaplun <skaplun@tarantool.org> wrote:
From: Mike Pall <mike>
Reported by Sergey Kaplun.
(cherry picked from commit b5b20191f3a8a2e2d28f1362b11bd26a51083d89)
`J->exitno` means the number of an exit from a currently executed trace
The can have two meanings: an exit or a trace to stitch after a C call.
Fixed.
and also a number of a trace with which we want to stitch after C
function execution. If part of a function has been compiled before a
^^^^^^^ is it about a C one?
No, it's about Lua function inside vmevent handler (in bc.dump() forexample) that called on routine to be compiled later.Possibly its better to say: if an vmevent handler itself has a trace
compiled then execution of this trace can change the `J->exitno` from the
original trace we plan to stitch into an taken exit from the handler’s trace.
Yes, It's really better:)! Thanks!
vmevent handler was called, its trace is used during the vmevent handler
on recording. When recording trace to stitch `J->exitno` is set to the
previously executed trace. After running trace in the vmevent handler
and exit it by a snapshot `J->exitno` is set to the number of exit
instead number of a trace to stitch.
It's the moment when stitching is
broken.
This patch saves all necessary fields (`J->exitno`, `J->parent`), which
And what about `J-parent`? Isn’t it a trace to stitch to?
Yes, it's a parent trace for the currently executed one.
may change during vmevent handler execution, before vmevent call and
restores them after.
Sergey Kaplun:
* added the description and the test for the problem
Resolves tarantool/tarantool#6782
Part of tarantool/tarantool#7230
---
The updated commit message is the following:| Save trace recorder state around VM event call.|| Reported by Sergey Kaplun.|| (cherry picked from commit b5b20191f3a8a2e2d28f1362b11bd26a51083d89)|| The `J->exitno` can have two meanings: an exit or a trace to stitch| after a C call.|| If an vmevent handler itself has a trace compiled, then an execution of| this trace can change the `J->exitno` from the original trace, we plan| to stitch, into an taken exit from the handler’s trace. It's the moment| when stitching is broken.|| This patch saves all necessary fields (`J->exitno`, `J->parent`), which| may change during vmevent handler execution, before vmevent call and| restores them after.|| Sergey Kaplun:| * added the description and the test for the problem|| Resolves tarantool/tarantool#6782| Part of tarantool/tarantool#7230Branch is force pushed, PR is updated.
Issues:
* https://github.com/tarantool/tarantool/issues/7230
* https://github.com/tarantool/tarantool/issues/6782
Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6782-fix-vmevent-handler
PR: https://github.com/tarantool/tarantool/pull/7826
PR contains huge change, because tarantool/luajit branch hasn't been
bumped yet in Tarantool's repo.
Also, lto checks on lua-Harness/303-package.t are red for LTO build,
this is related to CI/Build changes that were merged a few days ago;
Tested on the previous commit to my commit.
src/lj_trace.c | 6 +++-
...6782-stitching-in-vmevent-handler.test.lua | 28 +++++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
create mode 100644 test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua
<snipped>2.34.1
-- Best regards,Sergey Kaplun