<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the fixes!</div><div>LGTM</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;">Среда, 16 августа 2023, 16:00 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16921908091975992011_BODY">Hi, Maxim!<br>Thanks for the review!<br>Please, see my replies below.<br><br>On 15.08.23, Maxim Kokryashkin wrote:<br>> Hi, Sergey!<br>> Thanks for the patch!<br>> Please consider my comments below.<br>><br>> On Wed, Aug 09, 2023 at 06:35:51PM +0300, Sergey Kaplun via Tarantool-patches wrote:<br>> > The test <test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64><br>> > depends on particular offset of mcode for side trace regarding the<br>> > parent trace. Before this commit just run some amount of functions to<br>> > generate traces to fill the required mcode range. Unfortunately, this<br>> > approach is not robust, since sometimes trace is not recorded due to<br>> > errors "leaving loop in root trace" observed because of hotcount<br>> > collisions.<br>> ><br>> > This patch introduces the following helpers:<br>> > * `frontend.gettraceno(func)` -- returns the traceno for the given<br>> > function, assumming that there is compiled trace for its prototype<br>> > (i.e. the 0th bytecode is JFUNC).<br>> > * `jit.generators.fillmcode(traceno, size)` fills mcode area of the<br>> > given size from the given trace. It is useful to generate some mcode<br>> > to test jumps to side traces remote enough from the parent.<br>> > ---<br>> > ...8-fix-side-exit-patching-on-arm64.test.lua | 78 ++----------<br>> > test/tarantool-tests/utils/frontend.lua | 24 ++++<br>> > test/tarantool-tests/utils/jit/generators.lua | 115 ++++++++++++++++++<br>> > 3 files changed, 150 insertions(+), 67 deletions(-)<br>> > create mode 100644 test/tarantool-tests/utils/jit/generators.lua<br>> ><br>> > diff --git a/test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua b/test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua<br>> > index 93db3041..678ac914 100644<br>> > --- a/test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua<br>> > +++ b/test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/utils/frontend.lua b/test/tarantool-tests/utils/frontend.lua<br>> > index 2afebbb2..414257fd 100644<br>> > --- a/test/tarantool-tests/utils/frontend.lua<br>> > +++ b/test/tarantool-tests/utils/frontend.lua<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/utils/jit/generators.lua b/test/tarantool-tests/utils/jit/generators.lua<br>> > new file mode 100644<br>> > index 00000000..62b6e0ef<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/utils/jit/generators.lua<br>> > @@ -0,0 +1,115 @@<br>> > +local M = {}<br>> > +<br>> > +local jutil = require('jit.util')<br>> > +<br>> > +local function getlast_traceno()<br>> > + return misc.getmetrics().jit_trace_num<br>> > +end<br>> > +<br>> > +-- Convert addr to positive value if needed.<br>> > +local function canonize_address(addr)<br>> Nit: most of the time, the `canonize` variant is used in theological materials,<br>> while the `canonicalize` is more common in the sphere of software development.<br>> Feel free to ignore.<br><br>Fixed, thanks.<br><br>> > + if addr < 0 then addr = addr + 2 ^ 32 end<br>> > + return addr<br>> > +end<br>> > +<br>> > +-- Need some storage to avoid functions and traces to be<br>> > +-- collected.<br>> Typo: s/Need/We need/ or s/Need some storage/Some storage is needed/<br>> Typo: s/to be collected/being collected/<br><br>Fixed.<br><br>> > +local recfuncs = {}<br>> > +local last_i = 0<br>> > +-- This function generates a table of functions with heavy mcode<br>> > +-- payload with tab arithmetics to fill the mcode area from the<br>> > +-- one trace mcode by the some given size. This size is usually<br>> Typo: s/by the some/by some/<br><br>Fixed, thanks!<br><br>> > +-- big enough, because we want to check long jump side exits from<br>> > +-- some traces.<br>> > +-- Assumes, that maxmcode and maxtrace options are set to be sure,<br>> Typo: s/that/that the/<br><br>Fixed.<br><br>> > +-- that we can produce such amount of mcode.<br>> > +function M.fillmcode(trace_from, size)<br>> > + local mcode, addr_from = jutil.tracemc(trace_from)<br>> > + assert(mcode, 'the #1 argument should be an existed trace number')<br>> Typo: s/existed/existing/<br><br>Fixed, thanks!<br><br>> > + addr_from = canonize_address(addr_from)<br>> > + local required_diff = size + #mcode<br>> > +<br>> > + -- Marker to check that traces are not flushed.<br>> > + local maxtraceno = getlast_traceno()<br>> > + local FLUSH_ERR = 'Traces are flushed, check your maxtrace, maxmcode options'<br>> > +<br>> > + local _, last_addr = jutil.tracemc(maxtraceno)<br>> > + last_addr = canonize_address(last_addr)<br>> > +<br>> > + -- Addresses of traces may increase or decrease depending on OS,<br>> > + -- so use absolute diff.<br>> > + while math.abs(last_addr - addr_from) > required_diff do<br>> > + last_i = last_i + 1<br>> > + -- This is a quite heavy workload (though it doesn't look like<br>> Typo: s/This is a quite/This is quite a/<br><br>Fixed.<br><br>> > + -- one at first). Each load from a table is type guarded. Each<br>> > + -- table lookup (for both stores and loads) is guarded for<br>> > + -- table <hmask> value and presence of the metatable. The code<br>> Typo: s/and presence/and the presence/<br><br>Fixed.<br><br>> > + -- below results to ~8Kb of mcode for ARM64 and MIPS64 in<br>> Typo: s/results to/results in/<br><br>Fixed.<br><br>> > + -- practice.<br>> > + local fname = ('fillmcode[%d]'):format(last_i)<br>> > + recfuncs[last_i] = assert(loadstring(([[<br>> > + return function(src)<br>> > + local p = %d<br>> Nit: Poor naming, a more descriptive name is preferred.<br><br>It has no much sense, because we really don't care about of the<br>function's content. Since it's just moved part of the code, I prefer to<br>leave it as is.<br><br>Ignoring for now.<br><br>> > + local tmp = { }<br>> > + local dst = { }<br>> > + -- XXX: use 5 as stop index to reduce LLEAVE (leaving loop<br>> Typo: s/as stop/as a stop/<br><br>Fixed, thanks!<br><br>> > + -- in root trace) errors due to hotcount collisions.<br>> > + for i = 1, 5 do<br><br><snipped><br><br>> > + local function tnew(p)<br>> Nit: same issue with naming.<br><br>Ditto.<br><br>> > + return {<br><br><snipped><br><br>See the iterative patch below:<br><br>===================================================================<br>diff --git a/test/tarantool-tests/utils/jit/generators.lua b/test/tarantool-tests/utils/jit/generators.lua<br>index 62b6e0ef..65abfdaa 100644<br>--- a/test/tarantool-tests/utils/jit/generators.lua<br>+++ b/test/tarantool-tests/utils/jit/generators.lua<br>@@ -7,26 +7,26 @@ local function getlast_traceno()<br> end<br> <br> -- Convert addr to positive value if needed.<br>-local function canonize_address(addr)<br>+local function canonicalize_address(addr)<br> if addr < 0 then addr = addr + 2 ^ 32 end<br> return addr<br> end<br> <br>--- Need some storage to avoid functions and traces to be<br>+-- Some storage is needed to avoid functions and traces being<br> -- collected.<br> local recfuncs = {}<br> local last_i = 0<br> -- This function generates a table of functions with heavy mcode<br> -- payload with tab arithmetics to fill the mcode area from the<br>--- one trace mcode by the some given size. This size is usually<br>--- big enough, because we want to check long jump side exits from<br>--- some traces.<br>--- Assumes, that maxmcode and maxtrace options are set to be sure,<br>--- that we can produce such amount of mcode.<br>+-- one trace mcode by some given size. This size is usually big<br>+-- enough, because we want to check long jump side exits from some<br>+-- traces.<br>+-- Assumes, that the maxmcode and maxtrace options are set to be<br>+-- sure, that we can produce such amount of mcode.<br> function M.fillmcode(trace_from, size)<br> local mcode, addr_from = jutil.tracemc(trace_from)<br>- assert(mcode, 'the #1 argument should be an existed trace number')<br>- addr_from = canonize_address(addr_from)<br>+ assert(mcode, 'the #1 argument should be an existing trace number')<br>+ addr_from = canonicalize_address(addr_from)<br> local required_diff = size + #mcode<br> <br> -- Marker to check that traces are not flushed.<br>@@ -34,17 +34,17 @@ function M.fillmcode(trace_from, size)<br> local FLUSH_ERR = 'Traces are flushed, check your maxtrace, maxmcode options'<br> <br> local _, last_addr = jutil.tracemc(maxtraceno)<br>- last_addr = canonize_address(last_addr)<br>+ last_addr = canonicalize_address(last_addr)<br> <br> -- Addresses of traces may increase or decrease depending on OS,<br> -- so use absolute diff.<br> while math.abs(last_addr - addr_from) > required_diff do<br> last_i = last_i + 1<br>- -- This is a quite heavy workload (though it doesn't look like<br>+ -- This is quite a heavy workload (though it doesn't look like<br> -- one at first). Each load from a table is type guarded. Each<br> -- table lookup (for both stores and loads) is guarded for<br>- -- table <hmask> value and presence of the metatable. The code<br>- -- below results to ~8Kb of mcode for ARM64 and MIPS64 in<br>+ -- table <hmask> value and the presence of the metatable. The<br>+ -- code below results in ~8Kb of mcode for ARM64 and MIPS64 in<br> -- practice.<br> local fname = ('fillmcode[%d]'):format(last_i)<br> recfuncs[last_i] = assert(loadstring(([[<br>@@ -52,8 +52,8 @@ function M.fillmcode(trace_from, size)<br> local p = %d<br> local tmp = { }<br> local dst = { }<br>- -- XXX: use 5 as stop index to reduce LLEAVE (leaving loop<br>- -- in root trace) errors due to hotcount collisions.<br>+ -- XXX: use 5 as a stop index to reduce LLEAVE (leaving<br>+ -- loop in root trace) errors due to hotcount collisions.<br> for i = 1, 5 do<br> tmp.a = src.a * p tmp.j = src.j * p tmp.s = src.s * p<br> tmp.b = src.b * p tmp.k = src.k * p tmp.t = src.t * p<br>@@ -108,7 +108,7 @@ function M.fillmcode(trace_from, size)<br> if not last_addr then<br> error(FLUSH_ERR)<br> end<br>- last_addr = canonize_address(last_addr)<br>+ last_addr = canonicalize_address(last_addr)<br> end<br> end<br> <br>===================================================================<br><br>> > +end<br>> > +<br>> > +return M<br>> > --<br>> > 2.41.0<br>> Best regards,<br>> Maxim Kokryashkin<br>> ><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>