From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 9CB84F0AB14; Tue, 21 Jan 2025 15:28:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9CB84F0AB14 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1737462513; bh=hPmuQoht+a+8Xglp16ii2CfLhW/SSYi7wBQLNv9uoyQ=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=tmAEJZ9omT6qAkUjlx/Iw8nR+SwbtdAJvnvjhnU51xMTJiFo8EbaMsNIplRuXn+44 bhI2gf0nef1rpQv1Xpoa8JSXcUpSsyTs4NPMFGLXRslGQn5BhcnczMQq0Q23WWxr6H mlaE9mGZWxc11hngI5SMxBPzyRnJoa/8P0PLHHIg= Received: from send219.i.mail.ru (send219.i.mail.ru [95.163.59.58]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0D6FA57AFDD for ; Tue, 21 Jan 2025 15:28:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0D6FA57AFDD Received: by exim-smtp-6758d5575c-rcbnz with esmtpa (envelope-from ) id 1taDMs-000000006qr-3ph6; Tue, 21 Jan 2025 15:28:31 +0300 Date: Tue, 21 Jan 2025 15:27:53 +0300 To: Sergey Bronnikov Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F92BE5667526BA91E395D7587768BB3AAAAB6B6460CC5D35182A05F5380850405ABA639AD5E0E5403DE06ABAFEAF6705A5D3B8C231C04FC6190C0D4EE70634F0495FBEDC84B91D45 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CC84CC3AD347B910EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D6424E594306CB78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D82E0B44B7FD83E94449F32AE26CADE7EB861A63F1ECD9FA22CC7F00164DA146DAFE8445B8C89999728AA50765F790063741F7343E26298569389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8C2B5EEE3591E0D35F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B9F5955FECEF5819E75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5EB3F45485F34DE505002B1117B3ED696F3844F8284F849AF7E0012C66AE17B00823CB91A9FED034534781492E4B8EEAD3CCD70CEBBF18A22BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D348D68DCC07DD06FF5DB5F6B52C9E12DB6CE0AD81185FF5127A2A29703747F1832BEB48B2D12D46FEB1D7E09C32AA3244C00552BF68E36F38B77DD89D51EBB774229C4F7F3541DADF2EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj/aSCloFp7d/RVgL1rCuMwQ== X-DA7885C5: B44CBEC05F5DF973F255D290C0D534F94F3901CAE2EA37F0197CDB20496D430C391186B7A85587105B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F739381B31377CF4CA219D7B644D5EBBC95561DEAC039A3F6C54394D7676ED7C38AF6E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit][v2] Prevent loop in snap_usedef(). X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 > 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