<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey!</p>
<p>thanks for the comments! Changes force-pushed.<br>
</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 21.01.2025 15:27, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It is called during trace recording (more precisely on creation of the
snapshot for the guarded trace check).</pre>
</blockquote>
Updated.<br>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Please also mention that "This means that no values will be purged from
the snapshot".</pre>
</blockquote>
<p>Added.</p>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Sergey Bronnikov:
* added the description and the test for the problem
Part of tarantool/tarantool#10709
---
Branch: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/ligurio/lj-736-prevent-loop-in-snap_usedef">https://github.com/tarantool/luajit/tree/ligurio/lj-736-prevent-loop-in-snap_usedef</a>
Related issues:
* <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/10709">https://github.com/tarantool/tarantool/issues/10709</a>
* <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/736">https://github.com/LuaJIT/LuaJIT/issues/736</a>
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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Filename suffix should be ".test.lua".
The ctest doesn't recognize this file.</pre>
</blockquote>
Thanks! Fixed.<br>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@@ -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 <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/736">https://github.com/LuaJIT/LuaJIT/issues/736</a>.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Lets be consistent with most of our test files and put the short
description before the `tap.test()`:</pre>
</blockquote>
<p>Fixed:</p>
<p><br>
</p>
<p>---
a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua<br>
+++
b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua<br>
@@ -3,11 +3,11 @@ local test =
tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({<br>
['Test requires JIT enabled'] = not jit.status(),<br>
})<br>
<br>
-test:plan(2)<br>
-<br>
-- Test reproduces an issue when BC_UCLO triggers an infinite
loop.<br>
-- See details in <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/736">https://github.com/LuaJIT/LuaJIT/issues/736</a>.<br>
---<br>
+<br>
+test:plan(2)<br>
+<br>
-- Listing below demonstrates a problem -<br>
-- the bytecode UCLO on the line 13 makes a loop at 0013-0014:<br>
--<br>
</p>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
===================================================================
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 <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/736">https://github.com/LuaJIT/LuaJIT/issues/736</a>.
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 <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/736">https://github.com/LuaJIT/LuaJIT/issues/736</a>.
---
-- Listing below demonstrates a problem -
-- the bytecode UCLO on the line 13 makes a loop at 0013-0014:
--
===================================================================
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+--
+-- 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.'</pre>
</blockquote>
<p>Fixed:</p>
<p><br>
</p>
<p>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<br>
index fb053e9a..30fea28b 100644<br>
---
a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua<br>
+++
b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.test.lua<br>
@@ -3,31 +3,23 @@ local test =
tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({<br>
-- Test reproduces an issue when BC_UCLO triggers an infinite
loop.<br>
-- See details in <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/736">https://github.com/LuaJIT/LuaJIT/issues/736</a>.<br>
---<br>
--- Listing below demonstrates a problem -<br>
--- the bytecode UCLO on the line 13 makes a loop at 0013-0014:<br>
---<br>
--- - BYTECODE -- bc_uclo.lua:0-20<br>
--- 0001 KPRI 0 0<br>
--- 0002 FNEW 1 0 ; bc_uclo.lua:5<br>
--- 0003 KSHORT 2 1<br>
--- 0004 KSHORT 3 4<br>
--- 0005 KSHORT 4 1<br>
--- 0006 FORI 2 => 0011<br>
--- 0007 => ISNEN 5 0 ; 2<br>
--- 0008 JMP 6 => 0010<br>
--- 0009 UCLO 0 => 0012<br>
--- 0010 => FORL 2 => 0007<br>
--- 0011 => UCLO 0 => 0012<br>
--- 0012 => KPRI 0 0<br>
--- 0013 UCLO 0 => 0012<br>
--- 0014 FNEW 1 1 ; bc_uclo.lua:18<br>
--- 0015 UCLO 0 => 0016<br>
--- 0016 => RET0 0 1<br>
+<br>
+test:plan(2)<br>
+<br>
+-- Before the patch, the code flow like in the `testcase()` below<br>
+-- may cause the problem -- use-def analysis for the 0019 UCLO<br>
+-- creates an infinite loop in 0014 - 0019:<br>
+-- | 0008 FORI base: 4 jump: => 0013<br>
+-- | 0009 ISNEN var: 7 num: 0 ; number 2<br>
+-- | 0010 JMP rbase: 8 jump: => 0012<br>
+-- | 0011 UCLO rbase: 2 jump: => 0014<br>
+-- | 0012 FORL base: 4 jump: => 0009<br>
+-- | 0013 UCLO rbase: 2 jump: => 0014<br>
+-- | 0014 KPRI dst: 2 pri: 0 ; Start of `assert()`
line.<br>
+-- | ...<br>
+-- | 0019 UCLO rbase: 2 jump: => 0014<br>
<br>
jit.opt.start('hotloop=1')<br>
<br>
<br>
</p>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
===================================================================
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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</pre>
</blockquote>
<p>Updated:</p>
<p> <br>
@@ -36,11 +28,9 @@ local assert = assert<br>
<br>
local function testcase()<br>
-- The code in the first scope `do`/`end` is a prerequisite.<br>
- -- It is needed so that we have a trace at the exit from which<br>
- -- the creation of the snapshot will begin.<br>
+ -- It contains the UCLO instruction for the `uv1`. The use-def<br>
+ -- analysis for it escapes this `do`/`end` scope.<br>
do<br>
- -- Upvalue below is not used actually, but it is required<br>
- -- for calling `snap_usedef()` on trace exit.<br>
local uv1 -- luacheck: ignore<br>
local _ = function() return uv1 end<br>
</p>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
===================================================================
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ do
+ -- Upvalue below is not used actually, but it is required
+ -- for calling `snap_usedef()` on trace exit.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
Updated by patch above.<br>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ local uv1 -- luacheck: ignore
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: It is better to use "no unused" here to be more specific.
</pre>
</blockquote>
Updated.<br>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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</pre>
</blockquote>
Updated.<br>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
===================================================================
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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</pre>
</blockquote>
Updated.<br>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
===================================================================
Also I would add the following comment:
| -- This code is unreachable by design. Prevent luacheck warning.
</pre>
</blockquote>
Added "-- This code is unreachable by design."<br>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I would rather say here 'correct error message'. Or, as an alternative,</pre>
</blockquote>
<p>"correct error message" is too general for the specific test.</p>
<p>I left the message the same.<br>
</p>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+os.exit(test:check() and 0 or 1)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: Please use `test:done(true)` like in all other tests.
</pre>
</blockquote>
<p>Updated:</p>
<p>@@ -79,4 +64,4 @@ local ok, err = pcall(testcase)<br>
test:is(ok, false, 'assertion is triggered in a function with
testcase')<br>
test:ok(err:match(assert_msg), 'BC_UCLO does not trigger an
infinite loop')<br>
<br>
-os.exit(test:check() and 0 or 1)<br>
+test:done(true)<br>
</p>
<blockquote type="cite" cite="mid:Z4-SyXYqzKeh18IK@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>