Re: [Tarantool-patches] [PATCH v3 luajit 6/6] test: rewrite lj-49-bad-lightuserdata test in C

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

Sergey,

thanks for a third version of the patch!

LGTM in general, however I have a minor comment, see below.


On 6/5/23 13:41, Sergey Kaplun wrote:
> This patch rewrites the aforementioned test with the usage libtest
> recently introduced. The approach is similar to the previous patch.
>
> Nevertheless, glibc `assert()` is used to check the correctness
> of the `mmap()` usage.
>
> Part of tarantool/tarantool#7900
> ---
>   .../lj-49-bad-lightuserdata.test.c}           | 47 ++++++++++---------
>   test/tarantool-tests/CMakeLists.txt           |  1 -
>   .../lj-49-bad-lightuserdata.test.lua          | 11 -----
>   .../lj-49-bad-lightuserdata/CMakeLists.txt    |  1 -
>   4 files changed, 25 insertions(+), 35 deletions(-)
>   rename test/{tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c => tarantool-c-tests/lj-49-bad-lightuserdata.test.c} (55%)
>   delete mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
>   delete mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
>
> diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c b/test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
> similarity index 55%
> rename from test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
> rename to test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
> index 1b909fc6..a9cc4763 100644
> --- a/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
> +++ b/test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
> @@ -1,16 +1,21 @@
> -#include <lua.h>
> -#include <lauxlib.h>
> +#include "lua.h"
> +#include "lauxlib.h"
>   
>   #include <sys/mman.h>
>   #include <unistd.h>
>   
> -#undef NDEBUG
> -#include <assert.h>
> +#include "test.h"
> +#include "utils.h"
>   
>   #define START ((void *)-1)
>   
> -static int crafted_ptr(lua_State *L)
> +/* XXX: Still need normal assert to validate mmap correctness. */
> +#undef NDEBUG
> +#include <assert.h>
> +
> +static int crafted_ptr(void *test_state)

Nit: I would rename test_state to something like lua_state here and in 
mmaped_ptr.

Feel free to ignore.

>   {
> +	lua_State *L = test_state;
>   	/*
>   	 * We know that for arm64 at least 48 bits are available.
>   	 * So emulate manually push of lightuseradata within
> @@ -18,15 +23,15 @@ static int crafted_ptr(lua_State *L)
>   	 */
>   	void *longptr = (void *)(1llu << 48);
>   	lua_pushlightuserdata(L, longptr);
> -	assert(longptr == lua_topointer(L, -1));
> +	assert_ptr_equal(longptr, lua_topointer(L, -1));
>   	/* Clear our stack. */
>   	lua_pop(L, 0);
> -	lua_pushboolean(L, 1);
> -	return 1;
> +	return TEST_EXIT_SUCCESS;
>   }
>   
> -static int mmaped_ptr(lua_State *L)
> +static int mmaped_ptr(void *test_state)
>   {
> +	lua_State *L = test_state;
>   	/*
>   	 * If start mapping address is not NULL, then the kernel
>   	 * takes it as a hint about where to place the mapping, so
> @@ -38,24 +43,22 @@ static int mmaped_ptr(lua_State *L)
>   			    -1, 0);
>   	if (mmaped != MAP_FAILED) {
>   		lua_pushlightuserdata(L, mmaped);
> -		assert(mmaped == lua_topointer(L, -1));
> +		assert_ptr_equal(mmaped, lua_topointer(L, -1));
>   		assert(munmap(mmaped, pagesize) == 0);
>   	}
>   	/* Clear our stack. */
>   	lua_pop(L, 0);
> -	lua_pushboolean(L, 1);
> -	return 1;
> +	return TEST_EXIT_SUCCESS;
>   }
>   
> -static const struct luaL_Reg testlightuserdata[] = {
> -	{"crafted_ptr", crafted_ptr},
> -	{"mmaped_ptr", mmaped_ptr},
> -	{NULL, NULL}
> -};
> -
> -LUA_API int luaopen_testlightuserdata(lua_State *L)
> +int main(void)
>   {
> -	luaL_register(L, "testlightuserdata", testlightuserdata);
> -	return 1;
> +	lua_State *L = utils_lua_init();
> +	const struct test_unit tgroup[] = {
> +		test_unit_new(crafted_ptr),
> +		test_unit_new(mmaped_ptr)
> +	};
> +	const int test_result = test_run_group(tgroup, L);
> +	utils_lua_close(L);
> +	return test_result;
>   }
> -
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index b1c7207f..527905b6 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -61,7 +61,6 @@ add_subdirectory(gh-5813-resolving-of-c-symbols/gnuhash)
>   add_subdirectory(gh-5813-resolving-of-c-symbols/stripped)
>   add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
>   add_subdirectory(gh-6189-cur_L)
> -add_subdirectory(lj-49-bad-lightuserdata)
>   add_subdirectory(lj-416-xor-before-jcc)
>   add_subdirectory(lj-601-fix-gc-finderrfunc)
>   add_subdirectory(lj-727-lightuserdata-itern)
> diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua b/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
> deleted file mode 100644
> index 94a743c7..00000000
> --- a/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -local tap = require('tap')
> -
> -local test = tap.test('lj-49-bad-lightuserdata')
> -test:plan(2)
> -
> -local testlightuserdata = require('testlightuserdata')
> -
> -test:ok(testlightuserdata.crafted_ptr())
> -test:ok(testlightuserdata.mmaped_ptr())
> -
> -os.exit(test:check() and 0 or 1)
> diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt b/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
> deleted file mode 100644
> index ec6bb62c..00000000
> --- a/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
> +++ /dev/null
> @@ -1 +0,0 @@
> -BuildTestCLib(testlightuserdata testlightuserdata.c)