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