Re: [Tarantool-patches] [PATCH luajit 2/2] Fix frame for more types of on-trace error messages.

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

Hi, Maxim!
Thanks for the patch!
LGTM, with a few nits below.

On 05.09.23, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Thanks to Maxim Kokryashkin.
> 
> (cherry-picked from commit d5bbf9cdb4c5eddc404a90bd44f077cfb3a57a90)
> 
> This patch fixes the same issue with frame, as the previous

Typo: s/frame,/the frame/

> one, but now for the table overflow error in the `err_msgv`
> function. The test for the problem uses the table of GC
> finalizers, although they are not required to reproduce the
> issue. They only used to make the test as simple as possible.

Typo: s/They only used/They are only used/

Minor: Feel free to add the similar comment to the test itself.

> 
> Resolves tarantool/tarantool#562
> Part of tarantool/tarantool#8825
> ---
>  src/lj_err.c                                  |  4 +++
>  .../lj-1034-tabov-error-frame.test.lua        | 27 +++++++++++++++++++
>  2 files changed, 31 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> 
> diff --git a/src/lj_err.c b/src/lj_err.c
> index 763746e6..46fb81ee 100644
> --- a/src/lj_err.c
> +++ b/src/lj_err.c

<snipped>

> diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> new file mode 100644
> index 00000000..b7520d92
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> @@ -0,0 +1,27 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Test requires GC64 mode enabled'] = not ffi.abi('gc64'),
> +  ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
> +})
> +
> +test:plan(2)
> +

Side note: The test requires ~6Gb of memory to see the error.
Feel free to add the corresponding comment or ignore.

> +-- luacheck: no unused
> +local anchor = {}
> +local function on_gc(t) end
> +
> +local function test_finalizers()
> +    local i = 1
> +    while true do
> +        anchor[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
> +        i = i + 1
> +    end
> +end

Minor: Please use 2 spaces for indentation instead.

> +
> +local st, err = pcall(test_finalizers)
> +st, err = pcall(test_finalizers)
> +test:ok(st == false, 'error handled successfully')
> +test:like(err, '^.+table overflow', 'error is table overflow')
> +test:done(true)
> -- 
> 2.41.0
> 

-- 
Best regards,
Sergey Kaplun