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