Re: [Tarantool-patches] [PATCH luajit] Respect jit.off() on pending trace exit.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!

On 23.01.24, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> 
> thanks for the patch! LGTM with a minor comment below
> 
> 
> Sergey
> 
> 
> On 1/22/24 12:32, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun.
> >
> > (cherry-picked from commit 7dbe545933485849977d50384f2f20f2cccf0cf9)
> >
> > Before this commit, the JIT engine didn't check the status of JIT flags
> > when compiling the side trace. Hence, after calling `jit.off()` the side
> > traces are still recorded. This patch adds the aforementioned check.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9595
> > ---
> >
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1134-hotside-jit-off
> > Tarantool PR: https://github.com/tarantool/tarantool/pull/9607
> > Related issues:
> > * https://github.com/tarantool/tarantool/issues/9595
> > * https://github.com/LuaJIT/LuaJIT/issues/1134
> >
> >   src/lj_trace.c                                |  2 +-
> >   .../lj-1134-hotside-jit-off.test.lua          | 44 +++++++++++++++++++
> >   2 files changed, 45 insertions(+), 1 deletion(-)
> >   create mode 100644 test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
> >
> > diff --git a/src/lj_trace.c b/src/lj_trace.c
> > index 236e06a0..20014ecb 100644
> > --- a/src/lj_trace.c
> > +++ b/src/lj_trace.c
> > @@ -917,7 +917,7 @@ int LJ_FASTCALL lj_trace_exit(jit_State *J, void *exptr)
> >     } else if (G(L)->gc.state == GCSatomic || G(L)->gc.state == GCSfinalize) {
> >       if (!(G(L)->hookmask & HOOK_GC))
> >         lj_gc_step(L);  /* Exited because of GC: drive GC forward. */
> > -  } else {
> > +  } else if ((J->flags & JIT_F_ON)) {
> >       trace_hotside(J, pc);
> >     }
> >     if (bc_op(*pc) == BC_JLOOP) {
> > diff --git a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
> > new file mode 100644
> > index 00000000..cdee3eb2
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
> > @@ -0,0 +1,44 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate the JIT misbehaviour, when the side
> > +-- trace is compiled after `jit.off()`.
> > +-- See also: https://github.com/LuaJIT/LuaJIT/issue/1134.
> > +
> > +local test = tap.test('lj-1134-hotside-jit-off'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> > +})
> > +
> > +local traceinfo = require('jit.util').traceinfo
> > +
> > +test:plan(1)
> > +
> > +local take_side
> > +local function trace()
> > +  -- luacheck: ignore
> > +  -- Branch for the side exit.
> > +  if take_side then end
> > +end
> > +
> > +-- Flush all possible traces.
> > +jit.flush()
> > +
> > +jit.opt.start('hotloop=1', 'hotexit=1')
> > +
> > +trace()
> > +trace()
> > +
> > +assert(traceinfo(1), 'root trace not compiled')
> 
> It is not clear what "1" means (here and below). As I got it right, it 
> is a trace number.
> 
> I would left a comment about magic number or replace constant with 
> variable with self-explained name.

Added the following comment. Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
index cdee3eb2..080b1e87 100644
--- a/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
+++ b/test/tarantool-tests/lj-1134-hotside-jit-off.test.lua
@@ -9,6 +9,7 @@ local test = tap.test('lj-1134-hotside-jit-off'):skipcond({
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
 
+-- `traceinfo()` takes the trace number as an argument.
 local traceinfo = require('jit.util').traceinfo
 
 test:plan(1)
===================================================================

> 
> > +
> > +-- Force trace exit.
> > +take_side = true
> > +
> > +jit.off()
> > +
> > +-- Attempt to compile a side trace.
> > +trace()
> > +trace()
> > +
> > +test:ok(not traceinfo(2), 'no side trace compiled')
> > +
> > +test:done(true)

-- 
Best regards,
Sergey Kaplun