Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching.

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

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 06.06.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> 
> thanks for the patch! Please see my comments.
> 
> On 22.04.2024 11:49, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun and Peter Cawley.
> >
> > (cherry picked from commit d06beb0480c5d1eb53b3343e78063950275aa281)
> >
> > This commit is a follow-up for the commit
> > 1b8216023d5a79814389f1c1affef27c15d9de27 ("Throw any errors before stack
> > changes in trace stitching."). The patch prepends failures for the
> > specific error to be thrown. Nevertheless, the error may be thrown due
> > to retrying trace recording in the case when table bump optimization
> > is enabled or when OOM is observed during reallocation of the snapshot
> > or IR buffers.
> >
> > This patch adds the corresponding protected frame and rethrows the error
> > after a fixup of the stack.
> >
> > This patch also tests the correctness of copying the error message to
> > the top of the stack to get a valid "abort" reason in the `jit.dump`
> > utility.
> >
> > Also, this patch fixes a non-ASCII space character in the comment for
> > <lj-720-errors-before-stitch.test.lua>.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9924
> > ---
> >   src/lj_ffrecord.c                             | 21 ++++++--
> >   test/tarantool-tests/CMakeLists.txt           |  1 +
> >   .../lj-1166-error-stitch-oom-ir-buff.test.lua | 46 ++++++++++++++++
> >   ...j-1166-error-stitch-oom-snap-buff.test.lua | 54 +++++++++++++++++++
> >   .../lj-1166-error-stitch-table-bump.test.lua  | 38 +++++++++++++
> >   .../lj-1166-error-stitch/CMakeLists.txt       |  1 +
> >   .../lj-1166-error-stitch/mockalloc.c          | 51 ++++++++++++++++++
> >   .../lj-720-errors-before-stitch.test.lua      | 40 +++++++++++++-
> >   8 files changed, 245 insertions(+), 7 deletions(-)
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> >
> > diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> > index e3ed80fb..ff14e9e4 100644
> > --- a/src/lj_ffrecord.c
> > +++ b/src/lj_ffrecord.c

<snipped>

> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index 56660932..d7c96078 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -39,6 +39,7 @@ add_subdirectory(lj-802-panic-at-mcode-protfail)
> >   add_subdirectory(lj-flush-on-trace)
> >   add_subdirectory(lj-1004-oom-error-frame)
> >   add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
> > +add_subdirectory(lj-1166-error-stitch)
> >   
> >   # The part of the memory profiler toolchain is located in tools
> >   # directory, jit, profiler, and bytecode toolchains are located
> > 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
> > new file mode 100644
> > index 00000000..e3a5397d
> > --- /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.

> 
> > +  ['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.

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

> > +  local chunk = ''
> > +  chunk = chunk .. 'for i = 1, 2 do\n'
> > +  -- Each condition adds additional snapshot.
> > +  for i = 1, n_conds do
> > +    chunk = chunk .. ('  if i < %d then end\n'):format(i + n_conds)
> > +  end
> > +  -- `math.modf()` recording is NYI.
> > +  chunk = chunk .. '  math.modf(1)\n'
> > +  chunk = chunk .. 'end\n'
> > +  return chunk
> > +end
> > +
> > +-- XXX: Need to compile the cycle in the `create_chunk()` to
> > +-- preallocate the snapshot buffer.
> > +jit.opt.start('hotloop=1', '-loop', '-fold')
> > +
> > +-- XXX: Amount of slots is empirical.
> > +local tracef = assert(loadstring(create_chunk(6)))
> > +
> > +-- XXX: Remove previous trace.
> > +jit.off()
> > +jit.flush()
> > +
> > +-- XXX: Update hotcounts to avoid hash collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +jit.on()
> > +
> > +mockalloc.mock()
> > +
> > +tracef()
> > +
> > +mockalloc.unmock()
> > +
> > +test:ok(true, 'stack is balanced')
> > +
> > +test:done(true)
> > 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>

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

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

-- 
Best regards,
Sergey Kaplun