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 C9DCEF0BE1D; Tue, 21 Jan 2025 17:43:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C9DCEF0BE1D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1737470630; bh=2MytuHrphxYkb1kPLQeY9e1LjvzVUA1D4KiqxYahKrw=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=fteNFG3r3fvdndHRgbsvV2UZLuPIvGeoCqng2f4BUmVHru7+dhwE4ZoW2EYaL9NtD KH3qHu37tsiuSiTnKLEVnh3VS7mqPnqD7ks4AKFzUvlA4KqSQgNt2HvQpEEcf+bTjG tFSj/iEm505Jof3cVGzr9y4A5mljDl0eMtgDAz+8= Received: from send105.i.mail.ru (send105.i.mail.ru [89.221.237.200]) (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 91E5957AFDD for ; Tue, 21 Jan 2025 17:43:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 91E5957AFDD Received: by exim-smtp-6758d5575c-8729n with esmtpa (envelope-from ) id 1taFTk-00000000J4I-2dYk; Tue, 21 Jan 2025 17:43:49 +0300 Content-Type: multipart/alternative; boundary="------------1JI4eA14Rf2iZdcrMfghc8DU" Message-ID: Date: Tue, 21 Jan 2025 17:43:43 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F92BE5667526BA91771AC702472B4022644F91ED6DC72CFD182A05F5380850408E01A26613455EDD3DE06ABAFEAF6705567D797A3ACAA3D109DB795B4D7D73F9DAF06271CF5AE1BC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75644E22E05AA81AEB287FD4696A6DC2FA8DF7F3B2552694A4E2F5AFA99E116B42401471946AA11AF0E30A4C9C8E338DA82BFA309970D60C252120BFB3F63BC185F65E78799B30205C33C3ADAEA971F8E611E41BBFE2FEB2BEF933F3E754BE7A890D503CAE7B48B1C26A1F466678A0F4080D848B19C444D289FA2833FD35BB23D9E625A9149C048EE1E561CDFBCA1751FC26CFBAC0749D213D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAE9A1BBD95851C5BA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FCD19C4ED1728F8E7A3AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F790063710743A3D55712608D81D268191BDAD3D3666184CF4C3C14F3FC91FA280E0CE3D1A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F890A246B268E114EA91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5BD5F8AFB895ECC335002B1117B3ED696E76D6CFA1DD27CBE92B673A2F5DDD7E7823CB91A9FED034534781492E4B8EEAD577AE849BCD98940BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3488E5CD2E691F556622D32D1420672275A3AFFD13916206F465DFBB2FC981CEF8872011A07FFC0D9E1D7E09C32AA3244C28F8D08082FEAFEF77DD89D51EBB774217BFAA3E10928DA4EA455F16B58544A2E30DDF7C44BCB90DA5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj/aSCloFp7d80L6K/9bZaiA== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F258845877039BA2692A0A64CC811723C208B4179F5944F5F6676E2289C208F5AB1C7272645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------1JI4eA14Rf2iZdcrMfghc8DU Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > >> 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 >> --------------1JI4eA14Rf2iZdcrMfghc8DU Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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 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.
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



      

      

    

  
  


--------------1JI4eA14Rf2iZdcrMfghc8DU--