Re: [Tarantool-patches] [PATH luajit] GC64: fix 64-bit constant fusion

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergos!

Thanks for the review!

On 21.06.22, sergos wrote:
> Hi!
> 
> Thanks for working on it!
> Just minor nits, 
> LGTM

Fixed. See the iterative diff below.
Branch is force pushed.

> 
> Sergos
> 
> > On 16 Feb 2022, at 18:44, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > Hi, Sergos!
> > Thanks for the patch!
> > 
> > Hi , Igor!
> > Thanks for the review!
> > 
> > On 05.07.21, Igor Munkin wrote:
> >> Sergos,
> >> 
> >> Thanks for the patch! Something went wrong with formatting the patch I
> >> guess, so I'll share you the recipe how I send the backported patches:
> >> 1. cherry-pick the original patch from the upstream
> >> 2. format-patch the backported commit
> >> 3. Add "From" header with the original author of the changes at the
> >> very beginning of the formatted patch
> >> 4. Adjust the commit message according to our backporting procedure
> >> 
> >> BTW, I've found neither the branch with the patch in tarantool/luajit
> >> nor the branch with the green CI in tarantool/tarantool...
> > 
> > I've updated a little the old Sergos's branch [1] and save results on my
> > own branch [2].
> > Also I've added GC64 workflow for branch checkouted from the current
> > Tarantool's master branch [3].
> > 
> > The new commit message and patch is the following:
> > 
> > ===================================================================
> > commit 0759d1783ef96c4bb47869c6bc1181b38d95f654
> > Author: Mike Pall <mike>
> > Date: Mon Aug 28 10:43:37 2017 +0200
> > 
> > From: Mike Pall <mike>

Fixed the commit message :)

| x64/LJ_GC64: Fix fallback case of asm_fuseloadk64().
|
| Contributed by Peter Cawley.
|
| (chekky picked from commit 6b0824852677cc12570c20a3211fbfe0e4f0ce14)
|
| Code generation under LJ_GC64 missed an update to the mcode area
...


> > 
> > (chekky picked from commit 6b0824852677cc12570c20a3211fbfe0e4f0ce14)
> > 
> > x64/LJ_GC64: Fix fallback case of asm_fuseloadk64().
> > 
> > Contributed by Peter Cawley.
> > 
> > Code generation under LJ_GC64 missed an update to the mcode area
> > boundaries after a 64-bit constant encoding. It can lead to a
> > corruption of the constant value after next trace code generation.
> > The constant can be encoded in one of the following ways:
> > 
> > a. If the address of the constant fits into 32-bit one, then encode it
> > as a 32-bit displacement (the only option for non-GC64 mode).
> > b. If the offset of the constant slot from the dispatch table (pinned
> > to r14 that is not changed while trace execution) fits into 32-bit,
> > then encode this as a 32-bit displacement relative to r14.
> > c. If the offset of the constant slot from the mcode (i.e. rip) fits
> > into 32-bit, then encode this as a 32-bit displacement relative to
> > rip (considering long mode specifics and RID_RIP hack).
> > d. If none of the conditions above are valid, compiler materializes
> > this 64-bit constant right at the trace bottom and encodes this the
> > same way it does for the previous case.
> > 
> > The mentioned problem appears only with 2Gb distance from the currently
> > allocated mcode to the dispatch pointer, which may happen in real life
> > in case of long running instance.
> > 
> > Sergey Ostanevich:
> > * added the description and the test for the problem
> > 
> > Fixes tarantool/tarantool#4095
> > Fixes tarantool/tarantool#4199
> > Fixes tarantool/tarantool#4614
> > 
> > diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> > index 767bf6f3..2850aea9 100644
> > --- a/src/lj_asm_x86.h
> > +++ b/src/lj_asm_x86.h
> > @@ -387,6 +387,7 @@ static Reg asm_fuseloadk64(ASMState *as, IRIns *ir)
> > ir->i = (int32_t)(as->mctop - as->mcbot);
> > as->mcbot += 8;
> > as->mclim = as->mcbot + MCLIM_REDZONE;
> > + lj_mcode_commitbot(as->J, as->mcbot);
> > }
> > as->mrm.ofs = (int32_t)mcpofs(as, as->mctop - ir->i);
> > as->mrm.base = RID_RIP;
> > diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
> > new file mode 100644
> > index 00000000..9eabbdba
> > --- /dev/null
> > +++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
> > @@ -0,0 +1,79 @@
> > +-- The test is GC64 only.
> > +local ffi = require('ffi')
> > +require('utils').skipcond(not ffi.abi('gc64'), 'test is GC64 only')
> > +
> > +local tap = require('tap')
> > +local test = tap.test('gh-4199-gc64-fuse')
> > +test:plan(1)
> > +
> > +collectgarbage()
> > +-- Chomp memory in currently allocated GC space.
> > +collectgarbage('stop')
> > +
> > +for _ = 1, 8 do
> > + ffi.new('char[?]', 256 * 1024 * 1024)
> > +end
> > +
> > +jit.opt.start('hotloop=1')
> > +
> > +-- Generate a bunch of traces to trigger constant placement at the
> > +-- end of the trace. Since global describing the mcode area in the
>                              ^     ^
>                              a     variable
> > +-- jit structure is not updated, the next trace generated will
> > +-- invalidate the constant of the previous trace. Then
> > +-- execution of the _previous_ trace will use wrong value.
>                                                 ^
>                                                the
> > +
> > +-- Keep last two functions generated to compare results.
>           ^
>          the

I've updated comments as the following:

===================================================================
diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
index 9eabbdba..b34b4a4f 100644
--- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
+++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
@@ -17,12 +17,13 @@ end
 jit.opt.start('hotloop=1')
 
 -- Generate a bunch of traces to trigger constant placement at the
--- end of the trace. Since global describing the mcode area in the
--- jit structure is not updated, the next trace generated will
--- invalidate the constant of the previous trace. Then
--- execution of the _previous_ trace will use wrong value.
+-- end of the trace. Since a global variable describing the mcode
+-- area in the jit structure is not updated, the next trace
+-- generated will invalidate the constant of the previous trace.
+-- Then execution of the _previous_ trace will use the wrong
+-- value.
 
--- Keep last two functions generated to compare results.
+-- Keep the last two functions generated to compare results.
 local s = {}
 local test_res = true
 
===================================================================

The branch is force-pushed.

> > +local s = {}
> > +local test_res = true
> > +
> > +-- XXX: The number of iteration is fragile, depending on the trace
> > +-- length and the amount of currently pre-allocated mcode area.
> > +-- Usually works under 100, which doesn't take too long in case of
> > +-- success, so I gave up to locate a better way to chomp the mcode
> > +-- area.
> > +
> > +for n = 1, 100 do
> > + local src = string.format([[
> > + function f%d(x, y)
> 
> Not sure if indentation is preserved in the patch and should it be here?

Yes, this is something strange with offset here. I've used 2 spaces as
usual.

> 
> > + local a = {}

<snipped>

> >> IM
> > 
> > [1]: https://github.com/tarantool/luajit/tree/sergos/gh-4095-gc64-const-fusion <https://github.com/tarantool/luajit/tree/sergos/gh-4095-gc64-const-fusion>
> > [2]: https://github.com/tarantool/luajit/tree/skaplun/gc64-constant-fuse-full-ci <https://github.com/tarantool/luajit/tree/skaplun/gc64-constant-fuse-full-ci>
> > [3]: https://github.com/tarantool/tarantool/tree/skaplun/gh-4199-gc64-fuse-full-ci <https://github.com/tarantool/tarantool/tree/skaplun/gh-4199-gc64-fuse-full-ci>
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 

-- 
Best regards,
Sergey Kaplun