[Tarantool-patches] [PATCH luajit] Avoid assertion in case of stack overflow from stitched trace.

[Tarantool-patches] [PATCH luajit] Avoid assertion in case of stack overflow from stitched trace.

[Maxim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Bronnikov. Fixed by Sergey Kaplun.

(cherry-picked from commit 1d75cd4d7be638babe6d4e47bf73ea05fc65d81c)

When we exit from a stitched trace due to the Lua stack overflow
error, the Lua and JIT stacks are not synchronized, and they
won't be as long as the mentioned error is raised. Because of
that, we get the incorrect bytecode instruction in
`debug_framepc`. This patch fixes this behavior, so the
`debug_framepc` now returns `NO_BCPOS` for this case.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-913-avoid-assertion-stkov-from-stitched-trace
PR: https://github.com/tarantool/tarantool/pull/9484
Issues: https://github.com/tarantool/tarantool/issues/9145
https://github.com/LuaJIT/LuaJIT/issues/913

 src/lj_debug.c                                |  9 +++++---
 ...-913-stackoverflow-stitched-trace.test.lua | 23 +++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua

diff --git a/src/lj_debug.c b/src/lj_debug.c
index 46c442c6..107f464c 100644
--- a/src/lj_debug.c
+++ b/src/lj_debug.c
@@ -101,9 +101,12 @@ static BCPos debug_framepc(lua_State *L, GCfunc *fn, cTValue *nextframe)
   pos = proto_bcpos(pt, ins) - 1;
 #if LJ_HASJIT
   if (pos > pt->sizebc) {  /* Undo the effects of lj_trace_exit for JLOOP. */
-    GCtrace *T = (GCtrace *)((char *)(ins-1) - offsetof(GCtrace, startins));
-    lj_assertL(bc_isret(bc_op(ins[-1])), "return bytecode expected");
-    pos = proto_bcpos(pt, mref(T->startpc, const BCIns));
+    if (bc_isret(bc_op(ins[-1]))) {
+      GCtrace *T = (GCtrace *)((char *)(ins-1) - offsetof(GCtrace, startins));
+      pos = proto_bcpos(pt, mref(T->startpc, const BCIns));
+    } else {
+      pos = NO_BCPOS;  /* Punt in case of stack overflow for stitched trace. */
+    }
   }
 #endif
   return pos;
diff --git a/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
new file mode 100644
index 00000000..3c12f0d9
--- /dev/null
+++ b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
@@ -0,0 +1,23 @@
+local tap = require('tap')
+-- Test to demonstrate the incorrect LuaJIT behavior when exiting
+-- from a snapshot for stitched trace.
+local test = tap.test('lj-913-stackoverflow-stitched-trace'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(3)
+
+-- Recursion to cause stack overflow.
+local function callee()
+  -- `math.fmod()` is NYI, so trace will be stitched here.
+  local _ = math.fmod(42, 42)
+  callee()
+end
+
+local st, err = pcall(callee)
+
+test:ok(true, 'assertion is not triggered')
+test:ok(not st, 'error happened')
+test:like(err, 'stack overflow', 'stack overflow happened')
+
+test:done(true)
--
2.43.0

Re: [Tarantool-patches] [PATCH luajit] Avoid assertion in case of stack overflow from stitched trace.

[Sergey Kaplun via Tarantool-patches]

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

On 14.12.23, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Bronnikov. Fixed by Sergey Kaplun.
> 
> (cherry-picked from commit 1d75cd4d7be638babe6d4e47bf73ea05fc65d81c)
> 
> When we exit from a stitched trace due to the Lua stack overflow
> error, the Lua and JIT stacks are not synchronized, and they
> won't be as long as the mentioned error is raised. Because of

The reason is not the stack synchornization, but the determination of
the bytecode when the error is raised. In the case of stitched trace
(see the dumps below), the last slot in the snapshot map is the argument
for the function to stitch (in our case, ref to 42). Or for the second
trace, we have any other function where the recording is stopped but not
the function where it was started.
So determine bytecode position isn't trivial. And it is better just
return `NO_BCPOS` in such cases.

| Trace 1 start
| ---- TRACE IR
| ....          SNAP   #0   [ ---- ]
| 0001      fun SLOAD  [L ] lit: #0   lit: R
| 0002      tab FLOAD  [L ] ref: 0001 lit: func.env
| 0003      int FLOAD  [L ] ref: 0002 lit: tab.hmask
| 0004   >  int EQ     [C ] ref: 0003 ref: integer 63
| 0005      p32 FLOAD  [L ] ref: 0002 lit: tab.node
| 0006   >  p32 HREFK  [R ] ref: 0005 ref: string "math" @ 0x400061e8 KSLOT: @54
| 0007   >  tab HLOAD  [L ] ref: 0006
| 0008      int FLOAD  [L ] ref: 0007 lit: tab.hmask
| 0009   >  int EQ     [C ] ref: 0008 ref: integer 31
| 0010      p32 FLOAD  [L ] ref: 0007 lit: tab.node
| 0011   >  p32 HREFK  [R ] ref: 0010 ref: string "fmod" @ 0x40006c88 KSLOT: @18
| 0012   >  fun HLOAD  [L ] ref: 0011
| 0013   >  fun EQ     [C ] ref: 0012 ref: fast function #58
| ....          SNAP   #1   [ ---- {trace 0x1 @ 0x400271d8} contpc ftsz | {number 42} {number 42} ]
| 0014          NOP    [N ]
|
| Trace 2 start
| ---- TRACE IR
| ....          SNAP   #0   [ ---- ]
| 0001      fun SLOAD  [L ] lit: #0   lit: R
| 0002   >  fun EQ     [C ] ref: 0001 ref: Lua function @ 0x400157f0, 1 upvalues, "@../master/test/tarantool-tests/lj-913-so-stitched.test.lua":11
| ....          SNAP   #1   [ ftsz | ftsz | ]
| 0003          NOP    [N ]

> that, we get the incorrect bytecode instruction in
> `debug_framepc`. This patch fixes this behavior, so the
> `debug_framepc` now returns `NO_BCPOS` for this case.
> 
> Maxim Kokryashkin:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9145
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-913-avoid-assertion-stkov-from-stitched-trace
> PR: https://github.com/tarantool/tarantool/pull/9484
> Issues: https://github.com/tarantool/tarantool/issues/9145
> https://github.com/LuaJIT/LuaJIT/issues/913
> 
>  src/lj_debug.c                                |  9 +++++---
>  ...-913-stackoverflow-stitched-trace.test.lua | 23 +++++++++++++++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> 
> diff --git a/src/lj_debug.c b/src/lj_debug.c
> index 46c442c6..107f464c 100644
> --- a/src/lj_debug.c
> +++ b/src/lj_debug.c

<snipped>

> diff --git a/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> new file mode 100644
> index 00000000..3c12f0d9
> --- /dev/null
> +++ b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> @@ -0,0 +1,23 @@
> +local tap = require('tap')
> +-- Test to demonstrate the incorrect LuaJIT behavior when exiting
> +-- from a snapshot for stitched trace.
> +local test = tap.test('lj-913-stackoverflow-stitched-trace'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(3)
> +
> +-- Recursion to cause stack overflow.
> +local function callee()
> +  -- `math.fmod()` is NYI, so trace will be stitched here.
> +  local _ = math.fmod(42, 42)

We don't need this `_` variable here. Just using `math.fmod()` is
enough.

> +  callee()
> +end
> +
> +local st, err = pcall(callee)
> +
> +test:ok(true, 'assertion is not triggered')
> +test:ok(not st, 'error happened')
> +test:like(err, 'stack overflow', 'stack overflow happened')
> +
> +test:done(true)
> --
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Avoid assertion in case of stack overflow from stitched trace.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On Thu, Dec 14, 2023 at 03:20:53PM +0300, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Maxim!
> Thanks for the patch!
> Please, consider my comments below.
>
> On 14.12.23, Maxim Kokryashkin wrote:
> > From: Mike Pall <mike>
> >
> > Reported by Sergey Bronnikov. Fixed by Sergey Kaplun.
> >
> > (cherry-picked from commit 1d75cd4d7be638babe6d4e47bf73ea05fc65d81c)
> >
> > When we exit from a stitched trace due to the Lua stack overflow
> > error, the Lua and JIT stacks are not synchronized, and they
> > won't be as long as the mentioned error is raised. Because of
>
> The reason is not the stack synchornization, but the determination of
> the bytecode when the error is raised. In the case of stitched trace
> (see the dumps below), the last slot in the snapshot map is the argument
> for the function to stitch (in our case, ref to 42). Or for the second
> trace, we have any other function where the recording is stopped but not
> the function where it was started.
> So determine bytecode position isn't trivial. And it is better just
> return `NO_BCPOS` in such cases.

Fixed, thanks!
Branch is force-pushed.
Here is the new commit message:
===
Avoid assertion in case of stack overflow from stitched trace.

Reported by Sergey Bronnikov. Fixed by Sergey Kaplun.

(cherry-picked from commit 1d75cd4d7be638babe6d4e47bf73ea05fc65d81c)

When we exit from a stitched trace due to the Lua stack overflow
error, we cannot determine the bytecode position properly, and
because of that, we get the incorrect bytecode instruction in
`debug_framepc`. This patch fixes this behavior, so the
`debug_framepc` now returns `NO_BCPOS` for this case.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
===
>
> | Trace 1 start
> | ---- TRACE IR
> | ....          SNAP   #0   [ ---- ]
> | 0001      fun SLOAD  [L ] lit: #0   lit: R
> | 0002      tab FLOAD  [L ] ref: 0001 lit: func.env
> | 0003      int FLOAD  [L ] ref: 0002 lit: tab.hmask
> | 0004   >  int EQ     [C ] ref: 0003 ref: integer 63
> | 0005      p32 FLOAD  [L ] ref: 0002 lit: tab.node
> | 0006   >  p32 HREFK  [R ] ref: 0005 ref: string "math" @ 0x400061e8 KSLOT: @54
> | 0007   >  tab HLOAD  [L ] ref: 0006
> | 0008      int FLOAD  [L ] ref: 0007 lit: tab.hmask
> | 0009   >  int EQ     [C ] ref: 0008 ref: integer 31
> | 0010      p32 FLOAD  [L ] ref: 0007 lit: tab.node
> | 0011   >  p32 HREFK  [R ] ref: 0010 ref: string "fmod" @ 0x40006c88 KSLOT: @18
> | 0012   >  fun HLOAD  [L ] ref: 0011
> | 0013   >  fun EQ     [C ] ref: 0012 ref: fast function #58
> | ....          SNAP   #1   [ ---- {trace 0x1 @ 0x400271d8} contpc ftsz | {number 42} {number 42} ]
> | 0014          NOP    [N ]
> |
> | Trace 2 start
> | ---- TRACE IR
> | ....          SNAP   #0   [ ---- ]
> | 0001      fun SLOAD  [L ] lit: #0   lit: R
> | 0002   >  fun EQ     [C ] ref: 0001 ref: Lua function @ 0x400157f0, 1 upvalues, "@../master/test/tarantool-tests/lj-913-so-stitched.test.lua":11
> | ....          SNAP   #1   [ ftsz | ftsz | ]
> | 0003          NOP    [N ]
>
> > that, we get the incorrect bytecode instruction in
> > `debug_framepc`. This patch fixes this behavior, so the
> > `debug_framepc` now returns `NO_BCPOS` for this case.
> >
> > Maxim Kokryashkin:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9145
> > ---
> > Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-913-avoid-assertion-stkov-from-stitched-trace
> > PR: https://github.com/tarantool/tarantool/pull/9484
> > Issues: https://github.com/tarantool/tarantool/issues/9145
> > https://github.com/LuaJIT/LuaJIT/issues/913
> >
> >  src/lj_debug.c                                |  9 +++++---
> >  ...-913-stackoverflow-stitched-trace.test.lua | 23 +++++++++++++++++++
> >  2 files changed, 29 insertions(+), 3 deletions(-)
> >  create mode 100644 test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> >
> > diff --git a/src/lj_debug.c b/src/lj_debug.c
> > index 46c442c6..107f464c 100644
> > --- a/src/lj_debug.c
> > +++ b/src/lj_debug.c
>
> <snipped>
>
> > diff --git a/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> > new file mode 100644
> > index 00000000..3c12f0d9
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> > @@ -0,0 +1,23 @@
> > +local tap = require('tap')
> > +-- Test to demonstrate the incorrect LuaJIT behavior when exiting
> > +-- from a snapshot for stitched trace.
> > +local test = tap.test('lj-913-stackoverflow-stitched-trace'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(3)
> > +
> > +-- Recursion to cause stack overflow.
> > +local function callee()
> > +  -- `math.fmod()` is NYI, so trace will be stitched here.
> > +  local _ = math.fmod(42, 42)
>
> We don't need this `_` variable here. Just using `math.fmod()` is
> enough.
Fixed, thanks!
Branch is force-pushed.
Here is the diff:
===
diff --git a/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
index 3c12f0d9..434901dc 100644
--- a/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
+++ b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
@@ -10,7 +10,7 @@ test:plan(3)
 -- Recursion to cause stack overflow.
 local function callee()
   -- `math.fmod()` is NYI, so trace will be stitched here.
-  local _ = math.fmod(42, 42)
+  math.fmod(42, 42)
   callee()
 end

===
>
> > +  callee()
> > +end
> > +
> > +local st, err = pcall(callee)
> > +
> > +test:ok(true, 'assertion is not triggered')
> > +test:ok(not st, 'error happened')
> > +test:like(err, 'stack overflow', 'stack overflow happened')
> > +
> > +test:done(true)
> > --
> > 2.43.0
> >
>
> --
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Avoid assertion in case of stack overflow from stitched trace.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Avoid assertion in case of stack overflow from stitched trace.

[Sergey Bronnikov via Tarantool-patches]

Hi, Max!

thanks for the patch! LGTM after addressing comments by Sergey.

Sergey

> won't be as long as the mentioned error is raised. Because of
> that, we get the incorrect bytecode instruction in
> `debug_framepc`. This patch fixes this behavior, so the
> `debug_framepc` now returns `NO_BCPOS` for this case.
>
> Maxim Kokryashkin:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9145
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-913-avoid-assertion-stkov-from-stitched-trace
> PR: https://github.com/tarantool/tarantool/pull/9484
> Issues: https://github.com/tarantool/tarantool/issues/9145
> https://github.com/LuaJIT/LuaJIT/issues/913
>
>   src/lj_debug.c                                |  9 +++++---
>   ...-913-stackoverflow-stitched-trace.test.lua | 23 +++++++++++++++++++
>   2 files changed, 29 insertions(+), 3 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
>
> diff --git a/src/lj_debug.c b/src/lj_debug.c
> index 46c442c6..107f464c 100644
> --- a/src/lj_debug.c
> +++ b/src/lj_debug.c
> @@ -101,9 +101,12 @@ static BCPos debug_framepc(lua_State *L, GCfunc *fn, cTValue *nextframe)
>     pos = proto_bcpos(pt, ins) - 1;
>   #if LJ_HASJIT
>     if (pos > pt->sizebc) {  /* Undo the effects of lj_trace_exit for JLOOP. */
> -    GCtrace *T = (GCtrace *)((char *)(ins-1) - offsetof(GCtrace, startins));
> -    lj_assertL(bc_isret(bc_op(ins[-1])), "return bytecode expected");
> -    pos = proto_bcpos(pt, mref(T->startpc, const BCIns));
> +    if (bc_isret(bc_op(ins[-1]))) {
> +      GCtrace *T = (GCtrace *)((char *)(ins-1) - offsetof(GCtrace, startins));
> +      pos = proto_bcpos(pt, mref(T->startpc, const BCIns));
> +    } else {
> +      pos = NO_BCPOS;  /* Punt in case of stack overflow for stitched trace. */
> +    }
>     }
>   #endif
>     return pos;
> diff --git a/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> new file mode 100644
> index 00000000..3c12f0d9
> --- /dev/null
> +++ b/test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> @@ -0,0 +1,23 @@
> +local tap = require('tap')
> +-- Test to demonstrate the incorrect LuaJIT behavior when exiting
> +-- from a snapshot for stitched trace.
> +local test = tap.test('lj-913-stackoverflow-stitched-trace'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(3)
> +
> +-- Recursion to cause stack overflow.
> +local function callee()
> +  -- `math.fmod()` is NYI, so trace will be stitched here.
> +  local _ = math.fmod(42, 42)
> +  callee()
> +end
> +
> +local st, err = pcall(callee)
> +
> +test:ok(true, 'assertion is not triggered')
> +test:ok(not st, 'error happened')
> +test:like(err, 'stack overflow', 'stack overflow happened')
> +
> +test:done(true)
> --
> 2.43.0
>

Re: [Tarantool-patches] [PATCH luajit] Avoid assertion in case of stack overflow from stitched trace.

[Igor Munkin via Tarantool-patches]

Max,

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

On 14.12.23, Maxim Kokryashkin via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Bronnikov. Fixed by Sergey Kaplun.
> 
> (cherry-picked from commit 1d75cd4d7be638babe6d4e47bf73ea05fc65d81c)
> 
> When we exit from a stitched trace due to the Lua stack overflow
> error, the Lua and JIT stacks are not synchronized, and they
> won't be as long as the mentioned error is raised. Because of
> that, we get the incorrect bytecode instruction in
> `debug_framepc`. This patch fixes this behavior, so the
> `debug_framepc` now returns `NO_BCPOS` for this case.
> 
> Maxim Kokryashkin:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#9145
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-913-avoid-assertion-stkov-from-stitched-trace
> PR: https://github.com/tarantool/tarantool/pull/9484
> Issues: https://github.com/tarantool/tarantool/issues/9145
> https://github.com/LuaJIT/LuaJIT/issues/913
> 
>  src/lj_debug.c                                |  9 +++++---
>  ...-913-stackoverflow-stitched-trace.test.lua | 23 +++++++++++++++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-913-stackoverflow-stitched-trace.test.lua
> 

<snipped>

> --
> 2.43.0
> 

-- 
Best regards,
IM