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

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

Hi, Maxim!
Thanks for the fixes!
LGTM, after fixing a few nits below.

On 22.11.23, 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

Minor: s/xpcall/`xpcall`/
Feel free to ignore.

> 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         |  9 +++++++++
>  .../lj-962-stack-overflow-report/script.lua       |  2 ++
>  test/tarantool-tests/utils/exec.lua               | 15 ++++++++++++---
>  5 files changed, 25 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

<snipped>

> 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

<snipped>

> 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..26a90f8d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> @@ -0,0 +1,9 @@
> +local tap = require('tap')

Minor: Please add a comment that the test reproduces the problem only
for GC64 mode (and 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..52734f2c
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report/script.lua
> @@ -0,0 +1,2 @@
> +-- luacheck: no global

Nit: It's better to add a comment about avoiding an additional stack
slot usage (local).

> +f = function() f() end; f()
> diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua
> index a56ca2dc..7ed0a9d8 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 cmd_start_idx(args)

Nit: I suggest naming this function like `executable_idx()` or something
similar.

>    -- 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[cmd_start_idx(args)]
> +end
> +
> +function M.luacmd(args)
> +  -- Return the full command with flags.
> +  return table.concat(args, ' ', cmd_start_idx(args), -1)
>  end
>  
>  local function makeenv(tabenv)
> -- 
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun