

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