Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

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

Hi, again!

Added minor fixes.

On 11.11.22, Sergey Kaplun via Tarantool-patches wrote:

<snipped>

> ===================================================================
> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> index f512e802..0ec1b30e 100644
> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> @@ -12,7 +12,7 @@ local KB = 1024
>  local MB = 1024 * KB
>  
>  -- The maximum available table size, taking into account created
> --- constants for one function.
> +-- constants for one function and nested level.
>  local TNEW_SIZE = 511
>  
>  local gc_anchor = {}
> @@ -29,9 +29,8 @@ local function eat_chunks(size)
>  end
>  
>  -- Function to format inner tab leading to TDUP emitting.
> -local function format_inner_tab()
> +local function make_flat_table(inner_depth)

Should be deep_table, obviously.

===================================================================
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
index 7dbc1614..69f74893 100644
--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -29,7 +29,7 @@ local function eat_chunks(size)
 end
 
 -- Function to format inner tab leading to TDUP emitting.
-local function make_flat_table(inner_depth)
+local function make_deep_table(inner_depth)
   local inner_tab = ''
   -- Repeate table template for TDUP.
   for _ = 1, inner_depth do
@@ -49,7 +49,7 @@ local function format_TDUP_chunk()
   local big_tab = 'local _ = {\n'
   -- The maximum available table size, taking into account created
   -- constants for one function and nested level.
-  local inner_tab = make_flat_table(128)
+  local inner_tab = make_deep_table(128)
   for _ = 1, TNEW_SIZE do
     big_tab = big_tab .. inner_tab .. '\n'
   end
===================================================================

>    local inner_tab = ''
> -  local inner_depth = 128
>    -- Repeate table template for TDUP.
>    for _ = 1, inner_depth do
>      inner_tab = inner_tab .. '{a ='
> @@ -43,9 +42,14 @@ local function format_inner_tab()
>    return inner_tab
>  end
>  
> +-- The `lj_err_mem()` doesn't fix `L->top`, when called from
> +-- helper function (like `lj_tab_dup()`) before the patch.
> +-- This function creates a chunk with many BC_TDUP inside.
>  local function format_TDUP_chunk()
>    local big_tab = 'local _ = {\n'
> -  local inner_tab = format_inner_tab()
> +  -- The maximum available table size, taking into account created
> +  -- constants for one function and nested level.
> +  local inner_tab = make_flat_table(128)
>    for _ = 1, TNEW_SIZE do
>      big_tab = big_tab .. inner_tab .. '\n'
>    end
> @@ -56,12 +60,10 @@ end
>  local TDUP, err = loadstring(format_TDUP_chunk())
>  assert(TDUP, err)
>  
> +-- Function to create the additional frame after to be rewrited
> +-- one in case of `lj_err_mem()` misbehaviour.

Also, minor comments fixes and renaming.
===================================================================
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
index 69f74893..f4ec6620 100644
--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -45,7 +45,7 @@ end
 -- The `lj_err_mem()` doesn't fix `L->top`, when called from
 -- helper function (like `lj_tab_dup()`) before the patch.
 -- This function creates a chunk with many BC_TDUP inside.
-local function format_TDUP_chunk()
+local function make_TDUP_chunk()
   local big_tab = 'local _ = {\n'
   -- The maximum available table size, taking into account created
   -- constants for one function and nested level.
@@ -57,11 +57,11 @@ local function format_TDUP_chunk()
   return big_tab
 end
 
-local TDUP, err = loadstring(format_TDUP_chunk())
+local TDUP, err = loadstring(make_TDUP_chunk())
 assert(TDUP, err)
 
--- Function to create the additional frame after to be rewrited
--- one in case of `lj_err_mem()` misbehaviour.
+-- Function  to create the additional frame to be rewritten later
+-- in case of `lj_err_mem()` misbehaviour.
 local function frame_before_TDUP()
   TDUP()
 end
===================================================================

Branch is force-pushed.
>  local function frame_before_TDUP()
> -  -- Stack slots are needed for coredump in case of misbehaviour.
> -  -- luacheck: no unused
> -  local frame_slot1, frame_slot2
>    TDUP()
> -  return frame_slot1, frame_slot2
>  end
>  
>  collectgarbage()
> ===================================================================
> 
> 
> > 
> > > +  -- luacheck: no unused
> > > +  local frame_slot1, frame_slot2
> > > +  TDUP()
> > > +  return frame_slot1, frame_slot2
> > > +end
> > > +
> > 
> > Minor: I believe you can pack these two lines into something one can
> > call <janitor>:
> > | local function janitor()
> > |   collectgarbage('collect')
> > |   collectgarbage('stop')
> > | end
> > 
> > Anyway, you can leave this as is.
> 
> Added, thanks!
> See the iterative patch below.
> ===================================================================
> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> index 519e6bf6..7dbc1614 100644
> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> @@ -66,8 +66,12 @@ local function frame_before_TDUP()
>    TDUP()
>  end
>  
> -collectgarbage()
> -collectgarbage('stop')
> +local function janitor()
> +  collectgarbage('collect')
> +  collectgarbage('stop')
> +end
> +
> +janitor()
>  
>  -- Avoid OOM on traces.
>  jit.off()
> @@ -75,14 +79,11 @@ jit.off()
>  -- Stack slots are needed for coredump in case of misbehaviour.
>  -- luacheck: no unused
>  local r, e = pcall(eat_chunks, 8 * MB)
> -collectgarbage()
> -collectgarbage('stop')
> +janitor()
>  pcall(eat_chunks, 8 * KB)
> -collectgarbage()
> -collectgarbage('stop')
> +janitor()
>  pcall(eat_chunks, 8)
> -collectgarbage()
> -collectgarbage('stop')
> +janitor()
>  
>  pcall(frame_before_TDUP)
> ===================================================================
> 
>  
> > 
> > > +collectgarbage()
> > > +collectgarbage('stop')
> > > +
> > > +-- Avoid OOM on traces.
> > > +jit.off()
> > > +
> > > +-- Stack slots are needed for coredump in case of misbehaviour.
> > > +-- luacheck: no unused
> > > +local r, e = pcall(eat_chunks, 8 * MB)
> > > +collectgarbage()
> > > +pcall(eat_chunks, 8 * KB)
> > > +collectgarbage()
> > > +pcall(eat_chunks, 8)
> > > +collectgarbage()
> > > +
> > > +pcall(frame_before_TDUP)
> > 
> > Do we need to check the status of the <pcall> above?
> 
> I suppose no. As we disscussed offline the test may be flaky (false
> positive, when we can alloc all tables) due to memory allocations.
> Also, this part is very fragile, and we don't want to fix test later by
> removing this check the status of the `pcall()`.
> 
> > 
> > > +
> > > +-- Release memory for `tap` functions.
> > > +gc_anchor = nil
> > > +collectgarbage()
> > 
> > Is this step required? I doubt.
> 
> Yes it is. Without it we may get the OOM error inside `test:ok()` or
> `test:check()` functions (I faced it on my local machine, when rewrite
> this test).
> 
> > 
> > > +
> > > +test:ok(true, 'correctly throw memory error')
> > > +
> > > +os.exit(test:check() and 0 or 1)
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
Sergey Kaplun