Re: [Tarantool-patches] [PATCH luajit 2/2] Fix base register coalescing in side trace.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

<snipped>

> > 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')
> > +

<snipped>

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

> > +

<snipped>

> > +  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)
===================================================================

<snipped>

> > 

-- 
Best regards,
Sergey Kaplun