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 02/19] test: introduce mcode generator for tests
Date: Wed, 16 Aug 2023 16:06:44 +0300 [thread overview]
Message-ID: <1692191204.476119903@f136.i.mail.ru> (raw)
In-Reply-To: <ZNzHOflhIcA-2jL3@root>
[-- Attachment #1: Type: text/plain, Size: 10152 bytes --]
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
[-- Attachment #2: Type: text/html, Size: 12464 bytes --]
next prev parent reply other threads:[~2023-08-16 13:06 UTC|newest]
Thread overview: 97+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-09 15:35 [Tarantool-patches] [PATCH luajit 00/19] Prerequisites for improve assertions Sergey Kaplun via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 01/19] MIPS: Use precise search for exit jump patching Sergey Kaplun via Tarantool-patches
2023-08-15 9:36 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 12:40 ` Sergey Kaplun via Tarantool-patches
2023-08-16 13:25 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 02/19] test: introduce mcode generator for tests Sergey Kaplun via Tarantool-patches
2023-08-15 10:14 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 12:55 ` Sergey Kaplun via Tarantool-patches
2023-08-16 13:06 ` Maxim Kokryashkin via Tarantool-patches [this message]
2023-08-16 14:32 ` Sergey Bronnikov via Tarantool-patches
2023-08-16 15:20 ` Sergey Kaplun via Tarantool-patches
2023-08-16 16:08 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 03/19] MIPS: Fix handling of spare long-range jump slots Sergey Kaplun via Tarantool-patches
2023-08-15 11:13 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:05 ` Sergey Kaplun via Tarantool-patches
2023-08-16 15:02 ` Sergey Bronnikov via Tarantool-patches
2023-08-16 15:32 ` Sergey Kaplun via Tarantool-patches
2023-08-16 16:08 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 04/19] MIPS64: Add soft-float support to JIT compiler backend Sergey Kaplun via Tarantool-patches
2023-08-15 11:27 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:10 ` Sergey Kaplun via Tarantool-patches
2023-08-16 16:07 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 05/19] PPC: Add soft-float support to interpreter Sergey Kaplun via Tarantool-patches
2023-08-15 11:40 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:13 ` Sergey Kaplun via Tarantool-patches
2023-08-17 14:53 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 06/19] PPC: Add soft-float support to JIT compiler backend Sergey Kaplun via Tarantool-patches
2023-08-15 11:46 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:21 ` Sergey Kaplun via Tarantool-patches
2023-08-17 14:33 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 07/19] build: fix non-Linux/macOS builds Sergey Kaplun via Tarantool-patches
2023-08-15 11:58 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:40 ` Sergey Kaplun via Tarantool-patches
2023-08-17 14:31 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 08/19] Windows: Add UWP support, part 1 Sergey Kaplun via Tarantool-patches
2023-08-15 12:09 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:50 ` Sergey Kaplun via Tarantool-patches
2023-08-16 16:40 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 09/19] FFI: Eliminate hardcoded string hashes Sergey Kaplun via Tarantool-patches
2023-08-15 13:07 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:52 ` Sergey Kaplun via Tarantool-patches
2023-08-16 17:04 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:35 ` [Tarantool-patches] [PATCH luajit 10/19] Cleanup math function compilation and fix inconsistencies Sergey Kaplun via Tarantool-patches
2023-08-11 8:06 ` Sergey Kaplun via Tarantool-patches
2023-08-15 13:10 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 17:15 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 11/19] Fix GCC 7 -Wimplicit-fallthrough warnings Sergey Kaplun via Tarantool-patches
2023-08-15 13:17 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 13:59 ` Sergey Kaplun via Tarantool-patches
2023-08-17 7:37 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 12/19] DynASM: Fix warning Sergey Kaplun via Tarantool-patches
2023-08-15 13:21 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 14:01 ` Sergey Kaplun via Tarantool-patches
2023-08-17 7:39 ` Sergey Bronnikov via Tarantool-patches
2023-08-17 7:51 ` Sergey Bronnikov via Tarantool-patches
2023-08-17 7:58 ` Sergey Kaplun via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 13/19] ARM: Fix GCC 7 -Wimplicit-fallthrough warnings Sergey Kaplun via Tarantool-patches
2023-08-15 13:25 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 14:08 ` Sergey Kaplun via Tarantool-patches
2023-08-17 7:44 ` Sergey Bronnikov via Tarantool-patches
2023-08-17 8:01 ` Sergey Kaplun via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 14/19] Fix debug.getinfo() argument check Sergey Kaplun via Tarantool-patches
2023-08-15 13:35 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 14:20 ` Sergey Kaplun via Tarantool-patches
2023-08-16 20:13 ` Maxim Kokryashkin via Tarantool-patches
2023-08-17 8:29 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 15/19] Fix LJ_MAX_JSLOTS assertion in rec_check_slots() Sergey Kaplun via Tarantool-patches
2023-08-15 14:07 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 14:22 ` Sergey Kaplun via Tarantool-patches
2023-08-17 8:57 ` Sergey Bronnikov via Tarantool-patches
2023-08-17 8:57 ` Sergey Kaplun via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 16/19] Prevent integer overflow while parsing long strings Sergey Kaplun via Tarantool-patches
2023-08-15 14:38 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 14:52 ` Sergey Kaplun via Tarantool-patches
2023-08-17 10:53 ` Sergey Bronnikov via Tarantool-patches
2023-08-17 13:57 ` Sergey Kaplun via Tarantool-patches
2023-08-17 14:28 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 17/19] MIPS64: Fix register allocation in assembly of HREF Sergey Kaplun via Tarantool-patches
2023-08-16 9:01 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 15:17 ` Sergey Kaplun via Tarantool-patches
2023-08-16 20:14 ` Maxim Kokryashkin via Tarantool-patches
2023-08-17 11:06 ` Sergey Bronnikov via Tarantool-patches
2023-08-17 13:50 ` Sergey Kaplun via Tarantool-patches
2023-08-17 14:30 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 18/19] DynASM/MIPS: Fix shadowed variable Sergey Kaplun via Tarantool-patches
2023-08-16 9:03 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 15:22 ` Sergey Kaplun via Tarantool-patches
2023-08-17 12:01 ` Sergey Bronnikov via Tarantool-patches
2023-08-09 15:36 ` [Tarantool-patches] [PATCH luajit 19/19] MIPS: Add MIPS64 R6 port Sergey Kaplun via Tarantool-patches
2023-08-16 9:16 ` Maxim Kokryashkin via Tarantool-patches
2023-08-16 15:24 ` Sergey Kaplun via Tarantool-patches
2023-08-17 13:03 ` Sergey Bronnikov via Tarantool-patches
2023-08-17 13:59 ` Sergey Kaplun via Tarantool-patches
2023-08-16 15:35 ` [Tarantool-patches] [PATCH luajit 00/19] Prerequisites for improve assertions Sergey Kaplun via Tarantool-patches
2023-08-17 14:06 ` Maxim Kokryashkin via Tarantool-patches
2023-08-17 14:38 ` Sergey Bronnikov via Tarantool-patches
2023-08-31 15:17 ` 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=1692191204.476119903@f136.i.mail.ru \
--to=tarantool-patches@dev.tarantool.org \
--cc=m.kokryashkin@tarantool.org \
--cc=skaplun@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit 02/19] test: introduce mcode generator for tests' \
/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