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