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 BB0F3BA1AE9; Thu, 13 Jun 2024 13:29:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BB0F3BA1AE9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1718274557; bh=z012Wdk3rtZMyrqSE7xztI31THxoHJfEZe2ArpcIfwI=; 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=pTS1p0H0g8L4Naop9RHGGCt0ejqKxGvb2jrEp78MnSFrdiSeNthR1feDJjdxIGld4 arn1ZwIdtlRdrWF0D3s6ytFUWW9wTtsEIcqyf2D8NP9ue2gWD6lfzkDCnp8c4r2hrr 0FzCwFPbgbzr8jc21RSf49SI3B0YePAe/sToYvi8= Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [95.163.41.90]) (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 419B7BA1AE8 for ; Thu, 13 Jun 2024 13:29:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 419B7BA1AE8 Received: by smtp53.i.mail.ru with esmtpa (envelope-from ) id 1sHhhk-00000006270-1fYD; Thu, 13 Jun 2024 13:29:16 +0300 Date: Thu, 13 Jun 2024 13:25:03 +0300 To: Sergey Bronnikov Message-ID: References: <362ec5c556a9e9832726ec89978ef25ba865af41.1713773432.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9AC8CA0B4439200FA48E649EE0BB96B6C79307777E79D26E800894C459B0CD1B94877A55C5C209674608635B212260D36ADBE07E1B42E3B1F1DABAF5FDD7C91DC1E35E39F85F323EC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BAE5222749FC9020C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE77EB2E345998A721DEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B043BF0FB74779F368964E7EAEDC4989F01A48DC06AD6F3A27DD8C8DBCD2601C4A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE767883B903EA3BAEA9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C34964A708C60C975A117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C2FFDA4F57982C5F42E808ACE2090B5E1725E5C173C3A84C34964A708C60C975A089D37D7C0E48F6C8AA50765F7900637427B078F297B269AEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5979F77D09ACEBB815002B1117B3ED6966C81829FA2E438503E67C18142C611B7823CB91A9FED034534781492E4B8EEAD003C2D46C52F18F2BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFC2766BA788CBB8ADEC31A2FC4582ADE35855B9C175F176E6F9FA0E6053F7DA04A7825560E8A5912C04A26A3878DC1330D86F002A09BDB02D9C3E4AB3CE3B5CD0F12177D1D4BCC90E5F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojP/c/PTD82AmAHBFoGkPgJw== X-DA7885C5: EA384F15EAA662EDF255D290C0D534F96FF7FD130F30FDF4D9EF1BF1D6B94E1A24CC768B182552B85B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393C6D0B12EA33CAA9B632E8886F72F5319640D2711EEA9CC5540BBD3C2368CAC31E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching. 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 review! Please consider my answers below. On 06.06.24, Sergey Bronnikov wrote: > Hi, Sergey, > > > thanks for the patch! Please see my comments. > > On 22.04.2024 11:49, Sergey Kaplun wrote: > > From: Mike Pall > > > > Thanks to Sergey Kaplun and Peter Cawley. > > > > (cherry picked from commit d06beb0480c5d1eb53b3343e78063950275aa281) > > > > This commit is a follow-up for the commit > > 1b8216023d5a79814389f1c1affef27c15d9de27 ("Throw any errors before stack > > changes in trace stitching."). The patch prepends failures for the > > specific error to be thrown. Nevertheless, the error may be thrown due > > to retrying trace recording in the case when table bump optimization > > is enabled or when OOM is observed during reallocation of the snapshot > > or IR buffers. > > > > This patch adds the corresponding protected frame and rethrows the error > > after a fixup of the stack. > > > > This patch also tests the correctness of copying the error message to > > the top of the stack to get a valid "abort" reason in the `jit.dump` > > utility. > > > > Also, this patch fixes a non-ASCII space character in the comment for > > . > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#9924 > > --- > > src/lj_ffrecord.c | 21 ++++++-- > > test/tarantool-tests/CMakeLists.txt | 1 + > > .../lj-1166-error-stitch-oom-ir-buff.test.lua | 46 ++++++++++++++++ > > ...j-1166-error-stitch-oom-snap-buff.test.lua | 54 +++++++++++++++++++ > > .../lj-1166-error-stitch-table-bump.test.lua | 38 +++++++++++++ > > .../lj-1166-error-stitch/CMakeLists.txt | 1 + > > .../lj-1166-error-stitch/mockalloc.c | 51 ++++++++++++++++++ > > .../lj-720-errors-before-stitch.test.lua | 40 +++++++++++++- > > 8 files changed, 245 insertions(+), 7 deletions(-) > > create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua > > create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua > > create mode 100644 test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua > > create mode 100644 test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt > > create mode 100644 test/tarantool-tests/lj-1166-error-stitch/mockalloc.c > > > > diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c > > index e3ed80fb..ff14e9e4 100644 > > --- a/src/lj_ffrecord.c > > +++ b/src/lj_ffrecord.c > > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > > index 56660932..d7c96078 100644 > > --- a/test/tarantool-tests/CMakeLists.txt > > +++ b/test/tarantool-tests/CMakeLists.txt > > @@ -39,6 +39,7 @@ add_subdirectory(lj-802-panic-at-mcode-protfail) > > add_subdirectory(lj-flush-on-trace) > > add_subdirectory(lj-1004-oom-error-frame) > > add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume) > > +add_subdirectory(lj-1166-error-stitch) > > > > # The part of the memory profiler toolchain is located in tools > > # directory, jit, profiler, and bytecode toolchains are located > > diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua > > new file mode 100644 > > index 00000000..e3a5397d > > --- /dev/null > > +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua > > @@ -0,0 +1,46 @@ > > +local tap = require('tap') > > + > > +-- Test file to demonstrate unbalanced Lua stack after instruction > > +-- recording due to throwing an error at recording of a stitched > > +-- function. > > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1166. > > + > > +local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({ > > should a name in tap.test match to test file name? > > now it is not. My mistake during copipasting. Fixed. > > > + ['Test requires JIT enabled'] = not jit.status(), > > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', > > +}) > > + > > +test:plan(1) > > + > > +local mockalloc = require('mockalloc') > > + > > +local function create_chunk(n_slots) > > I would add a comment like this: > > > --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua > +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua > @@ -14,6 +14,18 @@ test:plan(1) > >  local mockalloc = require('mockalloc') > > +-- Generate a Lua chunk like below: > +-- local s1 > +-- local s2 > +-- ... > +-- local sN > +-- for i = 1, 2 do > +--   s1 = i + 1 > +--   s2 = i + 2 > +--   ... > +--   sN = i + N > +--   math.modf(1) > +-- end >  local function create_chunk(n_slots) >    local chunk = '' >    for i = 1, n_slots do Added, see the iterative patch below. > > > > + local chunk = '' > > + > > +local mockalloc = require('mockalloc') > > + > > +local function create_chunk(n_conds) > the same as above: please add a comment with an example of generated Lua > chunk Added, see the iterative patch below. > > + local chunk = '' > > + chunk = chunk .. 'for i = 1, 2 do\n' > > + -- Each condition adds additional snapshot. > > + for i = 1, n_conds do > > + chunk = chunk .. (' if i < %d then end\n'):format(i + n_conds) > > + end > > + -- `math.modf()` recording is NYI. > > + chunk = chunk .. ' math.modf(1)\n' > > + chunk = chunk .. 'end\n' > > + return chunk > > +end > > + > > +-- XXX: Need to compile the cycle in the `create_chunk()` to > > +-- preallocate the snapshot buffer. > > +jit.opt.start('hotloop=1', '-loop', '-fold') > > + > > +-- XXX: Amount of slots is empirical. > > +local tracef = assert(loadstring(create_chunk(6))) > > + > > +-- XXX: Remove previous trace. > > +jit.off() > > +jit.flush() > > + > > +-- XXX: Update hotcounts to avoid hash collisions. > > +jit.opt.start('hotloop=1') > > + > > +jit.on() > > + > > +mockalloc.mock() > > + > > +tracef() > > + > > +mockalloc.unmock() > > + > > +test:ok(true, 'stack is balanced') > > + > > +test:done(true) > > diff --git a/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua > > new file mode 100644 > > index 00000000..f2453bbe > > --- /dev/null > > +++ b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua > > this test is not failed after reverting patch Do you build luajit with -DLUAJIT_ENABLE_TABLE_BUMP=ON? > > diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c > > new file mode 100644 > > index 00000000..d6d3492e > > --- /dev/null > > +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c > > @@ -0,0 +1,51 @@ > > + > > +/* Function to be used instead of the default allocator. */ > > +static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize) > > +{ > > + assert(old_allocf != NULL); > > + /* > > + * Check the specific reallocation related to the IR > > + * buffer or the snapshot buffer. > > + */ > > + if (osize * 2 == nsize) > > + return NULL; > > + return old_allocf(ud, ptr, osize, nsize); > > +} > > + > > +static int mock(lua_State *L) > > > It is actually not a test mock. > > According to definition [1] test mock imitate a behavior of a real object. > > Your memory allocator behaves as a real allocator, but in some cases it > will return > > a NULL instead of memory address. What if we rename "mock" to "allocator > with fault injection"? I renamed `mock_allocf` to `allocf_with_injection()` to avoid confusing. =================================================================== diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua index cf3ab0f5..eb229a5c 100644 --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua @@ -5,7 +5,7 @@ local tap = require('tap') -- function. -- See also: https://github.com/LuaJIT/LuaJIT/issues/1166. -local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({ +local test = tap.test('lj-1166-error-stitch-oom-ir-buff'):skipcond({ ['Test requires JIT enabled'] = not jit.status(), ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', }) @@ -22,6 +22,16 @@ jit.flush() test:plan(2) +-- Generate the following Lua chunk: +-- local s1 +-- ... +-- local sN +-- for i = 1, 2 do +--   s1 = i + 1 +--   ... +--   sN = i + N +--   math.modf(1) +-- end local function create_chunk(n_slots) local chunk = '' for i = 1, n_slots do diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua index 8bbdd96b..8ae26386 100644 --- a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua @@ -20,6 +20,13 @@ jit.flush() test:plan(2) +-- Generate the following Lua chunk: +-- for i = 1, 2 do +--   if i < 1 then end +--   ... +--   if i < N then end +--   math.modf(1) +-- end local function create_chunk(n_conds) local chunk = '' chunk = chunk .. 'for i = 1, 2 do\n' diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c index d6d3492e..19d32f8b 100644 --- a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c @@ -8,7 +8,8 @@ static lua_Alloc old_allocf = NULL; static void *old_alloc_state = NULL; /* Function to be used instead of the default allocator. */ -static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize) +static void *allocf_with_injection(void *ud, void *ptr, size_t osize, + size_t nsize) { assert(old_allocf != NULL); /* @@ -24,14 +25,14 @@ static int mock(lua_State *L) { assert(old_allocf == NULL); old_allocf = lua_getallocf(L, &old_alloc_state); - lua_setallocf(L, mock_allocf, old_alloc_state); + lua_setallocf(L, allocf_with_injection, old_alloc_state); return 0; } static int unmock(lua_State *L) { assert(old_allocf != NULL); - assert(old_allocf != mock_allocf); + assert(old_allocf != allocf_with_injection); lua_setallocf(L, old_allocf, old_alloc_state); old_allocf = NULL; old_alloc_state = NULL; =================================================================== > > > 1. https://www.martinfowler.com/articles/mocksArentStubs.html > > > +{ > > diff --git a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua > > index d750b721..6e8f70c2 100644 > > --- a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua > > +++ b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua -- Best regards, Sergey Kaplun