Re: [Tarantool-patches] [PATCH luajit] Prevent down-recursion for side traces.

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

Hi, Sergey!
Thanks for the review!
I've fixed your comments, rebased the branch on the current
tarantool/master, and force-pushed it.

On 26.03.24, Sergey Bronnikov wrote:
> Sergey,
> 
> thanks for the patch. LGTM with three minor comments below.
> 
> Sergey
> 
> On 3/19/24 19:41, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun.
> >
> > (cherry picked from commit cae361187e7e1e3545353fb560c032cdace32d5f)
> >
> > Assume we have the root trace that uses some spill slots with the
> > corresponding stack adjustment. Then its side trace will restore stack
> s/stack/the stack/

Fixed, thanks.

> > only at its tail. It may look like the following:
> >
> > | ---- TRACE 4 mcode 1247
> > | 55557f7df953  mov rax, [r14-0xe28]
> > | 55557f7df95a  mov rax, [rax+0x30]
> > | 55557f7df95e  sub rax, rdx
> > | 55557f7df961  cmp rax, +0x68
> > | 55557f7df965  jb 0x55557f7d004c       ->0
> > | 55557f7df96b  add rsp, -0x10
> > | ...
> > | 55557f6efa71  cmp dword [rdx+0x4], -0x05
> > | 55557f6efa75  jnz 0x55557f6e004c      ->0
> > | ...
> > | 55557f7dfe29  add rsp, +0x10
> > | 55557f7dfe2d  jmp 0x5555556fe573
> > | ---- TRACE 4 stop -> stitch
> > |
> > | ---- TRACE 5 start 4/0
> > | ---- TRACE 5 mcode 101
> > | 55557f6ef9d4  mov dword [0x40000518], 0x5
> > | ...
> > | 55557f6efa30  add rsp, +0x10
> > | 55557f6efa34  jmp 0x55557f6ef9d4
> > | ---- TRACE 5 stop -> down-recursion
> >
> > Such side traces have no stack addjustment at their heads since their
> s/addjustment/adjustment/

Fixed, thanks!

> > stack addjustment is inherited from the parent trace. The issue occurs
> > if the side trace has a down-recursion, as mentioned above. Before any
> > exit, we can jump back to the start of the trace several times with
> > growing `rsp`. In that case, the `rsp` is restored incorrectly after
> > exiting from the trace.
> >
> > This patch forbids down-recursion for non-root traces.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9595
> > ---
> >
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1169-down-rec-side
> > Related issues:
> > * https://github.com/tarantool/tarantool/issues/9595
> > * https://github.com/LuaJIT/LuaJIT/issues/1169

<snipped>

> > +
> > +-- If a parent trace has more than the default amount of spill
> > +-- slots, the `rsp` register is adjusted at the start of the trace
> > +-- and restored after. If there is a side trace created, it
> > +-- modifies the stack only at exit (since adjustment is inherited
> > +-- from a parent trace). If the side trace has down-recursion (for
> s/own-recursion/a down-recursion/

Fixed, see the iterative patch below.

===================================================================
diff --git a/test/tarantool-tests/lj-1169-down-rec-side.test.lua b/test/tarantool-tests/lj-1169-down-rec-side.test.lua
index 63f9925f..6363edd7 100644
--- a/test/tarantool-tests/lj-1169-down-rec-side.test.lua
+++ b/test/tarantool-tests/lj-1169-down-rec-side.test.lua
@@ -22,10 +22,10 @@ test:plan(1)
 -- slots, the `rsp` register is adjusted at the start of the trace
 -- and restored after. If there is a side trace created, it
 -- modifies the stack only at exit (since adjustment is inherited
--- from a parent trace). If the side trace has down-recursion (for
--- now only down-recursion to itself is used), `rsp` may be
--- modified several times before exit, so the host stack becomes
--- corrupted.
+-- from a parent trace). If the side trace has a down-recursion
+-- (for now only the down-recursion to itself is used), `rsp` may
+-- be modified several times before exit, so the host stack
+-- becomes corrupted.
 --
 -- This test provides the example of a side trace (5) with
 -- down-recursion.
===================================================================

> > +-- now only down-recursion to itself is used), `rsp` may be

<snipped>

-- 
Best regards,
Sergey Kaplun