Re: [Tarantool-patches] [PATCH luajit 1/2] LJ_FR2: Fix stack checks in vararg calls.

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

Hi, Sergey!
Thanks for the fixes!
Please consider my comments below.
Also, please send the next version via v2 series to simplify the
review.

On 23.09.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review! Please see my comments below.
> 
> Sergey
> 
> On 9/1/25 16:07, Sergey Kaplun via Tarantool-patches wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> >
> > On 27.08.25, Sergey Bronnikov wrote:

<snipped>

> >> Sergey Bronnikov:
> >> * added the description and the test for the problem
> >>
> >> Part of tarantool/tarantool#11691
> >> ---
> >>   src/lj_def.h                                  |  2 +-
> >>   src/lj_dispatch.c                             |  2 +-
> >>   src/vm_arm64.dasc                             |  1 +
> >>   src/vm_mips64.dasc                            |  1 +
> >>   ...048-fix-stack-checks-vararg-calls.test.lua | 56 +++++++++++++++++++
> >>   5 files changed, 60 insertions(+), 2 deletions(-)
> >>   create mode 100644 test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
> >>
> >> diff --git a/src/lj_def.h b/src/lj_def.h
> > <snipped>
> >
> >> diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
> >> index a44a5adf..431cb3c2 100644
> >> --- a/src/lj_dispatch.c
> >> +++ b/src/lj_dispatch.c
> >> @@ -453,7 +453,7 @@ static int call_init(lua_State *L, GCfunc *fn)
> >>       int numparams = pt->numparams;
> >>       int gotparams = (int)(L->top - L->base);
> >>       int need = pt->framesize;
> >> -    if ((pt->flags & PROTO_VARARG)) need += 1+gotparams;
> >> +    if ((pt->flags & PROTO_VARARG)) need += 1+LJ_FR2+gotparams;
> > I can't see the test related to this change. Not `prober_1()` nor
> > `prober_2()` lead to the assertion failure for x86_64 or aarch64 without
> > it.
> 
> Please check again. Both testcases trigger segfault on AArch64 (odroid).

Double checked:
| root@odroid:/home/skaplun/lj-1048-review# git diff
| diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
| index 431cb3c2..a44a5adf 100644
| --- a/src/lj_dispatch.c
| +++ b/src/lj_dispatch.c
| @@ -453,7 +453,7 @@ static int call_init(lua_State *L, GCfunc *fn)
|      int numparams = pt->numparams;
|      int gotparams = (int)(L->top - L->base);
|      int need = pt->framesize;
| -    if ((pt->flags & PROTO_VARARG)) need += 1+LJ_FR2+gotparams;
| +    if ((pt->flags & PROTO_VARARG)) need += 1+gotparams;
|      lj_state_checkstack(L, (MSize)need);
|      numparams -= gotparams;
|      return numparams >= 0 ? numparams : 0;
| Test project /home/skaplun/lj-1048-review
|     Start 118: test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua
| 1/1 Test #118: test/tarantool-tests/lj-1048-fix-stack-checks-vararg-calls.test.lua ...   Passed    3.38 sec
|
| 100% tests passed, 0 tests failed out of 1
|
| Label Time Summary:
| tarantool-tests    =   3.38 sec*proc (1 test)
|
| Total Test time (real) =   3.42 sec

<snipped>

> >> +-- patch.
> >> +local function prober_1(...) -- luacheck: no unused
> >> +  pcall(pcall, pcall, pcall, pcall, pcall, pcall, pcall, pcall, pairs, {})
> >> +end
> > Why do we want to use probber_1 here? Why is this different from the
> > second example? Only because of the metamethods?

Still need an explanation.

> >
> > If we want to keep it, please describe why we need at least 9 pcall-s.
> As I got right, exactly this number of pcall's is needed to trigger a 
> stack overflow.

Yes, but why 9 is minimum number of pcall's when the issue is reproduced?

<snipped>

-- 
Best regards,
Sergey Kaplun