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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.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)