Среда, 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