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 > : > 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 > > > > 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) >