Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix lua_concat().

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

Hi, Max


thanks for review, see my comments below


Sergey

On 9/29/23 11:24, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> On Tue, Sep 26, 2023 at 09:56:31AM +0300, Sergey Bronnikov via Tarantool-patches wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Reported by Mathias Westerdahl.
>>
>> (cherry picked from commit 633f265f67f322cbe2c5fd11d3e46d968ac220f7)
>>
>> Lua 5.1 Reference Manual [1] defines a function `lua_concat`, that:
>>
>>> void lua_concat (lua_State *L, int n);
>>>
>>> Concatenates the n values at the top of the stack, pops them, and leaves
>>> the result at the top.
>> Without the patch `lua_concat()` behaved incorrectly with userdata with
>> defined `__concat` metamethod. The problem is GC64-specific.
>>
>> Assuming we have three literals and a userdata with defined "__concat"
>> metamethod on top of the Lua stack:
>>
>> 1 [string]
>> 2 [string]
>> 3 [string]
>> 4 [string]
>> 5 [userdata] <--- top
>>
>> On attempt to concatenate *two* items on top Lua stack, `lua_concat()`
> Typo: s/on top/on top of the/
Fixed.
>> concatenates *four* items and leaves result at the top:
> Typo: s/at the/on the/
Fixed.
>> 1 [string]
>> 2 [string][string][string][userdata] <--- top
>>
>> The problem is in incorrect calculation of `n` counter in a loop in
> Typo: s/in a loop/in the loop/
Fixed.
>> implementation of function `lua_concat`. Without a fix `n` is equal to 3
> Typo: s/a fix/the fix/
Fixed.
>> at the end of the first iteration and therefore it goes to the next
>> iteration of concatenation. In a fixed implementation of `lua_concat()`
> Typo: s/In a fixed/In the fixed/
Fixed.
>> `n` is equal to 1 at the end of the first loop iteration, decremented in
>> a loop postcondition and breaks a loop.
> Typo: s/a loop/the loop/
Fixed.

<snipped> because no errors, typos, objections were found

>> +
>> +static const luaL_Reg INFO_methods[] =
>> +{
>> +	{0, 0}
> Nit: It is more common to write `NULL` as sentinel.
Fixed.
>> +};
> This table is redundant.
Fixed.
>> +
>> +static const luaL_Reg INFO_meta[] =
>> +{
>> +	{"__concat",    INFO_concat},
>> +	{0, 0}
> Nit: It is more common to write `NULL` as sentinel.
Fixed too.
>> +};
>> +
>> +static int lua_concat_testcase(void *test_state)
>> +{
>> +	lua_State *L = test_state;
>> +
>> +	/* Create methods table, add it to the globals. */
>> +	luaL_register(L, TYPE_NAME_INFO, INFO_methods);
>> +	int methods_idx = lua_gettop(L);
> The methods table is redundant.
Fixed as well.
>> +	/* Create metatable and add it to the Lua registry. */
>> +	luaL_newmetatable(L, TYPE_NAME_INFO);
>> +
>> +	int metatable_idx = lua_gettop(L);
>> +	/* Fill metatable. */
>> +	luaL_register(L, 0, INFO_meta);
> The middle argument is meant to be NULL.
Sure, fixed.
>> +
>> +	lua_pushliteral(L, "__metatable");
>> +	/* Duplicate methods table. */
>> +	lua_pushvalue(L, methods_idx);
>> +	lua_settable(L, metatable_idx);
>> +	lua_pop(L, 2);
> This whole part can be reduced to this:
> | /* Create metatable and add it to the Lua registry. */
> | luaL_newmetatable(L, TYPE_NAME_INFO);
> |
> | /* Fill metatable. */
> | luaL_register(L, 0, INFO_meta);
> | lua_pop(L, 1);
> |
Reduced,  thanks.
>
>> +
>> +	assert_true(lua_gettop(L) == 0);
>> +
>> +	Info info;
>> +	info.value = 7;
>> +
>> +	lua_pushliteral(L, "C");
>> +	lua_pushliteral(L, "B");
>> +	lua_pushliteral(L, "A");
>> +	lua_pushinfo(L, &info);
>> +
>> +	int top = 4;
>> +	assert_true(lua_gettop(L) == top);
>> +
>> +	/**
>> +	 * void lua_concat (lua_State *L, int n);
>> +	 *
>> +	 * Concatenates the n values at the top of the stack,
>> +	 * pops them, and leaves the result at the top. If n is 1,
>> +	 * the result is the single value on the stack; if n is 0,
>> +	 * the result is the empty string [1].
>> +	 *
>> +	 * 1. https://www.lua.org/manual/5.1/manual.html
>> +	 */
>> +	int n = 2;
>> +	lua_concat(L, n);
>> +
>> +	const char *str = lua_tostring(L, -1);
>> +	assert_true(top - n + 1 == lua_gettop(L));
> We have `assert_int_equal` in `test.h` for that.

Sure, added in a separate patch and updated branch force-pushed.

commit 71f4161115c2e2c13c91e2804f9c41f673ada5ce
Author: Sergey Bronnikov <sergeyb@tarantool.org>
Date:   Tue Oct 3 17:39:48 2023 +0300

     test: introduce asserts assert_str{_not}_equal

     The patch follows up commit a0483bd214f2 ("test: introduce module for C
     tests") and adds additional asserts suitable for comparing strings.

diff --git a/test/tarantool-c-tests/README.md 
b/test/tarantool-c-tests/README.md
index 462534be..8fad6407 100644
--- a/test/tarantool-c-tests/README.md
+++ b/test/tarantool-c-tests/README.md
@@ -35,6 +35,8 @@ glibc `assert()`:
    equal to the `b`.
  * `assert_double{_not}_equal(a, b)` -- check that two doubles are {not}
    **exactly** equal.
+* `assert_str{_not}_equal(a, b)` -- check that `char *` variable `a` is 
{not}
+  equal to the `b`.

  ## Directives

diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
index 8b14c705..bbf573b2 100644
--- a/test/tarantool-c-tests/test.h
+++ b/test/tarantool-c-tests/test.h
@@ -13,8 +13,6 @@
   * * Helpers assert macros:
   *   - assert_uint_equal if needed
   *   - assert_uint_not_equal if needed
- *   - assert_str_equal if needed
- *   - assert_str_not_equal if needed
   *   - assert_memory_equal if needed
   *   - assert_memory_not_equal if needed
   * * Pragmas.
@@ -214,4 +212,19 @@ static inline int todo(const char *reason)
      );                                \
  } while (0)

+#define assert_str_equal(got, expected, n) do {                \
+    assert_general(strncmp(got, expected, n) == 0,            \
+               ASSERT_EQUAL_FMT(int, "%d"),            \
+               __FILE__, __LINE__, (got), (expected)        \
+    );                                \
+} while (0)
+
+#define assert_str_not_equal(got, unexpected, n) do { \
+    assert_general(strncmp(got, expected, n) != 0,            \
+               ASSERT_NOT_EQUAL_FMT(int, "%d"),            \
+               __FILE__, __LINE__, (got), (unexpected)        \
+    );                                \
+} while (0)
+
+
  #endif /* TARANTOOL_LUAJIT_TEST_H */

>> +	assert_true(strcmp(str, "A[Info.value=7]") == 0);
> One should never use any of the unrestricted string functions, as
> they are unsafe. Always use the `n`-limited versions: `strncmp`
> instead of `strcmp`, `strnlen` instead of `strlen`, etc.
>
> Side note: there is a TODO in test.h about `assert_str_equal`,
> maybe it's a great time to introduce it in this patch.
Ok, replaced with strncmp.
>
>> +
>> +	/* Cleanup. */
>> +	lua_settop(L, 0);
>> +
>> +	return TEST_EXIT_SUCCESS;
>> +}
> Some general comments about that function:
> 1. Please mark variables as `const` where possible.
Done.
> 2. C style requires you to put variable declarations at the
> top of the function.
Done.
> 3. You have three distinct parts in this function: set up procedure,
> the test itself, and the the tear down. It would be better to split them
> into three separate functions to improve readability.

Added a comments for splitting sections visually.

>> +
>> +int main(void)
>> +{
>> +	lua_State *L = utils_lua_init();
>> +	const struct test_unit tgroup[] = {
>> +		test_unit_def(lua_concat_testcase),
>> +	};
>> +	const int test_result = test_run_group(tgroup, L);
>> +	utils_lua_close(L);
>> +
>> +	return test_result;
>> +}
>> -- 
>> 2.34.1
>>
> Best regards,
> Maxim Kokryashkin