Re: [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching.

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

Hi, Sergos!

Thanks for the review!

On 27.06.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> Just some nits in changelog, LGTM

I've updated commit message to the following:

===================================================================
Fix bytecode dump unpatching.

Reported by Christopher Oliver.

(cherry picked from commit 20ac817a747cf8cab044ae81b09c08d23e34342b)

RET bytecodes are patched to JLOOP bytecode in a function with
up-recursion. During dump those bytecodes they should be unpatched to
the original one. It is done by restoring the opcode by subtraction the
diff between JLOOP and ILOOP bytecodes. The restore was done by the
erroneous opcode subtraction, that led to a LOOP bytecode in place of
the RET one.

This patch fixes the bytecode unpatching via copy of the original
instruction, that was patched.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#6548
===================================================================

> 
> Sergos
> 
> > On 27 Jan 2022, at 14:53, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > Reported by Christopher Oliver.
> > 
> > (cherry picked from commit 20ac817a747cf8cab044ae81b09c08d23e34342b)
> > 
> > When a compiled function with up-recursion RET bytecodes are patched to
> > JLOOP bytecode.
> 
> If I got it right? 
> “RET bytecodes are patched to JLOOP bytecode in a function with up-recursion."
> 
> > During dump of those bytecodes they should be unpatched
>              ^^^                ^^^^^ remove 2 words
> 
> > to the original one.
> > It is done by restoring the opcode by subtraction
> 
> > the diff between JLOOP and ILOOP bytecodes. That gives the LOOP
> > bytecodes instead RET as expected.
> 
> The restore was done by the erroneous opcode subtraction, that led to a LOOP
> bytecode in place of the RET one.
> 
> > This patch fixes the bytecode unpatching via copy the original start
>                                                   of             ????
> > instruction, that was patched.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#6548
> > ---
> > 
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-wrong-bc-ret
> > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-wrong-bc-ret-full-ci
> > Related issue: https://github.com/tarantool/tarantool/issues/6548
> > 
> > src/lj_bcwrite.c                              |  5 +----
> > .../bc-jit-unpatching.test.lua                | 22 +++++++++++++++++++
> > 2 files changed, 23 insertions(+), 4 deletions(-)
> > create mode 100644 test/tarantool-tests/bc-jit-unpatching.test.lua
> > 
> > diff --git a/src/lj_bcwrite.c b/src/lj_bcwrite.c
> > index 5e05caea..a86d6d00 100644
> > --- a/src/lj_bcwrite.c
> > +++ b/src/lj_bcwrite.c
> > @@ -219,10 +219,7 @@ static char *bcwrite_bytecode(BCWriteCtx *ctx, char *p, GCproto *pt)
> > 	q[LJ_ENDIAN_SELECT(0, 3)] = (uint8_t)(op-BC_IFORL+BC_FORL);
> >       } else if (op == BC_JFORL || op == BC_JITERL || op == BC_JLOOP) {
> > 	BCReg rd = q[LJ_ENDIAN_SELECT(2, 1)] + (q[LJ_ENDIAN_SELECT(3, 0)] << 8);
> > -	BCIns ins = traceref(J, rd)->startins;
> > -	q[LJ_ENDIAN_SELECT(0, 3)] = (uint8_t)(op-BC_JFORL+BC_FORL);
> > -	q[LJ_ENDIAN_SELECT(2, 1)] = bc_c(ins);
> > -	q[LJ_ENDIAN_SELECT(3, 0)] = bc_b(ins);
> > +	memcpy(q, &traceref(J, rd)->startins, 4);
> >       }
> >     }
> >   }
> > diff --git a/test/tarantool-tests/bc-jit-unpatching.test.lua b/test/tarantool-tests/bc-jit-unpatching.test.lua
> > new file mode 100644
> > index 00000000..9f9cb390
> > --- /dev/null
> > +++ b/test/tarantool-tests/bc-jit-unpatching.test.lua
> > @@ -0,0 +1,22 @@
> > +local tap = require('tap')
> > +local utils = require('utils')
> > +
> > +local test = tap.test('bc-jit-unpatching')
> > +test:plan(1)
> > +
> > +-- Function with up-recursion.
> > +local function f(n)
> > +  return n < 2 and n or f(n - 1) + f(n - 2)
> > +end
> > +
> > +local ret1bc = 'RET1%s*1%s*2'
> > +-- Check that this bytecode still persists.
> > +assert(utils.hasbc(loadstring(string.dump(f)), ret1bc))
> > +
> > +-- Compile function to get JLOOP bytecode in recursion.
> 
> Do you need any jit.opt.start(‘hotloop=1’) here?

Yes, we can reduce amount of recursion calls with it:

===================================================================
diff --git a/test/tarantool-tests/bc-jit-unpatching.test.lua b/test/tarantool-tests/bc-jit-unpatching.test.lua
index 9f9cb390..6a5bed52 100644
--- a/test/tarantool-tests/bc-jit-unpatching.test.lua
+++ b/test/tarantool-tests/bc-jit-unpatching.test.lua
@@ -13,8 +13,9 @@ local ret1bc = 'RET1%s*1%s*2'
 -- Check that this bytecode still persists.
 assert(utils.hasbc(loadstring(string.dump(f)), ret1bc))

+jit.opt.start('hotloop=1', 'hotexit=1')
 -- Compile function to get JLOOP bytecode in recursion.
-f(10)
+f(5)

 test:ok(utils.hasbc(loadstring(string.dump(f)), ret1bc),
         'bytecode unpached correctly')
===================================================================

> 
> > +f(10)
> > +
> > +test:ok(utils.hasbc(loadstring(string.dump(f)), ret1bc),
> > +        'bytecode unpached correctly')
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun