Re: [Tarantool-patches] [PATCH luajit] Throw any errors before stack changes in trace stitching.

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

Hello, Max,

thanks for the patch! see my comments

Sergey

On 2/16/24 14:11, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> Thanks to doujiang24.
>
> (cherry-picked from commit 3f9389edc6cdf3f78a6896d550c236860aed62b2)
>
> The Lua stack is changed in the `lj_record_stop`, so if the trace
> is aborted, for example, when the `maxsnap` is reached, and an
> error is thrown from there, the execution continues with an
> unbalanced stack.
>
> Since the only error that is thrown from the `lj_record_stop`
> with modified stack is `LJ_TRERR_SNAPOV`, this patch adds a

As I see lj_record_stop [1] throws only LJ_TRERR_RETRY or

I got you wrong.


1. 
https://github.com/LuaJIT/LuaJIT/blob/0d313b243194a0b8d2399d8b549ca5a0ff234db5/src/lj_record.c#L299

> check for snapshot overflow to the `recff_stitch`.
>
> Maxim Kokryashkin:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-pr-720-errors-before-stitch
> PR: https://github.com/tarantool/tarantool/pull/9703
> Issues:
> https://github.com/tarantool/tarantool/issues/9595
> https://github.com/LuaJIT/LuaJIT/pull/720
>
>   src/lj_ffrecord.c                             |  4 ++++
>   .../lj-pr-720-errors-before-stitch.test.lua   | 20 +++++++++++++++++++
>   2 files changed, 24 insertions(+)
>   create mode 100644 test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua
>
> diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> index 1b2ddb72..e3ed80fb 100644
> --- a/src/lj_ffrecord.c
> +++ b/src/lj_ffrecord.c
> @@ -107,6 +107,10 @@ static void recff_stitch(jit_State *J)
>     const BCIns *pc = frame_pc(base-1);
>     TValue *pframe = frame_prevl(base-1);
>
> +  /* Check for this now. Throwing in lj_record_stop messes up the stack. */
> +  if (J->cur.nsnap >= (MSize)J->param[JIT_P_maxsnap])
> +    lj_trace_err(J, LJ_TRERR_SNAPOV);
> +
>     /* Move func + args up in Lua stack and insert continuation. */
>     memmove(&base[1], &base[-1-LJ_FR2], sizeof(TValue)*nslot);
>     setframe_ftsz(nframe, ((char *)nframe - (char *)pframe) + FRAME_CONT);
> diff --git a/test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua
> new file mode 100644
> index 00000000..53b7b5ea
> --- /dev/null
> +++ b/test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua

You could omit "-pr" in the name, anyway

https://github.com/LuaJIT/LuaJIT/issues/720 redirects to 
https://github.com/LuaJIT/LuaJIT/pull/720.

> @@ -0,0 +1,20 @@
> +local tap = require('tap')
> +local test = tap.test('lj-pr-720-errors-before-stitch'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +test:plan(1)
> +
> +-- `math.modf` recording is NYI.
> +local modf = require('math').modf
> +jit.opt.start('hotloop=1', 'maxsnap=1')
> +
> +-- The loop has only two iterations: the first to detect its
> +-- hotness and the second to record it. The snapshot limit is
> +-- set to one and is certainly reached.
> +for _ = 1, 2 do
> +  -- Forcify stitch.
> +  modf(1.2)
> +end
> +
> +test:ok(true, 'stack is balanced')
> +test:done(true)
> --
> 2.43.0
>