Hi, Sergey thanks for the fixes and answers! See my answers below. On 13.06.2024 13:25, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the review! > Please consider my answers below. > > On 06.06.24, Sergey Bronnikov wrote: > --- /dev/null > +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua > @@ -0,0 +1,46 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate unbalanced Lua stack after instruction > +-- recording due to throwing an error at recording of a stitched > +-- function. > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1166. > + > +local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({ >> should a name in tap.test match to test file name? >> >> now it is not. > My mistake during copipasting. Fixed. Thanks! >>> + ['Test requires JIT enabled'] = not jit.status(), >>> + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', >>> +}) >>> + >>> +test:plan(1) >>> + >>> +local mockalloc = require('mockalloc') >>> + >>> +local function create_chunk(n_slots) >> I would add a comment like this: >> >> >> --- 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 >> @@ -14,6 +14,18 @@ test:plan(1) >> >>  local mockalloc = require('mockalloc') >> >> +-- Generate a Lua chunk like below: >> +-- local s1 >> +-- local s2 >> +-- ... >> +-- local sN >> +-- for i = 1, 2 do >> +--   s1 = i + 1 >> +--   s2 = i + 2 >> +--   ... >> +--   sN = i + N >> +--   math.modf(1) >> +-- end >>  local function create_chunk(n_slots) >>    local chunk = '' >>    for i = 1, n_slots do > Added, see the iterative patch below. Thanks! > >> >>> + local chunk = '' > > >>> + >>> +local mockalloc = require('mockalloc') >>> + >>> +local function create_chunk(n_conds) >> the same as above: please add a comment with an example of generated Lua >> chunk > Added, see the iterative patch below. > Thanks! > diff --git a/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua > new file mode 100644 > index 00000000..f2453bbe > --- /dev/null > +++ b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua >> this test is not failed after reverting patch > Do you build luajit with -DLUAJIT_ENABLE_TABLE_BUMP=ON? > > > My bad. With enabled option tests failed: The following tests FAILED:         127 - test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua (SEGFAULT)         128 - test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua (SEGFAULT)         129 - test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua (SEGFAULT) Errors while running CTest >>> diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c >>> new file mode 100644 >>> index 00000000..d6d3492e >>> --- /dev/null >>> +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c >>> @@ -0,0 +1,51 @@ > > >>> + >>> +/* Function to be used instead of the default allocator. */ >>> +static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize) >>> +{ >>> + assert(old_allocf != NULL); >>> + /* >>> + * Check the specific reallocation related to the IR >>> + * buffer or the snapshot buffer. >>> + */ >>> + if (osize * 2 == nsize) >>> + return NULL; >>> + return old_allocf(ud, ptr, osize, nsize); >>> +} >>> + >>> +static int mock(lua_State *L) >> >> It is actually not a test mock. >> >> According to definition [1] test mock imitate a behavior of a real object. >> >> Your memory allocator behaves as a real allocator, but in some cases it >> will return >> >> a NULL instead of memory address. What if we rename "mock" to "allocator >> with fault injection"? > I renamed `mock_allocf` to `allocf_with_injection()` to avoid > confusing. > > =================================================================== > 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 cf3ab0f5..eb229a5c 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 > @@ -5,7 +5,7 @@ local tap = require('tap') > -- function. > -- See also:https://github.com/LuaJIT/LuaJIT/issues/1166. > > -local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({ > +local test = tap.test('lj-1166-error-stitch-oom-ir-buff'):skipcond({ > ['Test requires JIT enabled'] = not jit.status(), > ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', > }) > @@ -22,6 +22,16 @@ jit.flush() > > test:plan(2) > > +-- Generate the following Lua chunk: > +-- local s1 > +-- ... > +-- local sN > +-- for i = 1, 2 do > +--   s1 = i + 1 > +--   ... > +--   sN = i + N > +--   math.modf(1) > +-- end > local function create_chunk(n_slots) > local chunk = '' > for i = 1, n_slots do > 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 8bbdd96b..8ae26386 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 > @@ -20,6 +20,13 @@ jit.flush() > > test:plan(2) > > +-- Generate the following Lua chunk: > +-- for i = 1, 2 do > +--   if i < 1 then end > +--   ... > +--   if i < N then end > +--   math.modf(1) > +-- end > local function create_chunk(n_conds) > local chunk = '' > chunk = chunk .. 'for i = 1, 2 do\n' > diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c > index d6d3492e..19d32f8b 100644 > --- a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c > +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c > @@ -8,7 +8,8 @@ static lua_Alloc old_allocf = NULL; > static void *old_alloc_state = NULL; > > /* Function to be used instead of the default allocator. */ > -static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize) > +static void *allocf_with_injection(void *ud, void *ptr, size_t osize, > + size_t nsize) > { > assert(old_allocf != NULL); > /* > @@ -24,14 +25,14 @@ static int mock(lua_State *L) > { > assert(old_allocf == NULL); > old_allocf = lua_getallocf(L, &old_alloc_state); > - lua_setallocf(L, mock_allocf, old_alloc_state); > + lua_setallocf(L, allocf_with_injection, old_alloc_state); > return 0; > } > > static int unmock(lua_State *L) > { > assert(old_allocf != NULL); > - assert(old_allocf != mock_allocf); > + assert(old_allocf != allocf_with_injection); > lua_setallocf(L, old_allocf, old_alloc_state); > old_allocf = NULL; > old_alloc_state = NULL; > =================================================================== > Filename and other variables still use prefix "mock": [0] ~/sources/MRG/tarantool/third_party/luajit$ grep -R mock test test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua:local mockalloc = require('mockalloc') test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua:mockalloc.mock() test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua:mockalloc.unmock() test/tarantool-tests/CMakeLists.txt:# Some tests use `LD_PRELOAD` to mock system calls (like test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua:local mockalloc = require('mockalloc') test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua:mockalloc.mock() test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua:mockalloc.unmock() test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:static int mock(lua_State *L) test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:static int unmock(lua_State *L) test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:static const struct luaL_Reg mockalloc[] = { test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:  {"mock", mock}, test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:  {"unmock", unmock}, test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:LUA_API int luaopen_mockalloc(lua_State *L) test/tarantool-tests/lj-1166-error-stitch/mockalloc.c: luaL_register(L, "mockalloc", mockalloc); test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt:BuildTestCLib(mockalloc mockalloc.c) have you left it intentionally? >> >> 1.https://www.martinfowler.com/articles/mocksArentStubs.html >> >>> +{ > > >>> diff --git a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua >>> index d750b721..6e8f70c2 100644 >>> --- a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua >>> +++ b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua > >