Re: [Tarantool-patches] [PATCH luajit 2/2] Fix VM event error handling for finalizers.

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

[-- Attachment #1: Type: text/plain, Size: 2871 bytes --]

Hi, Sergey!

thanks for the patch! LGTM with a couple of comments below

Sergey

On 3/28/26 18:31, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry picked from commit fbb36bb6bfa88716a47c58bcf9ce9f2ef752abac)
>
> After the previous commit, the VM handler for the 'errfin' VM event
> works incorrectly. The error object is taken not from the stack on which
> the VM handler is invoked. Hence, it breaks the non-VM stack and returns
> an incorrect error message.
>
> This patch fixes the issue by copying the object from the corresponding
> Lua stack.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#12134
> ---
>   src/lj_gc.c                                   |  6 +++--
>   .../lj-1445-errfin-errmsg.test.lua            | 27 +++++++++++++++++++
>   2 files changed, 31 insertions(+), 2 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1445-errfin-errmsg.test.lua
>
> diff --git a/src/lj_gc.c b/src/lj_gc.c
> index 83ad9ee9..7e4f0a47 100644
> --- a/src/lj_gc.c
> +++ b/src/lj_gc.c
> @@ -519,10 +519,12 @@ static void gc_call_finalizer(global_State *g, lua_State *L,
>     if (LJ_HASPROFILE && (oldh & HOOK_PROFILE)) lj_dispatch_update(g);
>     g->gc.threshold = oldt;  /* Restore GC threshold. */
>     if (errcode) {
> +    TValue tmp;
> +    copyTV(VL, &tmp, VL->top-1);
> +    VL->top--;
>       lj_vmevent_send(g, ERRFIN,
> -      copyTV(V, V->top++, L->top-1);
> +      copyTV(V, V->top++, &tmp);
>       );
> -    L->top--;
>     }
>   }
>   
> diff --git a/test/tarantool-tests/lj-1445-errfin-errmsg.test.lua b/test/tarantool-tests/lj-1445-errfin-errmsg.test.lua
> new file mode 100644
> index 00000000..def8b79c
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1445-errfin-errmsg.test.lua
> @@ -0,0 +1,27 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1445-errfin-errmsg')
> +

Usually you add a comment with test description and a link to the issue.

But this time a comment is absent. Should we add it?

> +test:plan(2)
> +
> +local EXPECTED_ERR = 'expected err'

s/err/error/

feel free to ignore

> +local function bad_fin()
> +  error(EXPECTED_ERR)
> +end
> +local EXPECTED_LOCATION = debug.getinfo(bad_fin).linedefined + 1
> +
> +local match_err = false
> +local match_line = false
> +local function errfin_handler(errmsg)
> +  match_err =errmsg:match(EXPECTED_ERR)
> +  match_line =errmsg:match(EXPECTED_LOCATION)
> +end
> +
> +jit.attach(errfin_handler, 'errfin')
> +
> +debug.getmetatable(newproxy(true)).__gc = bad_fin

I would define userdata previously for clarity:

local ud = newproxy(true)

and then use it in getmetatable().

feel free to ignore


> +collectgarbage()
> +
> +test:ok(match_err, 'correct error message in errfin handler')
> +test:ok(match_line, 'correct source line in errfin handler')
> +
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4055 bytes --]