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

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

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

On 18.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
> 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.
> 
> 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     | 24 +++++++++++++++++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-962-stack-overflow-report.test.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..bcddff01
> --- /dev/null
> +++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
> @@ -0,0 +1,24 @@
> +local tap = require('tap')
> +local test = tap.test('lj-962-stack-overflow-report')
> +
> +test:plan(2)
> +
> +-- luacheck: no unused
> +local _, _, _, _, _, _, _

I am very nervous, when I see these `_, _, ...` guys again.
I suggest introducing helper to fill up the necessary slots with
correponding comments.

Also, this test passes on the GC64/non-GC64 build on the current master
branch.

> +local error_message
> +local handler_is_called = false
> +local function errfunc(err)
> +  -- luacheck: no unused
> +  local _
> +  error_message = err
> +  handler_is_called = true
> +end
> +
> +local function recursive_f()
> +  recursive_f()
> +end
> +xpcall(recursive_f, errfunc)
> +
> +test:ok(handler_is_called, 'error handler was able to use a stack slot')

Typo: s/was/is/

Minor: Also maybe it's worth being more descriptive "an additional stack
slot".

> +test:like(error_message, '.+stack overflow$', 'stack overflow happened')

Minor: I suggest dropping `.+` at the start of the pattern since it
doesn't matter. NB: the `$` is important to distinguish `STKOV` and
`STKOVM`.

> +test:done(true)
> -- 
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun