[Tarantool-patches] [PATCH luajit 0/2] Fix corner cases of for loop recording

[Tarantool-patches] [PATCH luajit 0/2] Fix corner cases of for loop recording

[Sergey Kaplun via Tarantool-patches]

The first patch prevents recording of useless traces with always-fail
guards for NaN control vars or -0 step values. The second patch is the
follow-up to fix DUALNUM mode.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1432-1433-bad-for-loops
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/1432
* https://github.com/LuaJIT/LuaJIT/issues/1433
* https://github.com/LuaJIT/LuaJIT/issues/1438
* https://github.com/tarantool/tarantool/issues/12134

Mike Pall (2):
  Prevent recording of loops with -0 step or NaN values.
  DUALNUM: Fix recording of loops broken by previous change.

 src/lj_record.c                               |  6 ++
 .../lj-1432-minus-zero-step.test.lua          | 57 +++++++++++++
 .../lj-1433-nan-for-control-var.test.lua      | 79 +++++++++++++++++++
 .../lj-1438-jit-for-canary.test.lua           | 22 ++++++
 4 files changed, 164 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
 create mode 100644 test/tarantool-tests/lj-1438-jit-for-canary.test.lua

-- 
2.53.0

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

[Sergey Kaplun via Tarantool-patches]

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 also https://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.
+  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 also https://github.com/LuaJIT/LuaJIT/issues/1433.
+
+local test = tap.test('lj-1433-nan-for-control-var'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(3)
+
+local traceinfo = require('jit.util').traceinfo
+
+local function trace_nan_start()
+  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
+-- 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)
+
-- 
2.53.0

[Tarantool-patches] [PATCH luajit 2/2] DUALNUM: Fix recording of loops broken by previous change.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Nicholas Davies.

(cherry picked from commit 1c3b5a4d722598ecbb9219480142eda682e87bb1)

The previous commit forbids recording for the NaN control variables
stack slots. These checks are false positives for integers in DUALNUM
mode. This prevents any for loop with integers as slots from being
recorded.

This patch fixes the checks.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#12134
---
 src/lj_record.c                               |  6 ++---
 .../lj-1438-jit-for-canary.test.lua           | 22 +++++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1438-jit-for-canary.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index a3a68b57..dbfc7f47 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -510,9 +510,9 @@ static LoopEvent rec_for(jit_State *J, const BCIns *fori, int isforl)
   TRef stop;
   IRType t;
   /* Avoid semantic mismatches and always failing guards. */
-  if (tvisnan(&tv[FORL_IDX]) ||
-      tvisnan(&tv[FORL_STOP]) ||
-      tvisnan(&tv[FORL_STEP]) ||
+  if ((tvisnum(&tv[FORL_IDX]) && tvisnan(&tv[FORL_IDX])) ||
+      (tvisnum(&tv[FORL_STOP]) && tvisnan(&tv[FORL_STOP])) ||
+      (tvisnum(&tv[FORL_STEP]) && tvisnan(&tv[FORL_STEP])) ||
       tvismzero(&tv[FORL_STEP]))
     lj_trace_err(J, LJ_TRERR_GFAIL);
   if (isforl) {  /* Handle FORL/JFORL opcodes. */
diff --git a/test/tarantool-tests/lj-1438-jit-for-canary.test.lua b/test/tarantool-tests/lj-1438-jit-for-canary.test.lua
new file mode 100644
index 00000000..4b67df4c
--- /dev/null
+++ b/test/tarantool-tests/lj-1438-jit-for-canary.test.lua
@@ -0,0 +1,22 @@
+local tap = require('tap')
+
+-- The test file to check the correct recording of the for loop.
+-- Used as a canary test, since we have none.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1438.
+
+local test = tap.test('lj-1438-jit-for-canary'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local traceinfo = require('jit.util').traceinfo
+
+jit.flush()
+jit.opt.start('hotloop=1')
+
+for _ = 1, 4 do end
+
+test:ok(traceinfo(1), 'simple for loop is recorded')
+
+test:done(true)
-- 
2.53.0