Hi, Sergey! Thanks for the review! I’ve fixed everything you requested and updated the corresponding branch. -- Best regards, Maxim Kokryashkin     >Четверг, 16 июня 2022, 9:58 +03:00 от Sergey Kaplun : >  >Hi, Maxim! > >Thanks for the patch! >LGTM, except a few nits below. > >On 12.06.22, Maxim Kokryashkin wrote: >> There is no need to dump proto or trace information for sysprof >> in the default mode. Moreover, attempts to do so will lead to >> segmentation fault due to uninitialized buffer. This commit >> disables proto and trace dumps in the default mode. >> >> Resolves tarantool/tarantool#7264 >> --- >> PR: https://github.com/tarantool/tarantool/pull/7265 >> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-7264-dump-proto-default-mode >> >> src/lj_sysprof.c | 4 +- >> ...4-add-proto-trace-sysprof-default.test.lua | 58 +++++++++++++++++++ >> 2 files changed, 60 insertions(+), 2 deletions(-) >> create mode 100644 test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua >> >> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c >> index a4a70146..2e9ed9b3 100644 >> --- a/src/lj_sysprof.c >> +++ b/src/lj_sysprof.c > > > >> diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua >> new file mode 100644 >> index 00000000..2d2a756a >> --- /dev/null >> +++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua >> @@ -0,0 +1,58 @@ >> +-- Sysprof is implemented for x86 and x64 architectures only. >> +local ffi = require("ffi") >> +require("utils").skipcond( >> + jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux" >> + or ffi.abi("gc64"), >> + jit.arch.." architecture or "..jit.os.. >> + " OS is NIY for sysprof" >> +) >> + >> +local tap = require("tap") >> +local jit = require("jit") >> + >> +local test = tap.test("gh-7264-add-proto-trace-sysprof-default.test.lua") >> +test:plan(2) >> + >> +jit.off() >> +jit.flush() >> + >> +local function allocate() >> + local a = {} >> + for _ = 1, 1e4 do table.insert(a, "teststring") end > >Why do we need so many iterations? >We can manipulate JIT behaviour by using `jit.opt.start('hotloop=1')`. > >> + return a >> +end >> + >> +local chunk = [[ >> +function lua_global_f() >> + local a = 'teststring' > >Nit: I suggest to use one type of quotes in the whole test. >(Also, it is better to use single quotes everywhere (its offline >agreement with Igor). > >> +end >> +]] >> + >> +-- Trace creation during the sysprof runtime. >> +jit.on() >> + >> +local _ = nil > >Why do we need this variable? (If for the `res, _ =` below, we can just >use the single `res = `). > >> +local res, err = misc.sysprof.start({ mode = "D" }) >> +assert(res, err) >> +allocate() >> +res, _ = misc.sysprof.stop() >> +test:ok(res) > >Nit: please add name for the test. > >> + >> +jit.off() >> + >> +-- Proto creation during the sysprof runtime. >> +res, err = misc.sysprof.start({ mode = "D" }) >> +assert(res, err) >> + >> +res, err = loadstring(chunk) >> +assert(res, err) >> +res() >> + >> +-- luacheck: globals lua_global_f >> +lua_global_f() >> +collectgarbage() >> +res, _ = misc.sysprof.stop() >> +test:ok(res) > >Ditto. > >> + >> +jit.on() >> +os.exit(test:check() and 0 or 1) >> -- >> 2.36.1 >> > >-- >Best regards, >Sergey Kaplun