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

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

[-- Attachment #1: Type: text/plain, Size: 6463 bytes --]

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
> @@ -509,6 +509,12 @@ static LoopEvent rec_for(jit_State *J, const BCIns *fori, int isforl)
>     LoopEvent ev;
>     TRef stop;
>     IRType t;
> +  /* Avoid semantic mismatches and always failing guards. */
> +  if (tvisnan(&tv[FORL_IDX]) ||
> +      tvisnan(&tv[FORL_STOP]) ||
> +      tvisnan(&tv[FORL_STEP]) ||
> +      tvismzero(&tv[FORL_STEP]))
> +    lj_trace_err(J, LJ_TRERR_GFAIL);
>     if (isforl) {  /* Handle FORL/JFORL opcodes. */
>       TRef idx = tr[FORL_IDX];
>       if (mref(J->scev.pc, const BCIns) == fori && tref_ref(idx) == J->scev.idx) {
> 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
> +  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.

> +  ['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
> +  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/
> +-- 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)
> +

[-- Attachment #2: Type: text/html, Size: 7580 bytes --]