From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Sergey Kaplun" <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] Fix base register coalescing in side trace.
Date: Tue, 24 Oct 2023 16:50:56 +0300 [thread overview]
Message-ID: <1698155456.392491086@f103.i.mail.ru> (raw)
In-Reply-To: <ZTZB25wiXnlbB4_3@root>
[-- 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 --]
next prev parent reply other threads:[~2023-10-24 13:50 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-11 15:04 [Tarantool-patches] [PATCH luajit 0/2] Fix assembling of the head of side traces Sergey Kaplun via Tarantool-patches
2023-10-11 15:04 ` [Tarantool-patches] [PATCH luajit 1/2] Fix register mask for stack check in head of side trace Sergey Kaplun via Tarantool-patches
2023-10-13 5:48 ` Maxim Kokryashkin via Tarantool-patches
2023-10-23 9:27 ` Sergey Kaplun via Tarantool-patches
2023-10-24 13:51 ` Maxim Kokryashkin via Tarantool-patches
2023-11-07 18:42 ` Igor Munkin via Tarantool-patches
2023-10-11 15:04 ` [Tarantool-patches] [PATCH luajit 2/2] Fix base register coalescing in " Sergey Kaplun via Tarantool-patches
2023-10-13 5:59 ` Maxim Kokryashkin via Tarantool-patches
2023-10-23 9:50 ` Sergey Kaplun via Tarantool-patches
2023-10-24 13:50 ` Maxim Kokryashkin via Tarantool-patches [this message]
2023-11-07 18:42 ` Igor Munkin via Tarantool-patches
2023-11-23 6:30 ` [Tarantool-patches] [PATCH luajit 0/2] Fix assembling of the head of side traces Igor Munkin via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1698155456.392491086@f103.i.mail.ru \
--to=tarantool-patches@dev.tarantool.org \
--cc=m.kokryashkin@tarantool.org \
--cc=skaplun@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit 2/2] Fix base register coalescing in side trace.' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox