Tarantool development patches archive
 help / color / mirror / Atom feed
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 --]

  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