From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching. Date: Thu, 13 Jun 2024 17:52:31 +0300 [thread overview] Message-ID: <7bd8ffff-02c9-4c91-b12a-047a6ba707d4@tarantool.org> (raw) In-Reply-To: <ZmrI_4Ywx1wFNkpG@root> [-- Attachment #1: Type: text/plain, Size: 8913 bytes --] 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: <snipped> > --- /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 = '' > <snipped> > >>> + >>> +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? > > <snipped> > 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 @@ > <snipped> > >>> + >>> +/* 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 >> >>> +{ > <snipped> > >>> 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 > <snipped> > [-- Attachment #2: Type: text/html, Size: 12228 bytes --]
next prev parent reply other threads:[~2024-06-13 14:52 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-04-22 8:49 [Tarantool-patches] [PATCH luajit v1 0/5] Handle all errors during stitching Sergey Kaplun via Tarantool-patches 2024-04-22 8:49 ` [Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP Sergey Kaplun via Tarantool-patches 2024-05-05 12:39 ` Maxim Kokryashkin via Tarantool-patches 2024-05-13 11:47 ` Sergey Kaplun via Tarantool-patches 2024-06-06 10:53 ` Sergey Bronnikov via Tarantool-patches 2024-06-13 9:51 ` Sergey Kaplun via Tarantool-patches 2024-06-13 14:37 ` Sergey Bronnikov via Tarantool-patches 2024-04-22 8:49 ` [Tarantool-patches] [PATCH luajit v1 2/5] ci: add tablebump flavor for exotic builds Sergey Kaplun via Tarantool-patches 2024-05-05 12:40 ` Maxim Kokryashkin via Tarantool-patches 2024-05-13 11:49 ` Sergey Kaplun via Tarantool-patches 2024-06-06 10:56 ` Sergey Bronnikov via Tarantool-patches 2024-04-22 8:49 ` [Tarantool-patches] [PATCH luajit v1 3/5] test: allow `jit.parse` to return aborted traces Sergey Kaplun via Tarantool-patches 2024-05-05 12:55 ` Maxim Kokryashkin via Tarantool-patches 2024-05-13 11:53 ` Sergey Kaplun via Tarantool-patches 2024-06-06 11:01 ` Sergey Bronnikov via Tarantool-patches 2024-06-13 10:00 ` Sergey Kaplun via Tarantool-patches 2024-06-13 14:38 ` Sergey Bronnikov via Tarantool-patches 2024-04-22 8:49 ` [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching Sergey Kaplun via Tarantool-patches 2024-05-06 8:25 ` Maxim Kokryashkin via Tarantool-patches 2024-06-06 13:03 ` Sergey Bronnikov via Tarantool-patches 2024-06-13 10:25 ` Sergey Kaplun via Tarantool-patches 2024-06-13 14:52 ` Sergey Bronnikov via Tarantool-patches [this message] 2024-04-22 8:49 ` [Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM " Sergey Kaplun via Tarantool-patches 2024-05-06 8:27 ` Maxim Kokryashkin via Tarantool-patches 2024-06-06 14:06 ` Sergey Bronnikov via Tarantool-patches 2024-06-13 10:31 ` Sergey Kaplun via Tarantool-patches 2024-06-13 14:58 ` Sergey Bronnikov via Tarantool-patches 2024-06-16 19:13 ` Sergey Kaplun via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7bd8ffff-02c9-4c91-b12a-047a6ba707d4@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox