Re: [Tarantool-patches] [PATCH luajit 1/2] Prevent recording of loops with -0 step or NaN values.

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

Hi, Sergey!
Thanks for the review!
Fixed your comments and updated the branch.

On 13.03.26, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! LGTM with minor comments.
> 
> Sergey
> 
> On 3/12/26 18:55, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun.
> >
> > (cherry picked from commit 54cce2e1719a15fc33e40c57dbc3d62e9c104b03)
> >
> > The -0 step and NaN control variable values may lead to the traces
> > with always failed guards.
> >
> > This patch forbids recording of such traces since these traces are not
> > very useful. Unfortunately, this breaks for loop recording in DUALNUM
> > mode. This will be fixed in the next commit.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#12134
> > ---
> >   src/lj_record.c                               |  6 ++
> >   .../lj-1432-minus-zero-step.test.lua          | 57 +++++++++++++
> >   .../lj-1433-nan-for-control-var.test.lua      | 79 +++++++++++++++++++
> >   3 files changed, 142 insertions(+)
> >   create mode 100644 test/tarantool-tests/lj-1432-minus-zero-step.test.lua
> >   create mode 100644 test/tarantool-tests/lj-1433-nan-for-control-var.test.lua
> >
> > diff --git a/src/lj_record.c b/src/lj_record.c
> > index 81da43f5..a3a68b57 100644
> > --- a/src/lj_record.c
> > +++ b/src/lj_record.c

<snipped>

> > diff --git a/test/tarantool-tests/lj-1432-minus-zero-step.test.lua b/test/tarantool-tests/lj-1432-minus-zero-step.test.lua
> > new file mode 100644
> > index 00000000..112153dc
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1432-minus-zero-step.test.lua
> > @@ -0,0 +1,57 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to check the correct recording of -0 step for value.
> > +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1432.
> > +
> > +local test = tap.test('lj-1432-minus-zero-step'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(2)
> > +
> > +local traceinfo = require('jit.util').traceinfo
> > +
> > +local function trace_slot()
> > +  local counter = 0
> > +  local slot = -0
> > +  -- Run the inner trace several times. Before the patch, it leads
> > +  -- to several child traces due to the always failed guards.
> > +  while true do
> > +    if counter > 5 then break end
> > +    counter = counter + 1;
> > +    -- luacheck: ignore
> > +    for _ = 1, 1, slot do
> > +      break
> > +    end
> > +  end
> > +end
> > +
> > +local function trace_const()
> > +  local counter = 0
> > +  -- Run the inner trace several times. Before the patch, it leads
> > +  -- to several child traces due to the always failed guards.
> > +  while true do
> > +    if counter > 5 then break end
> > +    counter = counter + 1;
> > +    -- luacheck: ignore
> > +    for _ = 1, 1, -0 do
> > +      break
> > +    end
> > +  end
> > +end
> > +
> > +local function test_trace_recorded(test_payload)
> > +  jit.flush()
> > +  -- Reset hotcounters.
> nit: comment can be omitted

I prefer not to. There may be the question: why we don't declare this
parameters once? The reason is that the hotcounters may cause collisions
and lead to the false-positive tests failures. Should I make the comment
more verbose?

> > +  jit.opt.start('hotloop=1', 'hotexit=1')
> > +  test_payload()
> > +  return traceinfo(1)
> > +end
> > +
> > +-- The -0 step leads to the always failed guard, so such traces
> > +-- are now aborted and not recorded.
> > +
> > +test:ok(not test_trace_recorded(trace_slot), 'no trace recorded -0 as slot')
> > +test:ok(not test_trace_recorded(trace_const), 'no trace recorded -0 as const')
> > +
> > +test:done(true)
> > diff --git a/test/tarantool-tests/lj-1433-nan-for-control-var.test.lua b/test/tarantool-tests/lj-1433-nan-for-control-var.test.lua
> > new file mode 100644
> > index 00000000..1f67f0ad
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1433-nan-for-control-var.test.lua
> > @@ -0,0 +1,79 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to check the correct recording of for control
> > +-- variable with NaN value.
> > +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1433.
> > +
> > +local test = tap.test('lj-1433-nan-for-control-var'):skipcond({
> 
> I would rename: 
> s/lj-1433-nan-for-control-var/lj-1433-nan-for-loop-control-var/
> 
> Feel free to ignore.

Renamed here and the test name.

===================================================================
diff --git a/test/tarantool-tests/lj-1433-nan-for-control-var.test.lua b/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
similarity index 96%
rename from test/tarantool-tests/lj-1433-nan-for-control-var.test.lua
rename to test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
index 1f67f0ad..fccd13c4 100644
--- a/test/tarantool-tests/lj-1433-nan-for-control-var.test.lua
+++ b/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
@@ -4,7 +4,7 @@ local tap = require('tap')
 -- variable with NaN value.
 -- See also https://github.com/LuaJIT/LuaJIT/issues/1433.
 
-local test = tap.test('lj-1433-nan-for-control-var'):skipcond({
+local test = tap.test('lj-1433-nan-for-loop-control-var'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
===================================================================

> 
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(3)
> > +
> > +local traceinfo = require('jit.util').traceinfo
> > +
> > +local function trace_nan_start()
> 
> nit: s/trace_nan_start/trace_nan_loop_start/
> 
> the same below

Renamed:

===================================================================
diff --git a/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua b/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
index 63bbddcc..b9e5ad92 100644
--- a/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
+++ b/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
@@ -12,7 +12,7 @@ test:plan(3)
 
 local traceinfo = require('jit.util').traceinfo
 
-local function trace_nan_start()
+local function trace_nan_loop_start()
   local counter = 0
   -- XXX: Use NaN as stack slot, not upvalue.
   local nan = 0 / 0
@@ -28,7 +28,7 @@ local function trace_nan_start()
   end
 end
 
-local function trace_nan_stop()
+local function trace_nan_loop_stop()
   local counter = 0
   -- XXX: Use NaN as stack slot, not upvalue.
   local nan = 0 / 0
@@ -44,7 +44,7 @@ local function trace_nan_stop()
   end
 end
 
-local function trace_nan_step()
+local function trace_nan_loop_step()
   local counter = 0
   -- XXX: Use NaN as stack slot, not upvalue.
   local nan = 0 / 0
@@ -71,9 +71,12 @@ end
 -- The NaN loop control vars leads to the always failed guard, so
 -- such traces are now aborted and not recorded.
 
-test:ok(not test_trace_recorded(trace_nan_start), 'no trace recorded NaN start')
-test:ok(not test_trace_recorded(trace_nan_stop), 'no trace recorded NaN stop')
-test:ok(not test_trace_recorded(trace_nan_step), 'no trace recorded NaN step')
+test:ok(not test_trace_recorded(trace_nan_loop_start),
+        'no trace recorded NaN start')
+test:ok(not test_trace_recorded(trace_nan_loop_stop),
+        'no trace recorded NaN stop')
+test:ok(not test_trace_recorded(trace_nan_loop_step),
+        'no trace recorded NaN step')
 
 test:done(true)
===================================================================


> > +  local counter = 0
> > +  -- XXX: Use NaN as stack slot, not upvalue.
> > +  local nan = 0 / 0
> > +  -- Run the inner trace several times. Before the patch, it leads
> > +  -- to the trace with always fail guard.
> > +  while true do
> > +    if counter > 5 then break end
> > +    counter = counter + 1;
> > +    -- luacheck: ignore
> > +    for _ = nan, 1, 1 do
> > +      break
> > +    end
> > +  end
> > +end
> > +
> > +local function trace_nan_stop()
> > +  local counter = 0
> > +  -- XXX: Use NaN as stack slot, not upvalue.
> > +  local nan = 0 / 0
> > +  -- Run the inner trace several times. Before the patch, it leads
> > +  -- to the trace with always fail guard.
> > +  while true do
> > +    if counter > 5 then break end
> > +    counter = counter + 1;
> > +    -- luacheck: ignore
> > +    for _ = 1, nan, 1 do
> > +      break
> > +    end
> > +  end
> > +end
> > +
> > +local function trace_nan_step()
> > +  local counter = 0
> > +  -- XXX: Use NaN as stack slot, not upvalue.
> > +  local nan = 0 / 0
> > +  -- Run the inner trace several times. Before the patch, it leads
> > +  -- to several child traces due to the always failed guards.
> > +  while true do
> > +    if counter > 5 then break end
> > +    counter = counter + 1;
> > +    -- luacheck: ignore
> > +    for _ = 1, 1, nan do
> > +      break
> > +    end
> > +  end
> > +end
> > +
> > +local function test_trace_recorded(test_payload)
> > +  jit.flush()
> > +  -- Reset hotcounters.
> > +  jit.opt.start('hotloop=1', 'hotexit=1')
> > +  test_payload()
> > +  return traceinfo(1)
> > +end
> > +
> > +-- The NaN control vars leads to the always failed guard, so such
> s/control/loop control/

Fixed.
===================================================================
diff --git a/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua b/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
index fccd13c4..63bbddcc 100644
--- a/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
+++ b/test/tarantool-tests/lj-1433-nan-for-loop-control-var.test.lua
@@ -68,8 +68,8 @@ local function test_trace_recorded(test_payload)
   return traceinfo(1)
 end
 
--- The NaN control vars leads to the always failed guard, so such
--- traces are now aborted and not recorded.
+-- The NaN loop control vars leads to the always failed guard, so
+-- such traces are now aborted and not recorded.
 
 test:ok(not test_trace_recorded(trace_nan_start), 'no trace recorded NaN start')
 test:ok(not test_trace_recorded(trace_nan_stop), 'no trace recorded NaN stop')
===================================================================

> > +-- traces are now aborted and not recorded.
> > +
> > +test:ok(not test_trace_recorded(trace_nan_start), 'no trace recorded NaN start')
> > +test:ok(not test_trace_recorded(trace_nan_stop), 'no trace recorded NaN stop')
> > +test:ok(not test_trace_recorded(trace_nan_step), 'no trace recorded NaN step')
> > +
> > +test:done(true)
> > +

-- 
Best regards,
Sergey Kaplun