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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5802 bytes --]


Hi, Sergey!
Thanks for the fixes!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 23 октября 2023, 12:54 +03:00 от Sergey Kaplun <skaplun@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
 

[-- Attachment #2: Type: text/html, Size: 7260 bytes --]