* [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef().
@ 2025-01-16 16:36 Sergey Bronnikov via Tarantool-patches
2025-01-21 12:27 ` Sergey Kaplun via Tarantool-patches
2025-01-31 9:29 ` Sergey Kaplun via Tarantool-patches
0 siblings, 2 replies; 5+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2025-01-16 16:36 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin
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
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.
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
@@ -252,7 +252,12 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
BCReg minslot = bc_a(ins);
if (op >= BC_FORI && op <= BC_JFORL) minslot += FORL_EXT;
else if (op >= BC_ITERL && op <= BC_JITERL) minslot += bc_b(pc[-2])-1;
- else if (op == BC_UCLO) { pc += bc_j(ins); break; }
+ else if (op == BC_UCLO) {
+ ptrdiff_t delta = bc_j(ins);
+ if (delta < 0) return maxslot; /* Prevent loop. */
+ pc += delta;
+ break;
+ }
for (s = minslot; s < maxslot; s++) DEF_SLOT(s);
return minslot < maxslot ? minslot : maxslot;
}
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
@@ -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.
+--
+-- 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
+
+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.
+ 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
+
+ -- 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.
+ 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
+ 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')
+
+os.exit(test:check() and 0 or 1)
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef().
2025-01-16 16:36 [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef() Sergey Bronnikov via Tarantool-patches
@ 2025-01-21 12:27 ` Sergey Kaplun via Tarantool-patches
2025-01-21 14:43 ` Sergey Bronnikov via Tarantool-patches
2025-01-31 9:29 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2025-01-21 12:27 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
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".
>
> 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()`:
===================================================================
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.'
===================================================================
> +
> +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
===================================================================
> + 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,
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.
> --
> 2.34.1
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef().
2025-01-21 12:27 ` Sergey Kaplun via Tarantool-patches
@ 2025-01-21 14:43 ` Sergey Bronnikov via Tarantool-patches
2025-01-21 15:01 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2025-01-21 14:43 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 16895 bytes --]
Hi, Sergey!
thanks for the comments! Changes force-pushed.
Sergey
On 21.01.2025 15:27, Sergey Kaplun via Tarantool-patches wrote:
> 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).
Updated.
>
>> 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.
Thanks! Fixed.
>
>> @@ -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 inhttps://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 alsohttps://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 inhttps://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.
Updated by patch above.
>
>> + local uv1 -- luacheck: ignore
> Nit: It is better to use "no unused" here to be more specific.
Updated.
>> + 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
Updated.
> ===================================================================
>
>> + 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
Updated.
> ===================================================================
>
> Also I would add the following comment:
> | -- This code is unreachable by design. Prevent luacheck warning.
>
Added "-- This code is unreachable by design."
>> + 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
>>
[-- Attachment #2: Type: text/html, Size: 22304 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef().
2025-01-21 14:43 ` Sergey Bronnikov via Tarantool-patches
@ 2025-01-21 15:01 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2025-01-21 15:01 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Sergey,
Thanks for the fixes!
LGTM!
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef().
2025-01-16 16:36 [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef() Sergey Bronnikov via Tarantool-patches
2025-01-21 12:27 ` Sergey Kaplun via Tarantool-patches
@ 2025-01-31 9:29 ` Sergey Kaplun via Tarantool-patches
1 sibling, 0 replies; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2025-01-31 9:29 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Sergey,
I've applied the patch into all long-term branches in tarantool/luajit
and bumped a new version in master [1], release/3.3 [2], release/3.2 [3]
and release/2.11 [4].
[1]: https://github.com/tarantool/tarantool/pull/11063
[2]: https://github.com/tarantool/tarantool/pull/11064
[3]: https://github.com/tarantool/tarantool/pull/11065
[4]: https://github.com/tarantool/tarantool/pull/11066
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-31 9:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-16 16:36 [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef() Sergey Bronnikov via Tarantool-patches
2025-01-21 12:27 ` Sergey Kaplun via Tarantool-patches
2025-01-21 14:43 ` Sergey Bronnikov via Tarantool-patches
2025-01-21 15:01 ` Sergey Kaplun via Tarantool-patches
2025-01-31 9:29 ` Sergey Kaplun via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox