Re: [Tarantool-patches] [PATCH luajit 1/3] test: unify helpers for a custom allocator setting

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

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

Hi, Sergey!

Thanks for the patch! LGTM with two comments below.

Feel free to ignore.

Sergey

On 10.03.2025 17:51, Sergey Kaplun wrote:
> This patch merges two Lua C libraries that contain custom Lua allocators
> with the injection of returning `NULL` into 1 library
Minor: s/1/a single/
> <utils/allocinject.c>. The new library contains 2 helpers:
Minor: s/2/two/
> * `enable_null_alloc()` -- returns `NULL` on every allocation (not
>    reallocation).
> * `enable_null_doubling_realloc()` -- returns `NULL` on every
>    reallocation that doubles the size of the object. It can be useful for
>    testing corner cases, like OOM during IR/snapshot buffer reallocation
>    during trace recording.
> The original allocator is returned via the `disable()` library function.
>
> The unified library helps to avoid code duplication and can be reused
> for future tests.
>
> Needed for tarantool/tarantool#11055
> ---
>   test/tarantool-tests/CMakeLists.txt           |  5 +-
>   .../lj-1166-error-stitch-oom-ir-buff.test.lua |  2 +-
>   ...j-1166-error-stitch-oom-snap-buff.test.lua |  2 +-
>   ...j-1247-fin-tab-rehashing-on-trace.test.lua |  4 +-
>   .../CMakeLists.txt                            |  2 -
>   .../lj_1247_allocinject.c                     | 49 -------------------
>   .../CMakeLists.txt                            |  1 +
>   .../allocinject.c                             | 34 +++++++++++--
>   8 files changed, 37 insertions(+), 62 deletions(-)
>   delete mode 100644 test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/CMakeLists.txt
>   delete mode 100644 test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/lj_1247_allocinject.c
>   rename test/tarantool-tests/{lj-1166-error-stitch => utils}/CMakeLists.txt (77%)
>   rename test/tarantool-tests/{lj-1166-error-stitch => utils}/allocinject.c (50%)
>
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index a6d09dc7..682a883a 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -60,13 +60,14 @@ 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)
> -add_subdirectory(lj-1247-fin-tab-rehashing-on-trace)
>   add_subdirectory(profilers/gh-5813-resolving-of-c-symbols/both)
>   add_subdirectory(profilers/gh-5813-resolving-of-c-symbols/gnuhash)
>   add_subdirectory(profilers/gh-5813-resolving-of-c-symbols/hash)
>   add_subdirectory(profilers/gh-5813-resolving-of-c-symbols/stripped)
>   
> +# Module for alloc injections to return OOM for allocations.
> +add_subdirectory(utils)
> +
>   if(BUILDMODE STREQUAL "dynamic")
>     add_subdirectory(fix-argv-handling)
>   endif()
> 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 354f5975..dc21cfbf 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
> @@ -57,7 +57,7 @@ jparse.start('t')
>   jit.on()
>   jit.opt.start('hotloop=1', '-loop', '-fold')
>   
> -allocinject.enable()
> +allocinject.enable_null_doubling_realloc()
>   
>   tracef()
>   
> 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 c8e13760..b2387f23 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
> @@ -59,7 +59,7 @@ jparse.start('t')
>   jit.opt.start('hotloop=1')
>   jit.on()
>   
> -allocinject.enable()
> +allocinject.enable_null_doubling_realloc()
>   
>   tracef()
>   
> diff --git a/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace.test.lua b/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace.test.lua
> index 308043a2..b9a49700 100644
> --- a/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace.test.lua
> @@ -24,7 +24,7 @@ local test = tap.test('lj-1247-fin-tab-rehashing-on-trace'):skipcond({
>   -- omitted, we test only the first case.
>   test:plan(2)
>   
> -local allocinject = require('lj_1247_allocinject')
> +local allocinject = require('allocinject')
>   
>   local ffi = require('ffi')
>   ffi.cdef[[
> @@ -115,7 +115,7 @@ crash_on_trace_unwind_gc_setup()
>   
>   -- OOM on every allocation (i.e., on finalizer table rehashing
>   -- too).
> -allocinject.enable()
> +allocinject.enable_null_alloc()
>   
>   local r, err = pcall(f, res_tab)
>   
> diff --git a/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/CMakeLists.txt b/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/CMakeLists.txt
> deleted file mode 100644
> index 8c6fe7e4..00000000
> --- a/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/CMakeLists.txt
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -get_filename_component(test_name ${CMAKE_CURRENT_SOURCE_DIR} NAME)
> -BuildTestCLib(lj_1247_allocinject lj_1247_allocinject.c ${test_name}.test.lua)
> diff --git a/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/lj_1247_allocinject.c b/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/lj_1247_allocinject.c
> deleted file mode 100644
> index 81aea60b..00000000
> --- a/test/tarantool-tests/lj-1247-fin-tab-rehashing-on-trace/lj_1247_allocinject.c
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -#include "lua.h"
> -#include "lauxlib.h"
> -
> -#undef NDEBUG
> -#include <assert.h>
> -
> -static lua_Alloc old_allocf = NULL;
> -static void *old_alloc_state = NULL;
> -
> -/* Function to be used instead of the default allocator. */
> -static void *allocf_with_injection(void *ud, void *ptr, size_t osize,
> -				   size_t nsize)
> -{
> -	/* Always OOM on allocation (not on realloc). */
> -	if (ptr == NULL)
> -		return NULL;
> -	else
> -		return old_allocf(ud, ptr, osize, nsize);
> -}
> -
> -static int enable(lua_State *L)
> -{
> -	assert(old_allocf == NULL);
> -	old_allocf = lua_getallocf(L, &old_alloc_state);
> -	lua_setallocf(L, allocf_with_injection, old_alloc_state);
> -	return 0;
> -}
> -
> -static int disable(lua_State *L)
> -{
> -	assert(old_allocf != NULL);
> -	assert(old_allocf != allocf_with_injection);
> -	lua_setallocf(L, old_allocf, old_alloc_state);
> -	old_allocf = NULL;
> -	old_alloc_state = NULL;
> -	return 0;
> -}
> -
> -static const struct luaL_Reg allocinject[] = {
> -	{"enable", enable},
> -	{"disable", disable},
> -	{NULL, NULL}
> -};
> -
> -LUA_API int luaopen_lj_1247_allocinject(lua_State *L)
> -{
> -	luaL_register(L, "lj_1247_allocinject", allocinject);
> -	return 1;
> -}
> diff --git a/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt b/test/tarantool-tests/utils/CMakeLists.txt
> similarity index 77%
> rename from test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
> rename to test/tarantool-tests/utils/CMakeLists.txt
> index 658b3b8c..624cc933 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
> +++ b/test/tarantool-tests/utils/CMakeLists.txt
> @@ -1,5 +1,6 @@
>   list(APPEND tests
>     lj-1166-error-stitch-oom-ir-buff.test.lua
>     lj-1166-error-stitch-oom-snap-buff.test.lua
> +  lj-1247-fin-tab-rehashing-on-trace.test.lua
>   )
>   BuildTestCLib(allocinject allocinject.c "${tests}")
> diff --git a/test/tarantool-tests/lj-1166-error-stitch/allocinject.c b/test/tarantool-tests/utils/allocinject.c
> similarity index 50%
> rename from test/tarantool-tests/lj-1166-error-stitch/allocinject.c
> rename to test/tarantool-tests/utils/allocinject.c
> index 88fc9138..21e1d611 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch/allocinject.c
> +++ b/test/tarantool-tests/utils/allocinject.c
> @@ -7,9 +7,22 @@
>   static lua_Alloc old_allocf = NULL;
>   static void *old_alloc_state = NULL;
>   
> -/* Function to be used instead of the default allocator. */
> -static void *allocf_with_injection(void *ud, void *ptr, size_t osize,
> +/* Functions to be used instead of the default allocator. */
> +
> +/* Always OOM on allocation (not on realloc). */
> +static void *allocf_inj_null_alloc(void *ud, void *ptr, size_t osize,
>   				   size_t nsize)
> +{
> +	assert(old_allocf != NULL);
> +	if (ptr == NULL)
> +		return NULL;
> +	else
> +		return old_allocf(ud, ptr, osize, nsize);
> +}
> +
> +/* Returns `NULL` on reallocations doubling used memory. */
> +static void *allocf_inj_null_doubling_realloc(void *ud, void *ptr, size_t osize,
> +					      size_t nsize)
>   {
>   	assert(old_allocf != NULL);
>   	/*
> @@ -21,7 +34,7 @@ static void *allocf_with_injection(void *ud, void *ptr, size_t osize,
>   	return old_allocf(ud, ptr, osize, nsize);
>   }
>   
> -static int enable(lua_State *L)
> +static int enable(lua_State *L, lua_Alloc allocf_with_injection)
>   {
>   	assert(old_allocf == NULL);
>   	old_allocf = lua_getallocf(L, &old_alloc_state);
> @@ -29,10 +42,20 @@ static int enable(lua_State *L)
>   	return 0;
>   }
>   
> +static int enable_null_alloc(lua_State *L)
> +{
> +	return enable(L, allocf_inj_null_alloc);
> +}
> +
> +static int enable_null_doubling_realloc(lua_State *L)
> +{
> +	return enable(L, allocf_inj_null_doubling_realloc);
> +}
> +
> +/* Restore the default allocator function. */
>   static int disable(lua_State *L)
>   {
>   	assert(old_allocf != NULL);
> -	assert(old_allocf != allocf_with_injection);
>   	lua_setallocf(L, old_allocf, old_alloc_state);
>   	old_allocf = NULL;
>   	old_alloc_state = NULL;
> @@ -40,7 +63,8 @@ static int disable(lua_State *L)
>   }
>   
>   static const struct luaL_Reg allocinject[] = {
> -	{"enable", enable},
> +	{"enable_null_alloc", enable_null_alloc},
> +	{"enable_null_doubling_realloc", enable_null_doubling_realloc},
>   	{"disable", disable},
>   	{NULL, NULL}
>   };

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