Hi! Thanks for updates, LGTM. > On 26 Oct 2022, at 16:42, Sergey Kaplun 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.lua > index 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 wrote: >>> >>> From: Mike Pall >>> >>> 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() for > example) 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#7230 > > Branch 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 > > > >>> 2.34.1 >>> >> > > -- > Best regards, > Sergey Kaplun