Re: [Tarantool-patches] [PATCH luajit v3 1/3] Improve error reporting on stack overflow.

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

[-- Attachment #1: Type: text/plain, Size: 8797 bytes --]

Hi, Max


the problem is reproduced, thanks!

LGTM


On 12/6/23 18:07, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the review!
> The test case indeed fails only for the GC64 mode before the patch.
> I've tried to make it fail for non-GC64 mode, but the same issue arised —
> an arbitrary amount of slots needed, and that amount needs to be modified
> each time there is a minor change in the Lua stack behavior. With that in
> mind, I’ve decided to leave it as it is. Added a comment and modified the
> commit message. The new commit message:
> ===
> Improve error reporting on stack overflow.
> Thanks to Nicolas Lebedenco.
> (cherry-picked from commit 8135de2a0204e6acd92b231131c4a1e0be03ac1c)
> The `lj_state_growstack` doesn't account for a potential error
> handler invocation by `xpcall`, which may lead to the second
> error while handling a stack overflow, resulting in a misleading
> error "error in error handling", while the real issue is a stack
> overflow. This patch addresses this issue by fixing the condition
> at which stack overflow errors are thrown. Now it's thrown if the
> stack size is at least at the limit, instead of when it is over
> the limit.
> The test case is very fragile, and the issue is hard to reproduce
> in a robust way. The provided reproducer failed before the patch
> for GC64 mode only. Non-GC64 mode requires an arbitrary number
> of stack slots to be filled, which is even more fragile.
> This commit also disables the second test from
> `lj-603-err-snap-restore`, since after this patch and the two
> follow-ups for it, there is no such amount of stack slots with
> which the test works the way it should.
> Lastly, this patch adds an alternative to `luacmd` to the
> `utils.exec` module, which is called `luabin`. That function is
> similar to the `luacmd`, with the only difference of returning
> only the executable itself without any additional CLI options
> passed.
> Maxim Kokryashkin:
> * added the description and the test for the problem
> Part of tarantool/tarantool#9145
> ===
> And the diff for the comment:
> ===
> diff --git 
> a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua 
> b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> index 45a888f4..e5f9d9a4 100644
> --- a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> @@ -3,6 +3,12 @@ local tap = require('tap')
>  local test = tap.test('lj-962-stack-overflow-report')
>  test:plan(1)
> +-- XXX: The test case is very fragile, and the issue is hard to
> +-- reproduce in a robust way. The provided reproducer failed
> +-- before the patch for GC64 mode only. Non-GC64 mode requires
> +-- an arbitrary number of stack slots to be filled, which is
> +-- even more fragile.
> +
>  local LUABIN = require('utils').exec.luabin(arg)
>  local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua')
>  local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT, 'r'):read('*all')
> ===
> --
> Best regards,
> Maxim Kokryashkin
>
>     Пятница, 24 ноября 2023, 15:22 +03:00 от Sergey Bronnikov
>     <sergeyb@tarantool.org>:
>     Hello, Max
>
>     thanks for the patch!
>
>     proposed test is passed after reverting a fix
>
>
>     On 11/22/23 17:35, Maksim Kokryashkin wrote:
>     > From: Mike Pall <mike>
>     >
>     > Thanks to Nicolas Lebedenco.
>     >
>     > (cherry-picked from commit 8135de2a0204e6acd92b231131c4a1e0be03ac1c)
>     >
>     > The `lj_state_growstack` doesn't account for a potential error
>     > handler invocation by `xpcall`, which may lead to the second
>     > error while handling a stack overflow, resulting in a misleading
>     > error "error in error handling", while the real issue is a stack
>     > overflow. This patch addresses this issue by fixing the condition
>     > at which stack overflow errors are thrown. Now it's thrown if the
>     > stack size is at least at the limit, instead of when it is over
>     > the limit.
>     >
>     > This commit also disables the second test from
>     > `lj-603-err-snap-restore`, since after this patch and the two
>     > follow-ups for it, there is no such amount of stack slots with
>     > which the test works the way it should.
>     >
>     > Lastly, this patch adds an alternative to `luacmd` to the
>     > `utils.exec` module, which is called `luabin`. That function is
>     > similar to the `luacmd`, with the only difference of returning
>     > only the executable itself without any additional CLI options
>     > passed.
>     >
>     > Maxim Kokryashkin:
>     > * added the description and the test for the problem
>     >
>     > Part of tarantool/tarantool#9145
>     > ---
>     > src/lj_state.c | 2 +-
>     > .../lj-603-err-snap-restore.test.lua | 1 +
>     > .../lj-962-stack-overflow-report.test.lua | 10 ++++++++++
>     > .../lj-962-stack-overflow-report/script.lua | 3 +++
>     > test/tarantool-tests/utils/exec.lua | 15 ++++++++++++---
>     > 5 files changed, 27 insertions(+), 4 deletions(-)
>     > create mode 100644
>     test/tarantool-tests/lj-962-stack-overflow-report.test.lua
>     > create mode 100644
>     test/tarantool-tests/lj-962-stack-overflow-report/script.lua
>     >
>     > diff --git a/src/lj_state.c b/src/lj_state.c
>     > index 684336d5..76153bad 100644
>     > --- a/src/lj_state.c
>     > +++ b/src/lj_state.c
>     > @@ -132,7 +132,7 @@ void LJ_FASTCALL
>     lj_state_growstack(lua_State *L, MSize need)
>     > n = LJ_STACK_MAX;
>     > }
>     > resizestack(L, n);
>     > - if (L->stacksize > LJ_STACK_MAXEX)
>     > + if (L->stacksize >= LJ_STACK_MAXEX)
>     > lj_err_msg(L, LJ_ERR_STKOV);
>     > }
>     >
>     > diff --git
>     a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>     b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>     > index 96ebf92c..f5c8474f 100644
>     > --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>     > +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>     > @@ -36,6 +36,7 @@ local function do_test()
>     > -- Tarantool at start, so just skip test for it.
>     > -- luacheck: no global
>     > ['Disable test for Tarantool'] = _TARANTOOL,
>     > + ['Stack overflow is now handled differently'] = true,
>     > })
>     >
>     > test:ok(not handler_is_called)
>     > diff --git
>     a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
>     b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
>     > new file mode 100644
>     > index 00000000..45a888f4
>     > --- /dev/null
>     > +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
>     > @@ -0,0 +1,10 @@
>     > +local tap = require('tap')
>     > +-- The test reproduces the problem only for GC64 mode with
>     enabled JIT.
>     > +local test = tap.test('lj-962-stack-overflow-report')
>     > +test:plan(1)
>     > +
>     > +local LUABIN = require('utils').exec.luabin(arg)
>     > +local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua')
>     > +local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT,
>     'r'):read('*all')
>     > +test:like(output, 'stack overflow', 'stack overflow reported
>     correctly')
>     > +test:done(true)
>     > diff --git
>     a/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
>     b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
>     > new file mode 100644
>     > index 00000000..31c5ca33
>     > --- /dev/null
>     > +++ b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
>     > @@ -0,0 +1,3 @@
>     > +-- XXX: Function `f` is global to avoid using an additional
>     stack slot.
>     > +-- luacheck: no global
>     > +f = function() f() end; f()
>     > diff --git a/test/tarantool-tests/utils/exec.lua
>     b/test/tarantool-tests/utils/exec.lua
>     > index a56ca2dc..48a08ba5 100644
>     > --- a/test/tarantool-tests/utils/exec.lua
>     > +++ b/test/tarantool-tests/utils/exec.lua
>     > @@ -1,14 +1,23 @@
>     > local M = {}
>     >
>     > -function M.luacmd(args)
>     > +local function executable_idx(args)
>     > -- arg[-1] is guaranteed to be not nil.
>     > local idx = -2
>     > while args[idx] do
>     > assert(type(args[idx]) == 'string', 'Command part have to be a
>     string')
>     > idx = idx - 1
>     > end
>     > - -- return the full command with flags.
>     > - return table.concat(args, ' ', idx + 1, -1)
>     > + return idx + 1
>     > +end
>     > +
>     > +function M.luabin(args)
>     > + -- Return only the executable.
>     > + return args[executable_idx(args)]
>     > +end
>     > +
>     > +function M.luacmd(args)
>     > + -- Return the full command with flags.
>     > + return table.concat(args, ' ', executable_idx(args), -1)
>     > end
>     >
>     > local function makeenv(tabenv)
>

[-- Attachment #2: Type: text/html, Size: 15101 bytes --]