Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

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

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

Hi, Sergey,

fixes applied and force-pushed.

Sergey

On 10.07.2024 17:08, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, after fixing minor comments below.
>
> On 10.07.24, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>> thanks for review. Fixes applied and force-pushed.
>>
>> Sergey
>>
>>
>> On 09.07.2024 15:14, Sergey Kaplun via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> Please consider my comments below.
>>>
>>> On 09.07.24, Sergey Bronnikov wrote:
>>>> From: Mike Pall <mike>
>>>>
> <snipped>
>
>>> Minor: "will be collected at the end of the cycle if it is created after
>>> the start phase."
>> Updated.
> | Previous patch fixes the problem partially because the introduced
> | GC root may not exist at the start phase of the GC cycle. In that
> | case, the cdata finalizer table will be collected at the end of
> | the cycle.
>
> Minor: "cycle (since it isn't marked because it is not accessible from
> any GC root)."
Updated.
>
> |            Access to the cdata finalizer table exhibits heap use
> | after free. The patch turns the finalizer table into a proper
> <snipped>
>
>>>> diff --git a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
>>>> index c388c6a7..259528cb 100644
>>>> --- a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
>>>> +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
> <snipped>
>
>>>> +
>>>> +	/* Not trigger GC during `lua_openffi()`. */
>>>> +	lua_gc(L, LUA_GCSTOP, 0);
>>> Maybe it is worth adding this GC stop for the first test case too to
>>> make it more robust.
>> Ok, I'll add.
> Thanks!
>
>>>> +
>>>> +	int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
> I suggest renaming "chunk" to the "test_chunk" here too.

Fixed, but after this the line becomes longer max length and I need to 
split it for two lines:

--- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
@@ -83,7 +83,8 @@ unmarked_finalizer_tab_gcmark(void *test_state)
         /* Not trigger GC during `lua_openffi()`. */
         lua_gc(L, LUA_GCSTOP, 0);

-       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
+       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,
+                                  "test_chunk", "t");
         if (res != LUA_OK) {
                 test_comment("error loading Lua chunk: %s", 
lua_tostring(L, -1));
                 bail_out("error loading Lua chunk");

I would leave "chunk" due to this. And you?


>
> Also, please add here comment about `sizeof(buff) - 1` too.
>
>
>>>> +	assert_true(res == LUA_OK);
> <snipped>
>
>> --- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
>> +++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
>> @@ -78,7 +78,10 @@ unmarked_finalizer_tab_gcsweep(void *test_state)
>>           lua_gc(L, LUA_GCSTOP, 0);
>>
>>           int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk",
>> "t");
>> -       assert_true(res == LUA_OK);
>> +       if (res != LUA_OK) {
>> +               test_comment("error loading Lua chunk: %s",
>> lua_tostring(L, -1));
> Code line length is more than 80 symbols.
> (Same for the previous commit.)
Fixed for both commits.
>
>
>> +               bail_out("error loading Lua chunk");
>> +       }
>>
>>           /* Finish GC cycle. */
>>           while (!lua_gc(L, LUA_GCSTEP, -1));
>>
>>>> +
>>>> +	/* Finish GC cycle. */
> Let's add "to collect the finalizer table." to be consistent with
> another test.

Fixed:


--- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
+++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
@@ -93,7 +93,7 @@ unmarked_finalizer_tab_gcmark(void *test_state)
                 bail_out("error loading Lua chunk");
         }

-       /* Finish GC cycle. */
+       /* Finish GC cycle to collect the finalizer table. */
         while (!lua_gc(L, LUA_GCSTEP, -1));

         /* Teardown. */

>
>>>> +	while (!lua_gc(L, LUA_GCSTEP, -1));
>>>> +
> <snipped>
>

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