<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM</div><div> </div><div data-signature-widget="container"><div 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;">Вторник, 23 января 2024, 16:18 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_17060159271178260117_BODY">Hi, Sergey!<br>Thanks for the review!<br><br>On 23.01.24, Sergey Bronnikov wrote:<br>> Hi, Sergey!<br>><br>><br>> thanks for the patch! LGTM with a minor comment below<br>><br>><br>> Sergey<br>><br>><br>> On 1/22/24 12:32, Sergey Kaplun wrote:<br>> > From: Mike Pall <mike><br>> ><br>> > Thanks to Sergey Kaplun.<br>> ><br>> > (cherry-picked from commit 7dbe545933485849977d50384f2f20f2cccf0cf9)<br>> ><br>> > Before this commit, the JIT engine didn't check the status of JIT flags<br>> > when compiling the side trace. Hence, after calling `jit.off()` the side<br>> > traces are still recorded. This patch adds the aforementioned check.<br>> ><br>> > Sergey Kaplun:<br>> > * added the description and the test for the problem<br>> ><br>> > Part of tarantool/tarantool#9595<br>> > ---<br>> ><br>> > Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-1134-hotside-jit-off" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-1134-hotside-jit-off</a><br>> > Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/9607" target="_blank">https://github.com/tarantool/tarantool/pull/9607</a><br>> > Related issues:<br>> > * <a href="https://github.com/tarantool/tarantool/issues/9595" target="_blank">https://github.com/tarantool/tarantool/issues/9595</a><br>> > * <a href="https://github.com/LuaJIT/LuaJIT/issues/1134" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/1134</a><br>> ><br>> > src/lj_trace.c | 2 +-<br>> > .../lj-1134-hotside-jit-off.test.lua | 44 +++++++++++++++++++<br>> > 2 files changed, 45 insertions(+), 1 deletion(-)<br>> > create mode 100644 test/tarantool-tests/lj-1134-hotside-jit-off.test.lua<br>> ><br>> > diff --git a/src/lj_trace.c b/src/lj_trace.c<br>> > index 236e06a0..20014ecb 100644<br>> > --- a/src/lj_trace.c<br>> > +++ b/src/lj_trace.c<br>> > @@ -917,7 +917,7 @@ int LJ_FASTCALL lj_trace_exit(jit_State *J, void *exptr)<br>> > } else if (G(L)->gc.state == GCSatomic || G(L)->gc.state == GCSfinalize) {<br>> > if (!(G(L)->hookmask & HOOK_GC))<br>> > lj_gc_step(L); /* Exited because of GC: drive GC forward. */<br>> > - } else {<br>> > + } else if ((J->flags & JIT_F_ON)) {<br>> > trace_hotside(J, pc);<br>> > }<br>> > if (bc_op(*pc) == BC_JLOOP) {<br>> > diff --git a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua<br>> > new file mode 100644<br>> > index 00000000..cdee3eb2<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua<br>> > @@ -0,0 +1,44 @@<br>> > +local tap = require('tap')<br>> > +<br>> > +-- Test file to demonstrate the JIT misbehaviour, when the side<br>> > +-- trace is compiled after `jit.off()`.<br>> > +-- See also: <a href="https://github.com/LuaJIT/LuaJIT/issue/1134" target="_blank">https://github.com/LuaJIT/LuaJIT/issue/1134</a>.<br>> > +<br>> > +local test = tap.test('lj-1134-hotside-jit-off'):skipcond({<br>> > + ['Test requires JIT enabled'] = not jit.status(),<br>> > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',<br>> > +})<br>> > +<br>> > +local traceinfo = require('jit.util').traceinfo<br>> > +<br>> > +test:plan(1)<br>> > +<br>> > +local take_side<br>> > +local function trace()<br>> > + -- luacheck: ignore<br>> > + -- Branch for the side exit.<br>> > + if take_side then end<br>> > +end<br>> > +<br>> > +-- Flush all possible traces.<br>> > +jit.flush()<br>> > +<br>> > +jit.opt.start('hotloop=1', 'hotexit=1')<br>> > +<br>> > +trace()<br>> > +trace()<br>> > +<br>> > +assert(traceinfo(1), 'root trace not compiled')<br>><br>> It is not clear what "1" means (here and below). As I got it right, it<br>> is a trace number.<br>><br>> I would left a comment about magic number or replace constant with<br>> variable with self-explained name.<br><br>Added the following comment. Branch is force-pushed.<br><br>===================================================================<br>diff --git a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua<br>index cdee3eb2..080b1e87 100644<br>--- a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua<br>+++ b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua<br>@@ -9,6 +9,7 @@ local test = tap.test('lj-1134-hotside-jit-off'):skipcond({<br> ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',<br> })<br> <br>+-- `traceinfo()` takes the trace number as an argument.<br> local traceinfo = require('jit.util').traceinfo<br> <br> test:plan(1)<br>===================================================================<br><br>><br>> > +<br>> > +-- Force trace exit.<br>> > +take_side = true<br>> > +<br>> > +jit.off()<br>> > +<br>> > +-- Attempt to compile a side trace.<br>> > +trace()<br>> > +trace()<br>> > +<br>> > +test:ok(not traceinfo(2), 'no side trace compiled')<br>> > +<br>> > +test:done(true)<br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>