Hi, Sergey!

thanks for the comments! Changes force-pushed.

Sergey

Hi, Sergey!
Thanks for the patch and the fixes!
LGTM, after fixing my several suggestions about comments rephrasing and
a bunch of nits below.

On 16.01.25, Sergey Bronnikov wrote:

Reported by XmiliaH.

(cherry picked from commit 0e66fc96377853d898390f1a02723c54ec3a42f7)

It is possible to get an infinite loop in a function `snap_usedef`
when a `UCLO` makes a tight loop. This infinite loop could happen
when `snap_usedef()` is called on trace exit processes UCLO

It is called during trace recording (more precisely on creation of the
snapshot for the guarded trace check).

bytecode instruction and this instruction attempts a jump with
negative value. The patch fixes the problem by checking a number
of slots in a jump argument and replace this value my `maxslot` if
a value is negative.

Please also mention that "This means that no values will be purged from
the snapshot".

Added.

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

Part of tarantool/tarantool#10709
---

Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-736-prevent-loop-in-snap_usedef

Related issues:
* https://github.com/tarantool/tarantool/issues/10709
* https://github.com/LuaJIT/LuaJIT/issues/736

v2 changes:

- Updated test, now it hangs without patch with fix.
- Added more comments to the test with explanations.

 src/lj_snap.c                                 |  7 +-
 .../lj-736-BC_UCLO-triggers-infinite-loop.lua | 82 +++++++++++++++++++
 2 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua

diff --git a/src/lj_snap.c b/src/lj_snap.c
index 8a33dc22..8d7bd868 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c

<snipped>

diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
new file mode 100644
index 00000000..fb053e9a
--- /dev/null
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua

Filename suffix should be ".test.lua".
The ctest doesn't recognize this file.

@@ -0,0 +1,82 @@
+local tap = require('tap')
+local test = tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(2)
+
+-- Test reproduces an issue when BC_UCLO triggers an infinite loop.
+-- See details in https://github.com/LuaJIT/LuaJIT/issues/736.

Nit: Lets be consistent with most of our test files and put the short
description before the `tap.test()`:

Fixed:

--- a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua
@@ -3,11 +3,11 @@ local test = tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
-test:plan(2)
-
 -- Test reproduces an issue when BC_UCLO triggers an infinite loop.
 -- See details in https://github.com/LuaJIT/LuaJIT/issues/736.
---
+
+test:plan(2)
+
 -- Listing below demonstrates a problem -
 -- the bytecode UCLO on the line 13 makes a loop at 0013-0014:
 --

===================================================================
diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
index fb053e9a..f38098a8 100644
--- a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
@@ -1,13 +1,13 @@
 local tap = require('tap')
+-- Test file to demonstrate the infinite loop in LuaJIT during the
+-- use-def analysis for upvalues.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/736.
 local test = tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
 test:plan(2)
 
--- Test reproduces an issue when BC_UCLO triggers an infinite loop.
--- See details in https://github.com/LuaJIT/LuaJIT/issues/736.
---
 -- Listing below demonstrates a problem -
 -- the bytecode UCLO on the line 13 makes a loop at 0013-0014:
 --
===================================================================

+--
+-- Listing below demonstrates a problem -
+-- the bytecode UCLO on the line 13 makes a loop at 0013-0014:
+--
+-- - BYTECODE -- bc_uclo.lua:0-20
+-- 0001    KPRI     0   0
+-- 0002    FNEW     1   0      ; bc_uclo.lua:5
+-- 0003    KSHORT   2   1
+-- 0004    KSHORT   3   4
+-- 0005    KSHORT   4   1
+-- 0006    FORI     2 => 0011
+-- 0007 => ISNEN    5   0      ; 2
+-- 0008    JMP      6 => 0010
+-- 0009    UCLO     0 => 0012
+-- 0010 => FORL     2 => 0007
+-- 0011 => UCLO     0 => 0012
+-- 0012 => KPRI     0   0
+-- 0013    UCLO     0 => 0012
+-- 0014    FNEW     1   1      ; bc_uclo.lua:18
+-- 0015    UCLO     0 => 0016
+-- 0016 => RET0     0   1

I suggest to list only the necessary part of the bytecodes, also
lets take the bytecode dump for this particular function:

===================================================================
diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
index f38098a8..8d0558e9 100644
--- a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
@@ -8,27 +8,18 @@ local test = tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({
 
 test:plan(2)
 
--- Listing below demonstrates a problem -
--- the bytecode UCLO on the line 13 makes a loop at 0013-0014:
---
--- - BYTECODE -- bc_uclo.lua:0-20
--- 0001    KPRI     0   0
--- 0002    FNEW     1   0      ; bc_uclo.lua:5
--- 0003    KSHORT   2   1
--- 0004    KSHORT   3   4
--- 0005    KSHORT   4   1
--- 0006    FORI     2 => 0011
--- 0007 => ISNEN    5   0      ; 2
--- 0008    JMP      6 => 0010
--- 0009    UCLO     0 => 0012
--- 0010 => FORL     2 => 0007
--- 0011 => UCLO     0 => 0012
--- 0012 => KPRI     0   0
--- 0013    UCLO     0 => 0012
--- 0014    FNEW     1   1      ; bc_uclo.lua:18
--- 0015    UCLO     0 => 0016
--- 0016 => RET0     0   1
-
+-- Before the patch, the code flow like in the `testcase()` below
+-- may cause the problem -- use-def analysis for the 0019 UCLO
+-- creates an infinite loop in 0014 - 0019:
+-- | 0008 FORI   base:    4 jump:  => 0013
+-- | 0009 ISNEN  var:     7 num:     0 ; number 2
+-- | 0010 JMP    rbase:   8 jump:  => 0012
+-- | 0011 UCLO   rbase:   2 jump:  => 0014
+-- | 0012 FORL   base:    4 jump:  => 0009
+-- | 0013 UCLO   rbase:   2 jump:  => 0014
+-- | 0014 KPRI   dst:     2 pri:     0 ; Start of `assert()` line.
+-- | ...
+-- | 0019 UCLO   rbase:   2 jump:  => 0014
 jit.opt.start('hotloop=1')
 
 local assert_msg = 'Infinite loop is not reproduced.'

Fixed:

diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua
index fb053e9a..30fea28b 100644
--- a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua
@@ -3,31 +3,23 @@ local test = tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({
 -- Test reproduces an issue when BC_UCLO triggers an infinite loop.
 -- See details in https://github.com/LuaJIT/LuaJIT/issues/736.
---
--- Listing below demonstrates a problem -
--- the bytecode UCLO on the line 13 makes a loop at 0013-0014:
---
--- - BYTECODE -- bc_uclo.lua:0-20
--- 0001    KPRI     0   0
--- 0002    FNEW     1   0      ; bc_uclo.lua:5
--- 0003    KSHORT   2   1
--- 0004    KSHORT   3   4
--- 0005    KSHORT   4   1
--- 0006    FORI     2 => 0011
--- 0007 => ISNEN    5   0      ; 2
--- 0008    JMP      6 => 0010
--- 0009    UCLO     0 => 0012
--- 0010 => FORL     2 => 0007
--- 0011 => UCLO     0 => 0012
--- 0012 => KPRI     0   0
--- 0013    UCLO     0 => 0012
--- 0014    FNEW     1   1      ; bc_uclo.lua:18
--- 0015    UCLO     0 => 0016
--- 0016 => RET0     0   1
+
+test:plan(2)
+
+-- Before the patch, the code flow like in the `testcase()` below
+-- may cause the problem -- use-def analysis for the 0019 UCLO
+-- creates an infinite loop in 0014 - 0019:
+-- | 0008 FORI   base:    4 jump:  => 0013
+-- | 0009 ISNEN  var:     7 num:     0 ; number 2
+-- | 0010 JMP    rbase:   8 jump:  => 0012
+-- | 0011 UCLO   rbase:   2 jump:  => 0014
+-- | 0012 FORL   base:    4 jump:  => 0009
+-- | 0013 UCLO   rbase:   2 jump:  => 0014
+-- | 0014 KPRI   dst:     2 pri:     0 ; Start of `assert()` line.
+-- | ...
+-- | 0019 UCLO   rbase:   2 jump:  => 0014
 
 jit.opt.start('hotloop=1')
 

===================================================================

+
+jit.opt.start('hotloop=1')
+
+local assert_msg = 'Infinite loop is not reproduced.'
+local assert = assert
+
+local function testcase()
+  -- The code in the first scope `do`/`end` is a prerequisite.
+  -- It is needed so that we have a trace at the exit from which
+  -- the creation of the snapshot will begin.

The needed part is not the creation of the snapshot but the use-def
analysis for the first (`uv1`) UCLO. I would rephrase it like the
following:

===================================================================
diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
index 8d0558e9..3e6508d1 100644
--- a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
@@ -27,11 +27,9 @@ local assert = assert
 
 local function testcase()
   -- The code in the first scope `do`/`end` is a prerequisite.
-  -- It is needed so that we have a trace at the exit from which
-  -- the creation of the snapshot will begin.
+  -- It contains the UCLO instruction for the `uv1`. The use-def
+  -- analysis for it escapes this `do`/`end` scope.
   do
-    -- Upvalue below is not used actually, but it is required
-    -- for calling `snap_usedef()` on trace exit.
     local uv1 -- luacheck: ignore
     local _ = function() return uv1 end

Updated:

 
@@ -36,11 +28,9 @@ local assert = assert
 
 local function testcase()
   -- The code in the first scope `do`/`end` is a prerequisite.
-  -- It is needed so that we have a trace at the exit from which
-  -- the creation of the snapshot will begin.
+  -- It contains the UCLO instruction for the `uv1`. The use-def
+  -- analysis for it escapes this `do`/`end` scope.
   do
-    -- Upvalue below is not used actually, but it is required
-    -- for calling `snap_usedef()` on trace exit.
     local uv1 -- luacheck: ignore
     local _ = function() return uv1 end

 
===================================================================

+  do
+    -- Upvalue below is not used actually, but it is required
+    -- for calling `snap_usedef()` on trace exit.

Side note: The use-def analysis is called on trace recording.
I would rather merge this comment with the comment above, as you can see in
the diff.

+    local uv1 -- luacheck: ignore

Nit: It is better to use "no unused" here to be more specific.

+    local _ = function() return uv1 end
+
+    -- The loop below is required for recording a trace.
+    -- The condition inside a loop executes `goto` to a label
+    -- outside of the loop when the code executed by JIT and
+    -- this triggers snapshotting.
+    for i = 1, 2 do
+      -- Exit to interpreter once trace is compiled.

I would rephrase this 2 comments, since they a little bit misleading:

===================================================================
diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
index 3e6508d1..a95651fd 100644
--- a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
@@ -33,12 +33,11 @@ local function testcase()
     local uv1 -- luacheck: ignore
     local _ = function() return uv1 end
 
-    -- The loop below is required for recording a trace.
-    -- The condition inside a loop executes `goto` to a label
-    -- outside of the loop when the code executed by JIT and
-    -- this triggers snapshotting.
+    -- Records the trace for which use-def analysis is applied.
     for i = 1, 2 do
-      -- Exit to interpreter once trace is compiled.
+      -- This condition triggers snapshoting and use-def analysis.
+      -- Before the patch this triggers the infinite loop in the
+      -- `snap_usedef()`, so the `goto` is never taken.
       if i == 2 then
         goto x
       end

===================================================================

+      if i == 2 then
+        goto x
+      end
+    end
+  end
+
+::x::
+  do
+    local uv2 -- luacheck: no unused
+
+    -- `goto` if not executed without a patch and generates an
+    -- UCLO bytecode that makes an infinite loop in a function
+    -- `snap_usedef` when patch is not applied. `goto` must point
+    -- to the label on one of the previous lines. `assert()` is
+    -- executed when patch is applied.
+    assert(nil, assert_msg)
+    goto x
+
+    -- Line below is required, it makes `uv` upvalue, and must be
+    -- placed after `goto`, otherwise reproducer become broken.
+    local _ = function() return uv2 end -- luacheck: ignore

I would also rephrase this 2 comments like the following since they
are repeated what we say before:

===================================================================
diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
index a95651fd..8111d752 100644
--- a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
+++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
@@ -47,17 +47,11 @@ local function testcase()
 ::x::
   do
     local uv2 -- luacheck: no unused
-
-    -- `goto` if not executed without a patch and generates an
-    -- UCLO bytecode that makes an infinite loop in a function
-    -- `snap_usedef` when patch is not applied. `goto` must point
-    -- to the label on one of the previous lines. `assert()` is
-    -- executed when patch is applied.
     assert(nil, assert_msg)
+    -- Create a tight loop for the one more upvalue (`uv2`).
+    -- Before the patch, use-def analysis gets stuck in this code
+    -- flow.
     goto x
-
-    -- Line below is required, it makes `uv` upvalue, and must be
-    -- placed after `goto`, otherwise reproducer become broken.
     local _ = function() return uv2 end -- luacheck: ignore
   end
 end

===================================================================

Also I would add the following comment:
| -- This code is unreachable by design. Prevent luacheck warning.

+  end
+end
+
+local ok, err = pcall(testcase)
+
+test:is(ok, false, 'assertion is triggered in a function with testcase')
+test:ok(err:match(assert_msg), 'BC_UCLO does not trigger an infinite loop')

I would rather say here 'correct error message'. Or, as an alternative,

"correct error message" is too general for the specific test.

I left the message the same.

we may just use 1 test check:

| pcall(testcase)
|
| test:ok(true, 'no infinite loop in the use-def analysis')

instead of these 2 checks.

+
+os.exit(test:check() and 0 or 1)

Minor: Please use `test:done(true)` like in all other tests.

Updated:

@@ -79,4 +64,4 @@ local ok, err = pcall(testcase)
 test:is(ok, false, 'assertion is triggered in a function with testcase')
 test:ok(err:match(assert_msg), 'BC_UCLO does not trigger an infinite loop')
 
-os.exit(test:check() and 0 or 1)
+test:done(true)

-- 
2.34.1