Hi, Sergey! Thanks for the fixes! LGTM   -- Best regards, Maxim Kokryashkin     >Понедельник, 23 октября 2023, 12:54 +03:00 от Sergey Kaplun : >  >Hi, Maxim! >Thanks for the review! >Fixed your comments and force-pushed the branch! > >On 13.10.23, Maxim Kokryashkin wrote: >> Hi, Sergey! >> Thanks for the patch! >> LGTM, except for a few comments below. >> >> On Wed, Oct 11, 2023 at 06:04:10PM +0300, Sergey Kaplun wrote: >> > From: Mike Pall >> > >> > Thanks to Sergey Kaplun, NiLuJe and Peter Cawley. >> > >> > (cherry-picked from commit aa2db7ebd1267836af5221336ccb4e9b4aa8372d) >> > >> > The previous patch fixed just part of the problem with the register >> > coalesing. For example, the parent base register may be used inside the >> > parent or child register sets when it shouldn't. This leads to incorrect >> > register allocations, which may lead to crashes or undefined behaviour. >> > This patch fixes it by excluding the parent base register from both >> > register sets. >> > >> > The test case for this patch doesn't fail before the commit since it >> > requires specific register allocation, which is hard to construct and >> > very fragile. Due to the lack of ideal sync with the upstream >> > repository, the test is passed before the patch. >> > It should become correct in future patches. >> Typo: s/in future patches/after backporting future patches/ > >Fixed, thanks! > >> > >> > Resolves tarantool/tarantool#8767 >> How can I reassure that, since we don't have any reproducer? > >You may try the recipe mentioned in the ticket and see that there are no >any crashes anymore. > > > >> > diff --git a/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua b/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua >> > new file mode 100644 >> > index 00000000..fc5efaaa >> > --- /dev/null >> > +++ b/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua >> > @@ -0,0 +1,139 @@ >> > +local tap = require('tap') >> > +-- Test file to demonstrate incorrect side trace head assembling >> > +-- when use parent trace register holding base (`RID_BASE`). >> > +-- See also, https://github.com/LuaJIT/LuaJIT/issues/1031 . >> > +-- >> > +-- XXX: For now, the test doesn't fail even on arm64 due to the >> > +-- different from upstream register allocation. Nevertheless, this >> > +-- issue should be gone with future backporting, so just leave the >> > +-- test case as is. >> > +local test = tap.test('lj-1031-asm-head-side-base-reg'):skipcond({ >> > + ['Test requires JIT enabled'] = not jit.status(), >> > +}) >> > + >> > +local ffi = require 'ffi' >> Please use parantheses here too. > >Fixed, thanks! > >> > +local int64_t = ffi.typeof('int64_t') >> > + > > > >> > + >> > +local ARR_SIZE = 1e2 >> Maybe it is better to just write it as 100? > >Fixed. > >> > +local lim_arr = {} >> > + >> > +-- XXX: Prevent irrelevant output in jit.dump(). >> > +jit.off() >> > + >> > +local INNER_TRACE_LIMIT = 20 >> > +local INNER_LIMIT1 = 1 >> > +local INNER_LIMIT2 = 4 >> Please drop a comment with explanation of why those numbers were chosen. > >Added, see the full patch below. > >> > +for i = 1, INNER_TRACE_LIMIT do lim_arr[i] = INNER_LIMIT1 end >> > +for i = INNER_TRACE_LIMIT + 1, ARR_SIZE + 1 do lim_arr[i] = INNER_LIMIT2 end >> These loops are hardly readable, it would be nice to make the multiline. > >Fixed. > >> > + > > > >> > + if k > 55 then else end >> Please drop a comment and explain why it is 55, for some people >> that is not obvious. > >Added. See iterational patch below. Brach is force-pushed. > >=================================================================== >diff --git a/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua b/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua >index fc5efaaa..35364f4e 100644 >--- a/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua >+++ b/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua >@@ -11,7 +11,7 @@ local test = tap.test('lj-1031-asm-head-side-base-reg'):skipcond({ >   ['Test requires JIT enabled'] = not jit.status(), > }) >  >-local ffi = require 'ffi' >+local ffi = require('ffi') > local int64_t = ffi.typeof('int64_t') >  > test:plan(1) >@@ -60,21 +60,31 @@ local r17 > local r18 > local r19 >  >-local ARR_SIZE = 1e2 >+local ARR_SIZE = 100 > local lim_arr = {} >  > -- XXX: Prevent irrelevant output in jit.dump(). > jit.off() >  >+-- `INNER_LIMIT1` - no cycle is taken. >+-- `INNER_LIMIT2` - cycle is taken and compiled. >+-- `INNER_TRACE_LIMIT` - empirical number of iterations to compile >+-- all necessary traces from the outer cycle. > local INNER_TRACE_LIMIT = 20 > local INNER_LIMIT1 = 1 > local INNER_LIMIT2 = 4 >-for i = 1, INNER_TRACE_LIMIT do lim_arr[i] = INNER_LIMIT1 end >-for i = INNER_TRACE_LIMIT + 1, ARR_SIZE + 1 do lim_arr[i] = INNER_LIMIT2 end >+for i = 1, INNER_TRACE_LIMIT do >+ lim_arr[i] = INNER_LIMIT1 >+end >+for i = INNER_TRACE_LIMIT + 1, ARR_SIZE + 1 do >+ lim_arr[i] = INNER_LIMIT2 >+end >  > -- Enable compilation back. > jit.on() >  >+-- XXX: `hotexit` is set to 2 to decrease the number of >+-- meaningless side traces. > jit.opt.start('hotloop=1', 'hotexit=2') >  > -- XXX: Trace numbers are given with the respect of using >@@ -105,6 +115,8 @@ while k < ARR_SIZE do >   local l19 = ffi_new(int64_t, k + 19) >   -- Side exit for TRACE 6 start 2/1. >   -- luacheck: ignore >+ -- XXX: The number is meaningless, just needs to be big enough >+ -- to be sure that all necessary traces are compiled. >   if k > 55 then else end >   r1 = tonumber(l1) >   r2 = tonumber(l2) >=================================================================== > > > >> > > >-- >Best regards, >Sergey Kaplun