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

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

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

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
> @@ -10,10 +10,18 @@ local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
>     ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>   })
>   
> -test:plan(1)
> -
> +local jparse = require('utils').jit.parse
>   local mockalloc = require('mockalloc')
>   
> +local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0)
> +
> +-- XXX: Avoid other traces compilation due to hotcount collisions
> +-- for predictable results.
> +jit.off()
> +jit.flush()
> +
> +test:plan(2)
> +
>   local function create_chunk(n_slots)
>     local chunk = ''
>     for i = 1, n_slots do
> @@ -33,6 +41,10 @@ end
>   -- XXX: amount of slots is empirical.
>   local tracef = assert(loadstring(create_chunk(175)))
>   
> +-- We only need the abort reason in the test.
> +jparse.start('t')
> +
> +jit.on()
>   jit.opt.start('hotloop=1', '-loop', '-fold')
>   
>   mockalloc.mock()
> @@ -41,6 +53,31 @@ tracef()
>   
>   mockalloc.unmock()
>   
> +local _, aborted_traces = jparse.finish()
> +
> +jit.off()
> +
>   test:ok(true, 'stack is balanced')
>   
> +-- Tarantool may compile traces on the startup. These traces
> +-- already exceed the maximum IR amount before the trace in this
> +-- test is compiled. Hence, there is no need to reallocate the IR
> +-- buffer, so the check for the IR size is not triggered.
> +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.


[1]: 
https://lists.tarantool.org/tarantool-patches/88eab16fca9056a057df5506a0af637c8d4a0ffd.1717682341.git.sergeyb@tarantool.org/T/#u

> +  ['Impossible to predict the number of IRs for Tarantool'] = _TARANTOOL,
> +  -- The amount of IR for traces is different for non x86/x64
> +  -- arches and DUALNUM mode.
> +  ['Disabled for non-x86_64 arches'] = jit.arch ~= 'x64' and jit.arch ~= 'x86',
> +  ['Disabled for DUALNUM mode'] = IS_DUALNUM,
> +})
> +
> +assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
> +
> +-- We tried to compile only one trace.
> +local reason = aborted_traces[1][1].abort_reason
> +
> +test:like(reason, 'error thrown or hook called during recording',
> +          'abort reason is correct')
> +
>   test:done(true)
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> index 8d671f8d..8bbdd96b 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> @@ -10,10 +10,16 @@ local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
>     ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>   })
>   
> -test:plan(1)
> -
> +local jparse = require('utils').jit.parse
>   local mockalloc = require('mockalloc')
>   
> +-- XXX: Avoid other traces compilation due to hotcount collisions
> +-- for predictable results.
> +jit.off()
> +jit.flush()
> +
> +test:plan(2)
> +
>   local function create_chunk(n_conds)
>     local chunk = ''
>     chunk = chunk .. 'for i = 1, 2 do\n'
> @@ -27,6 +33,7 @@ local function create_chunk(n_conds)
>     return chunk
>   end
>   
> +jit.on()
>   -- XXX: Need to compile the cycle in the `create_chunk()` to
>   -- preallocate the snapshot buffer.
>   jit.opt.start('hotloop=1', '-loop', '-fold')
> @@ -38,9 +45,11 @@ local tracef = assert(loadstring(create_chunk(6)))
>   jit.off()
>   jit.flush()
>   
> +-- 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'.
> +
>   -- 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.
>   
> +local _, aborted_traces = jparse.finish()
> +
> +jit.off()
> +
>   test:ok(true, 'stack is balanced')
>   
> +-- Tarantool may compile traces on the startup. These traces
> +-- already exceed the maximum snapshot amount before the trace in
> +-- this test is compiled. Hence, there is no need to reallocate
> +-- the snapshot buffer, so the check for the snap size is not
> +-- triggered.
> +test:skipcond({
> +  -- luacheck: no global
> +  ['Impossible to predict the number of snapshots for Tarantool'] = _TARANTOOL,
> +})
> +
> +assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
> +
> +-- We tried to compile only one trace.
> +local reason = aborted_traces[1][1].abort_reason
> +
> +test:like(reason, 'error thrown or hook called during recording',
> +          'abort reason is correct')
> +
>   test:done(true)

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