Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching.
@ 2022-01-27 11:53 Sergey Kaplun via Tarantool-patches
  2022-06-27 16:04 ` sergos via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-01-27 11:53 UTC (permalink / raw)
  To: Sergey Ostanevich, Igor Munkin; +Cc: 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching.
  2022-01-27 11:53 [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching Sergey Kaplun via Tarantool-patches
@ 2022-06-27 16:04 ` sergos via Tarantool-patches
  2022-06-28  6:57   ` Sergey Kaplun via Tarantool-patches
  2022-06-28  9:12 ` Igor Munkin via Tarantool-patches
  2022-06-30 12:10 ` Igor Munkin via Tarantool-patches
  2 siblings, 1 reply; 5+ messages in thread
From: sergos via Tarantool-patches @ 2022-06-27 16:04 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching.
  2022-06-27 16:04 ` sergos via Tarantool-patches
@ 2022-06-28  6:57   ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-06-28  6:57 UTC (permalink / raw)
  To: sergos; +Cc: 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching.
  2022-01-27 11:53 [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching Sergey Kaplun via Tarantool-patches
  2022-06-27 16:04 ` sergos via Tarantool-patches
@ 2022-06-28  9:12 ` Igor Munkin via Tarantool-patches
  2022-06-30 12:10 ` Igor Munkin via Tarantool-patches
  2 siblings, 0 replies; 5+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-06-28  9:12 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

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

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching.
  2022-01-27 11:53 [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching Sergey Kaplun via Tarantool-patches
  2022-06-27 16:04 ` sergos via Tarantool-patches
  2022-06-28  9:12 ` Igor Munkin via Tarantool-patches
@ 2022-06-30 12:10 ` Igor Munkin via Tarantool-patches
  2 siblings, 0 replies; 5+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-06-30 12:10 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-06-30 12:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27 11:53 [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching Sergey Kaplun via Tarantool-patches
2022-06-27 16:04 ` sergos via Tarantool-patches
2022-06-28  6:57   ` Sergey Kaplun via Tarantool-patches
2022-06-28  9:12 ` Igor Munkin via Tarantool-patches
2022-06-30 12:10 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox