Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
@ 2023-08-25 15:00 Sergey Kaplun via Tarantool-patches
  2023-08-28 12:58 ` Sergey Bronnikov via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-08-25 15:00 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

From: Mike Pall <mike>

Analyzed by Sergey Kaplun.

(cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9)

Recording of the `BC_TSETM` bytecode may keep too optimistic JIT
maxslot. In that case, the slot above the top of the Lua stack may be
considered used. When any VM event handler is called before the
recording of the next instruction, this leads to an assertion failure in
`rec_check_slots()`.

This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a
table, which is always the highest slot after this bytecode. Also, it
adds an assertion that we check slots below the top of the Lua stack.

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

Part of tarantool/tarantool#8825
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1025-tsetm-maxslot
Tarantool PR: https://github.com/tarantool/tarantool/pull/9040
Issues:
* https://github.com/LuaJIT/LuaJIT/issues/1025
* https://github.com/tarantool/tarantool/issues/8825

 src/lj_record.c                               |  2 +
 .../lj-1025-tsetm-maxslot.test.lua            | 52 +++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index 34d1210a..58b040ec 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -115,6 +115,7 @@ static void rec_check_slots(jit_State *J)
       cTValue *tv = &base[s];
       IRRef ref = tref_ref(tr);
       IRIns *ir = NULL;  /* Silence compiler. */
+      lj_assertJ(tv < J->L->top, "slot %d above top of Lua stack", s);
       if (!LJ_FR2 || ref || !(tr & (TREF_FRAME | TREF_CONT))) {
 	lj_assertJ(ref >= J->cur.nk && ref < J->cur.nins,
 		   "slot %d ref %04d out of range", s, ref - REF_BIAS);
@@ -2342,6 +2343,7 @@ void lj_record_ins(jit_State *J)
 
   case BC_TSETM:
     rec_tsetm(J, ra, (BCReg)(J->L->top - J->L->base), (int32_t)rcv->u32.lo);
+    J->maxslot = ra;  /* The table slot at ra-1 is the highest used slot. */
     break;
 
   case BC_TNEW:
diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
new file mode 100644
index 00000000..7ae0a99d
--- /dev/null
+++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
@@ -0,0 +1,52 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT incorrect recording of `TSETM`
+-- bytecode.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1025.
+
+local test = tap.test('lj-1025-tsetm-maxslot'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local jit_dump = require('jit.dump')
+
+local TEST_VALUE = '5'
+local TEST_IDX = 5
+
+local function slot5()
+  return nil, nil, nil, nil, TEST_VALUE
+end
+
+local storage
+local function test_tsetm(...)
+  -- Usage of `TSETM` bytecode.
+  storage = {slot5()}
+  -- Use this function again to trick use-def analysis and avoid
+  -- cleaning JIT slots, so the last JIT slot contains
+  -- `TEST_VALUE`.
+  return slot5(...)
+end
+
+-- Wrapper to avoid the recording of just the inner `slot5()`
+-- function.
+local function wrap()
+  test_tsetm()
+end
+
+jit.opt.start('hotloop=1')
+-- We need to call the VM event handler after each recorded bytecode
+-- instruction to pollute the Lua stack and the issue
+-- becomes observable.
+jit_dump.start('b', '/dev/null')
+
+-- Compile and execute the trace with `TSETM`.
+wrap()
+wrap()
+wrap()
+
+test:is(storage[TEST_IDX], TEST_VALUE,
+        'BC_TSETM recording with enabled jit.dump')
+
+test:done(true)
-- 
2.41.0


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

* Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
  2023-08-25 15:00 [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM Sergey Kaplun via Tarantool-patches
@ 2023-08-28 12:58 ` Sergey Bronnikov via Tarantool-patches
  2023-08-28 14:04   ` Sergey Kaplun via Tarantool-patches
  2023-08-28 15:00 ` Maxim Kokryashkin via Tarantool-patches
  2023-09-01 15:45 ` Igor Munkin via Tarantool-patches
  2 siblings, 1 reply; 8+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-08-28 12:58 UTC (permalink / raw)
  To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Sergey


thanks for the patch! LGTM

See a minor comment inline.


Sincerely yours,

Sergey


On 8/25/23 18:00, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Analyzed by Sergey Kaplun.
>
> (cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9)
>
> Recording of the `BC_TSETM` bytecode may keep too optimistic JIT
> maxslot. In that case, the slot above the top of the Lua stack may be
> considered used. When any VM event handler is called before the
> recording of the next instruction, this leads to an assertion failure in
> `rec_check_slots()`.
>
> This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a
> table, which is always the highest slot after this bytecode. Also, it
> adds an assertion that we check slots below the top of the Lua stack.

I've discovered that bug is not reproduced when LUAJIT_ENABLE_TABLE_BUMP 
is enabled, by default it is disabled.

CFLAGS="-DLUAJIT_ENABLE_TABLE_BUMP" cmake -S . -B build 
-DCMAKE_BUILD_TYPE=Debug

cmake --build build --parallel

It is probably worth to mention in commit message. Feel free to ignore.



<snipped>


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

* Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
  2023-08-28 12:58 ` Sergey Bronnikov via Tarantool-patches
@ 2023-08-28 14:04   ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-08-28 14:04 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the review!

On 28.08.23, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> 
> thanks for the patch! LGTM
> 
> See a minor comment inline.
> 
> 
> Sincerely yours,
> 
> Sergey
> 
> 
> On 8/25/23 18:00, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Analyzed by Sergey Kaplun.
> >

<snipped>

> 
> I've discovered that bug is not reproduced when LUAJIT_ENABLE_TABLE_BUMP 
> is enabled, by default it is disabled.
> 
> CFLAGS="-DLUAJIT_ENABLE_TABLE_BUMP" cmake -S . -B build 
> -DCMAKE_BUILD_TYPE=Debug
> 
> cmake --build build --parallel
> 
> It is probably worth to mention in commit message. Feel free to ignore.

I suppose, since it's not enabled by default it's better to ignore it.
It's  hidden due-to DCE optimizations, most probably.

Ignoring.

> 
> 
> 
> <snipped>
> 

-- 
Best regards,
Sergey Kaplun

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

* Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
  2023-08-25 15:00 [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM Sergey Kaplun via Tarantool-patches
  2023-08-28 12:58 ` Sergey Bronnikov via Tarantool-patches
@ 2023-08-28 15:00 ` Maxim Kokryashkin via Tarantool-patches
  2023-08-28 15:02   ` Sergey Kaplun via Tarantool-patches
  2023-09-01 15:45 ` Igor Munkin via Tarantool-patches
  2 siblings, 1 reply; 8+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-08-28 15:00 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Fri, Aug 25, 2023 at 06:00:24PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Analyzed by Sergey Kaplun.
> 
> (cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9)
> 
> Recording of the `BC_TSETM` bytecode may keep too optimistic JIT
> maxslot. In that case, the slot above the top of the Lua stack may be
Typo: s/too optimistic JIT maxslot./the JIT maxslot too optimistic./
> considered used. When any VM event handler is called before the
> recording of the next instruction, this leads to an assertion failure in
> `rec_check_slots()`.
> 
> This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a
> table, which is always the highest slot after this bytecode. Also, it
> adds an assertion that we check slots below the top of the Lua stack.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1025-tsetm-maxslot
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9040
> Issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1025
> * https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_record.c                               |  2 +
>  .../lj-1025-tsetm-maxslot.test.lua            | 52 +++++++++++++++++++
>  2 files changed, 54 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> 
> diff --git a/src/lj_record.c b/src/lj_record.c
> index 34d1210a..58b040ec 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -115,6 +115,7 @@ static void rec_check_slots(jit_State *J)
>        cTValue *tv = &base[s];
>        IRRef ref = tref_ref(tr);
>        IRIns *ir = NULL;  /* Silence compiler. */
> +      lj_assertJ(tv < J->L->top, "slot %d above top of Lua stack", s);
>        if (!LJ_FR2 || ref || !(tr & (TREF_FRAME | TREF_CONT))) {
>  	lj_assertJ(ref >= J->cur.nk && ref < J->cur.nins,
>  		   "slot %d ref %04d out of range", s, ref - REF_BIAS);
> @@ -2342,6 +2343,7 @@ void lj_record_ins(jit_State *J)
>  
>    case BC_TSETM:
>      rec_tsetm(J, ra, (BCReg)(J->L->top - J->L->base), (int32_t)rcv->u32.lo);
> +    J->maxslot = ra;  /* The table slot at ra-1 is the highest used slot. */
>      break;
>  
>    case BC_TNEW:
> diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> new file mode 100644
> index 00000000..7ae0a99d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> @@ -0,0 +1,52 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT incorrect recording of `TSETM`
> +-- bytecode.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1025.
> +
> +local test = tap.test('lj-1025-tsetm-maxslot'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local jit_dump = require('jit.dump')
> +
> +local TEST_VALUE = '5'
> +local TEST_IDX = 5
> +
> +local function slot5()
> +  return nil, nil, nil, nil, TEST_VALUE
> +end
Why the fifth slot? Drop a comment.
> +
> +local storage
> +local function test_tsetm(...)
> +  -- Usage of `TSETM` bytecode.
> +  storage = {slot5()}
> +  -- Use this function again to trick use-def analysis and avoid
> +  -- cleaning JIT slots, so the last JIT slot contains
> +  -- `TEST_VALUE`.
> +  return slot5(...)
> +end
> +
> +-- Wrapper to avoid the recording of just the inner `slot5()`
> +-- function.
> +local function wrap()
> +  test_tsetm()
> +end
> +
> +jit.opt.start('hotloop=1')
> +-- We need to call the VM event handler after each recorded bytecode
> +-- instruction to pollute the Lua stack and the issue
> +-- becomes observable.
Typo: s/and the issue becomes/and make the issue/
> +jit_dump.start('b', '/dev/null')
> +
> +-- Compile and execute the trace with `TSETM`.
> +wrap()
> +wrap()
> +wrap()
> +
> +test:is(storage[TEST_IDX], TEST_VALUE,
> +        'BC_TSETM recording with enabled jit.dump')
> +
> +test:done(true)
> -- 
> 2.41.0
> 

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

* Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
  2023-08-28 15:00 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-08-28 15:02   ` Sergey Kaplun via Tarantool-patches
  2023-08-28 15:16     ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-08-28 15:02 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Maxim!
Thanks for the review!
I've updated the patch considering your comments and force-pushed the
branch.

On 28.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> On Fri, Aug 25, 2023 at 06:00:24PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> > Analyzed by Sergey Kaplun.
> > 
> > (cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9)
> > 
> > Recording of the `BC_TSETM` bytecode may keep too optimistic JIT
> > maxslot. In that case, the slot above the top of the Lua stack may be
> Typo: s/too optimistic JIT maxslot./the JIT maxslot too optimistic./

Fixed.

> > considered used. When any VM event handler is called before the
> > recording of the next instruction, this leads to an assertion failure in
> > `rec_check_slots()`.
> > 
> > This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a
> > table, which is always the highest slot after this bytecode. Also, it
> > adds an assertion that we check slots below the top of the Lua stack.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#8825
> > ---

<snipped>

> > +
> > +local TEST_VALUE = '5'
> > +local TEST_IDX = 5
> > +
> > +local function slot5()
> > +  return nil, nil, nil, nil, TEST_VALUE
> > +end
> Why the fifth slot? Drop a comment.

Fixed. See the iterative patch below.

> > +
> > +local storage
> > +local function test_tsetm(...)
> > +  -- Usage of `TSETM` bytecode.
> > +  storage = {slot5()}
> > +  -- Use this function again to trick use-def analysis and avoid
> > +  -- cleaning JIT slots, so the last JIT slot contains
> > +  -- `TEST_VALUE`.
> > +  return slot5(...)
> > +end
> > +
> > +-- Wrapper to avoid the recording of just the inner `slot5()`
> > +-- function.
> > +local function wrap()
> > +  test_tsetm()
> > +end
> > +
> > +jit.opt.start('hotloop=1')
> > +-- We need to call the VM event handler after each recorded bytecode
> > +-- instruction to pollute the Lua stack and the issue
> > +-- becomes observable.
> Typo: s/and the issue becomes/and make the issue/

Fixed.

See the iterative patch below:

===================================================================
diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
index 7ae0a99d..74625a79 100644
--- a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
+++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
@@ -15,6 +15,8 @@ local jit_dump = require('jit.dump')
 local TEST_VALUE = '5'
 local TEST_IDX = 5
 
+-- XXX: Use big enough slot numbewr to be overwritten by VM event
+-- handler function.
 local function slot5()
   return nil, nil, nil, nil, TEST_VALUE
 end
@@ -36,9 +38,9 @@ local function wrap()
 end
 
 jit.opt.start('hotloop=1')
--- We need to call the VM event handler after each recorded bytecode
--- instruction to pollute the Lua stack and the issue
--- becomes observable.
+-- We need to call the VM event handler after each recorded
+-- bytecode instruction to pollute the Lua stack and make the
+-- issue observable.
 jit_dump.start('b', '/dev/null')
 
 -- Compile and execute the trace with `TSETM`.
===================================================================

> > +jit_dump.start('b', '/dev/null')
> > +

<snipped>

> > -- 
> > 2.41.0
> > 

-- 
Best regards,
Sergey Kaplun

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

* Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
  2023-08-28 15:02   ` Sergey Kaplun via Tarantool-patches
@ 2023-08-28 15:16     ` Maxim Kokryashkin via Tarantool-patches
  2023-08-28 15:19       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-08-28 15:16 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the fixes!
LGTM, except for a few more comments below.
On Mon, Aug 28, 2023 at 06:02:12PM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the review!
> I've updated the patch considering your comments and force-pushed the
> branch.
> 
> On 28.08.23, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> > 
> > On Fri, Aug 25, 2023 at 06:00:24PM +0300, Sergey Kaplun wrote:
> > > From: Mike Pall <mike>
> > > 
> > > Analyzed by Sergey Kaplun.
> > > 
> > > (cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9)
> > > 
> > > Recording of the `BC_TSETM` bytecode may keep too optimistic JIT
> > > maxslot. In that case, the slot above the top of the Lua stack may be
> > Typo: s/too optimistic JIT maxslot./the JIT maxslot too optimistic./
> 
> Fixed.
> 
> > > considered used. When any VM event handler is called before the
> > > recording of the next instruction, this leads to an assertion failure in
> > > `rec_check_slots()`.
> > > 
> > > This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a
> > > table, which is always the highest slot after this bytecode. Also, it
> > > adds an assertion that we check slots below the top of the Lua stack.
> > > 
> > > Sergey Kaplun:
> > > * added the description and the test for the problem
> > > 
> > > Part of tarantool/tarantool#8825
> > > ---
> 
> <snipped>
> 
> > > +
> > > +local TEST_VALUE = '5'
> > > +local TEST_IDX = 5
> > > +
> > > +local function slot5()
> > > +  return nil, nil, nil, nil, TEST_VALUE
> > > +end
> > Why the fifth slot? Drop a comment.
> 
> Fixed. See the iterative patch below.
> 
> > > +
> > > +local storage
> > > +local function test_tsetm(...)
> > > +  -- Usage of `TSETM` bytecode.
> > > +  storage = {slot5()}
> > > +  -- Use this function again to trick use-def analysis and avoid
> > > +  -- cleaning JIT slots, so the last JIT slot contains
> > > +  -- `TEST_VALUE`.
> > > +  return slot5(...)
> > > +end
> > > +
> > > +-- Wrapper to avoid the recording of just the inner `slot5()`
> > > +-- function.
> > > +local function wrap()
> > > +  test_tsetm()
> > > +end
> > > +
> > > +jit.opt.start('hotloop=1')
> > > +-- We need to call the VM event handler after each recorded bytecode
> > > +-- instruction to pollute the Lua stack and the issue
> > > +-- becomes observable.
> > Typo: s/and the issue becomes/and make the issue/
> 
> Fixed.
> 
> See the iterative patch below:
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> index 7ae0a99d..74625a79 100644
> --- a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> +++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> @@ -15,6 +15,8 @@ local jit_dump = require('jit.dump')
>  local TEST_VALUE = '5'
>  local TEST_IDX = 5
>  
> +-- XXX: Use big enough slot numbewr to be overwritten by VM event
Typo: s/Use/Use a/
Typo: s/numbewr/number/
Typo: s/by VM/by the VM/
> +-- handler function.
Please also add that this is an empirical number.
>  local function slot5()
>    return nil, nil, nil, nil, TEST_VALUE
>  end
> @@ -36,9 +38,9 @@ local function wrap()
>  end
>  
>  jit.opt.start('hotloop=1')
> --- We need to call the VM event handler after each recorded bytecode
> --- instruction to pollute the Lua stack and the issue
> --- becomes observable.
> +-- We need to call the VM event handler after each recorded
> +-- bytecode instruction to pollute the Lua stack and make the
> +-- issue observable.
>  jit_dump.start('b', '/dev/null')
>  
>  -- Compile and execute the trace with `TSETM`.
> ===================================================================
> 
> > > +jit_dump.start('b', '/dev/null')
> > > +
> 
> <snipped>
> 
> > > -- 
> > > 2.41.0
> > > 
> 
> -- 
> Best regards,
> Sergey Kaplun

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

* Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
  2023-08-28 15:16     ` Maxim Kokryashkin via Tarantool-patches
@ 2023-08-28 15:19       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-08-28 15:19 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Maxim!
Thanks for the comments!
Fixed your comments inline.

On 28.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, except for a few more comments below.
> On Mon, Aug 28, 2023 at 06:02:12PM +0300, Sergey Kaplun wrote:

<snipped>

> > > > +
> > > > +local TEST_VALUE = '5'
> > > > +local TEST_IDX = 5
> > > > +
> > > > +local function slot5()
> > > > +  return nil, nil, nil, nil, TEST_VALUE
> > > > +end
> > > Why the fifth slot? Drop a comment.
> > 
> > Fixed. See the iterative patch below.
> > 

<snipped>

> > 
> > ===================================================================
> > diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> > index 7ae0a99d..74625a79 100644
> > --- a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> > +++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> > @@ -15,6 +15,8 @@ local jit_dump = require('jit.dump')
> >  local TEST_VALUE = '5'
> >  local TEST_IDX = 5
> >  
> > +-- XXX: Use big enough slot numbewr to be overwritten by VM event
> Typo: s/Use/Use a/
> Typo: s/numbewr/number/
> Typo: s/by VM/by the VM/
> > +-- handler function.
> Please also add that this is an empirical number.

Fixed, thanks.

===================================================================
diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
index 74625a79..0e82c0b9 100644
--- a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
+++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
@@ -15,8 +15,8 @@ local jit_dump = require('jit.dump')
 local TEST_VALUE = '5'
 local TEST_IDX = 5
 
--- XXX: Use big enough slot numbewr to be overwritten by VM event
--- handler function.
+-- XXX: Use a big enough slot number to be overwritten by the VM
+-- event handler function. This value is empirical.
 local function slot5()
   return nil, nil, nil, nil, TEST_VALUE
 end
===================================================================

<snipped>

-- 
Best regards,
Sergey Kaplun

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

* Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.
  2023-08-25 15:00 [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM Sergey Kaplun via Tarantool-patches
  2023-08-28 12:58 ` Sergey Bronnikov via Tarantool-patches
  2023-08-28 15:00 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-09-01 15:45 ` Igor Munkin via Tarantool-patches
  2 siblings, 0 replies; 8+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-09-01 15:45 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

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

On 25.08.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Analyzed by Sergey Kaplun.
> 
> (cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9)
> 
> Recording of the `BC_TSETM` bytecode may keep too optimistic JIT
> maxslot. In that case, the slot above the top of the Lua stack may be
> considered used. When any VM event handler is called before the
> recording of the next instruction, this leads to an assertion failure in
> `rec_check_slots()`.
> 
> This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a
> table, which is always the highest slot after this bytecode. Also, it
> adds an assertion that we check slots below the top of the Lua stack.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1025-tsetm-maxslot
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9040
> Issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1025
> * https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_record.c                               |  2 +
>  .../lj-1025-tsetm-maxslot.test.lua            | 52 +++++++++++++++++++
>  2 files changed, 54 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
> 

<snipped>

> -- 
> 2.41.0
> 

-- 
Best regards,
IM

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

end of thread, other threads:[~2023-09-01 16:02 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-25 15:00 [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM Sergey Kaplun via Tarantool-patches
2023-08-28 12:58 ` Sergey Bronnikov via Tarantool-patches
2023-08-28 14:04   ` Sergey Kaplun via Tarantool-patches
2023-08-28 15:00 ` Maxim Kokryashkin via Tarantool-patches
2023-08-28 15:02   ` Sergey Kaplun via Tarantool-patches
2023-08-28 15:16     ` Maxim Kokryashkin via Tarantool-patches
2023-08-28 15:19       ` Sergey Kaplun via Tarantool-patches
2023-09-01 15:45 ` 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