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 = ''

<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>