Re: [Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM during trace stitching.

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

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 06.06.24, Sergey Bronnikov wrote:
> Sergey,
> 
> 
> thanks for the patch! See my comments below.
> 
> 
> Sergey
> 
> On 22.04.2024 11:49, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun.
> >
> > (cherry picked from commit b8b49bf3954b23e32e34187a6ada00021c26e172)
> >
> > The previous commit doesn't handle the case when the error code is
> > `LUA_ERRMEM`. This patch adds a workaround by using the generic error
> > message.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9924
> > ---
> >   src/lj_ffrecord.c                             |  2 +
> >   .../lj-1166-error-stitch-oom-ir-buff.test.lua | 41 ++++++++++++++++++-
> >   ...j-1166-error-stitch-oom-snap-buff.test.lua | 37 +++++++++++++++--
> >   3 files changed, 75 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> > index ff14e9e4..d5fc081e 100644
> > --- a/src/lj_ffrecord.c
> > +++ b/src/lj_ffrecord.c
> > @@ -150,6 +150,8 @@ static void recff_stitch(jit_State *J)
> >     if (errcode) {
> >       if (errcode == LUA_ERRRUN)
> >         copyTV(L, L->top-1, L->top + (1 + LJ_FR2));
> > +    else
> > +      setintV(L->top-1, (int32_t)LJ_TRERR_RECERR);
> >       lj_err_throw(L, errcode);  /* Propagate errors. */
> >     }
> >   }
> > diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> > index e3a5397d..cf3ab0f5 100644
> > --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> > +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua

<snipped>

> > +test:skipcond({
> > +  -- luacheck: no global
> 
> I made a patch that remove inline suppressions [1].
> 
> I propose to merge it and remove inline suppressions in your patch 
> series too.

Ok, I'll remove these supressions after merging your patch and rebasing
to the master.

<snipped>

> >   
> > +-- We only need the abort reason in the test.
> > +jparse.start('t')
> Same comment as in previous mail - let's add a comment regarding 't'.

See my answers here [1].

> > +
> >   -- XXX: Update hotcounts to avoid hash collisions.
> >   jit.opt.start('hotloop=1')
> > -
> >   jit.on()
> >   
> >   mockalloc.mock()
> > @@ -49,6 +58,28 @@ tracef()
> >   
> >   mockalloc.unmock()
> Same comment as in previous mail - let's avoid name 'mock' here.

But we actually mock the allocator here, don't we?
Thus, I renamed the new allocated function to avoid confusion.
If you insist, please suggest the correct name instead `mock` |
`unmock`.

> >   

<snipped>

> >   test:done(true)

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-June/029218.html

-- 
Best regards,
Sergey Kaplun