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