<HTML><BODY><div>Hi!</div><div>Thanks for the review!</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16894356720118705443_BODY">Hi, Maxim!<br>Thanks for the fixes!<br>Generally LGTM, but please consider my comments below.<br><br>On 10.07.23, Maxim Kokryashkin wrote:<br>> Sometimes, the Lua stack can be inconsistent during<br>> the FFUNC execution, which may lead to a sysprof<br>> crash during the stack unwinding.<br>><br>> This patch replaces the `top_frame` property of `global_State`<br>> with `lj_sysprof_topframe` structure, which contains `top_frame`<br>> and `ffid` properties. `ffid` property makes sense only when the<br>> LuaJIT VM state is set to `FFUNC`. That property is set to the<br>> ffid of the fast function that VM is about to execute.<br>> In the same time, `top_frame` property is not updated now, so<br>> the top frame of the Lua stack can be streamed based on the ffid,<br>> and the rest of the Lua stack can be streamed as usual.<br>><br>> Also, this patch fixes build with plain makefile, by adding<br><br>Nit: I suggest to rephrase it like "original Makefile" or<br>Makefile.original.<br><br>Feel free to ignore.</div></div></div></div></blockquote><div>Fixed! The branch is force-pushed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> the `LJ_HASSYSPROF` flag support to it.<br>><br>> Resolves tarantool/tarantool#8594<br>> ---<br>> Changes in v3:<br>> - Fixed comments as per review by Sergey<br>><br>> Branch: <a href="https://github.com/tarantool/luajit/tree/fckxorg/gh-8594-sysprof-ffunc-crash" target="_blank">https://github.com/tarantool/luajit/tree/fckxorg/gh-8594-sysprof-ffunc-crash</a><br>> PR: <a href="https://github.com/tarantool/tarantool/pull/8737" target="_blank">https://github.com/tarantool/tarantool/pull/8737</a><br>> src/Makefile.original | 3 ++<br>> src/lj_obj.h | 7 +++-<br>> src/lj_sysprof.c | 26 ++++++++++++---<br>> src/vm_x64.dasc | 22 +++++++++++--<br>> src/vm_x86.dasc | 31 ++++++++++++++---<br>> .../gh-8594-sysprof-ffunc-crash.test.lua | 33 +++++++++++++++++++<br>> 6 files changed, 109 insertions(+), 13 deletions(-)<br>> create mode 100644 test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua<br>><br>> diff --git a/src/Makefile.original b/src/Makefile.original<br>> index aedaaa73..e21a0e56 100644<br>> --- a/src/Makefile.original<br>> +++ b/src/Makefile.original<br><br><snipped><br><br>> diff --git a/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua b/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua<br>> new file mode 100644<br>> index 00000000..e5cdeb07<br>> --- /dev/null<br>> +++ b/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua<br>> @@ -0,0 +1,33 @@<br>> +local tap = require('tap')<br>> +local test = tap.test('gh-8594-sysprof-ffunc-crash'):skipcond({<br>> + ['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and<br>> + jit.arch ~= 'x64',<br>> + ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',<br>> +})<br>> +<br>> +test:plan(1)<br>> +<br>> +jit.off()<br>> +-- XXX: Run JIT tuning functions in a safe frame to avoid errors<br>> +-- thrown when LuaJIT is compiled with JIT engine disabled.<br>> +pcall(jit.flush)<br>> +<br>> +local TMP_BINFILE = '/dev/null'<br>> +<br>> +local res, err = misc.sysprof.start{<br>> + mode = 'C',<br>> + interval = 3,<br>> + path = TMP_BINFILE,<br>> +}<br>> +assert(res, err)<br>> +<br>> +for i = 1, 1e5 do<br>> + tostring(i)<br>> +end<br><br>Within these (interval/iterations) changes I hardly can see assertion failure<br>on master branch for my laptop (1/10 cases).<br><br>| >>> git log -n1 --no-decorate --oneline<br>| 8e46d601 test: fix flaky <unit-jit-parse.test.lua><br>| >>> LUA_PATH="src/?.lua;test/tarantool-tests/?.lua;;" src/luajit test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua<br>| TAP version 13<br>| 1..1<br>| ok - sysprof finished successfully<br><br>I don't know how to resolve this problem with our CI at death's door...<br>OTOH, we may consider that this value is enough for our CI, so I'll see<br>the problem there.<br>So, I'll agree with Sergey's opinion about this flaky test.</div></div></div></div></blockquote><div>I’ve added a conditional parametrization for this test case, here is the diff:</div><div>==========================================================</div><div><div><div>diff --git a/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua b/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua</div><div>index a053e41c..347bd087 100644</div><div>--- a/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua</div><div>+++ b/test/tarantool-tests/gh-8594-sysprof-ffunc-crash.test.lua</div><div>@@ -17,17 +17,30 @@ local TMP_BINFILE = '/dev/null'</div><div> -- XXX: The best way to test the issue is to set the profile</div><div> -- interval to be as short as possible. However, our CI is</div><div> -- not capable of handling such intense testing, so it was a</div><div>--- forced decision to reduce the sampling frequency. As a</div><div>+-- forced decision to reduce the sampling frequency for it. As a</div><div> -- result, it is now less likely to reproduce the issue</div><div> -- statistically, but the test case is still valid.</div><div>+</div><div>+-- GitHub always sets[1] the `CI` environment variable to `true`</div><div>+-- for every step in a workflow.</div><div>+-- [1]: https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables</div><div>+local CI = os.getenv('CI') == 'true'</div><div>+</div><div>+-- Profile interval and number of iterations for CI are</div><div>+-- empirical. Non-CI profile interval is set to be as short</div><div>+-- as possible, so the issue is more likely to reproduce.</div><div>+-- Non-CI number of iterations is greater for the same reason.</div><div>+local PROFILE_INTERVAL = CI and 3 or 1</div><div>+local N_ITERATIONS = CI and 1e5 or 1e6</div><div>+</div><div> local res, err = misc.sysprof.start{</div><div> mode = 'C',</div><div>- interval = 3,</div><div>+ interval = PROFILE_INTERVAL,</div><div> path = TMP_BINFILE,</div><div> }</div><div> assert(res, err)</div><div> </div><div>-for i = 1, 1e5 do</div><div>+for i = 1, N_ITERATIONS do</div><div> -- XXX: `tostring` is FFUNC.</div><div> tostring(i)</div><div> end</div><div>==========================================================</div><div>The branch is force-pushed.</div></div></div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +<br>> +res, err = misc.sysprof.stop()<br>> +assert(res, err)<br>> +<br>> +test:ok(true, 'sysprof finished successfully')<br>> +<br>> +os.exit(test:check() and 0 or 1)<br>> --<br>> 2.40.1<br>><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>