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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8140 bytes --]


Hi, Sergey!
Thanks for the patch!
LGTM
> 
>>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
> 
Best regards,
Maxim Kokryashkin

[-- Attachment #2: Type: text/html, Size: 10017 bytes --]