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

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

[Sergey Kaplun via Tarantool-patches]

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. During dump of 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. That gives the LOOP
bytecodes instead RET as expected.

This patch fixes the bytecode unpatching via copy the original start
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.
+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

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

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!
Just some nits in changelog, LGTM

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?

> +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
> 

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, when all nits left by Sergos are fixed.

-- 
Best regards,
IM

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 27.01.22, Sergey Kaplun 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. During dump of 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. That gives the LOOP
> bytecodes instead RET as expected.
> 
> This patch fixes the bytecode unpatching via copy the original start
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM