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

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

[Sergey Bronnikov via Tarantool-patches]

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()`
concatenates *four* items and leaves result at the top:

1 [string]
2 [string][string][string][userdata] <--- top

The problem is in incorrect calculation of `n` counter in a loop in
implementation of function `lua_concat`. Without a fix `n` is equal to 3
at the end of the first iteration and therefore it goes to the next
iteration of concatenation. In a fixed implementation of `lua_concat()`
`n` is equal to 1 at the end of the first loop iteration, decremented in
a loop postcondition and breaks a loop.

The patch fixes incorrect behaviour.

1. https://www.lua.org/manual/5.1/manual.html

Sergey Bronnikov:
* added the description and the test for the problem

Part of tarantool/tarantool#8825
---
PR: https://github.com/tarantool/tarantool/pull/9176
Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat
Issues:
	- https://github.com/LuaJIT/LuaJIT/issues/881
	- https://github.com/tarantool/tarantool/issues/8825

 src/lj_api.c                                  |   2 +-
 .../lj-881-fix-lua-concat.test.c              | 116 ++++++++++++++++++
 2 files changed, 117 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-c-tests/lj-881-fix-lua-concat.test.c

diff --git a/src/lj_api.c b/src/lj_api.c
index 05e02029..3bacad33 100644
--- a/src/lj_api.c
+++ b/src/lj_api.c
@@ -801,7 +801,7 @@ LUA_API void lua_concat(lua_State *L, int n)
 	L->top -= n;
 	break;
       }
-      n -= (int)(L->top - top);
+      n -= (int)(L->top - (top - 2*LJ_FR2));
       L->top = top+2;
       jit_secure_call(L, top, 1+1);
       L->top -= 1+LJ_FR2;
diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
new file mode 100644
index 00000000..464ed27e
--- /dev/null
+++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
@@ -0,0 +1,116 @@
+#include <string.h>
+
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+#include "utils.h"
+
+/*
+ * This test demonstrates LuaJIT's incorrect behaviour, when
+ * calling `lua_concat()` with userdata with __concat metamethod.
+ * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
+ */
+
+#define TYPE_NAME_INFO "Info"
+
+typedef struct Info
+{
+	int value;
+} Info;
+
+static void lua_pushinfo(lua_State *L, Info *info)
+{
+	Info *infop = (Info *)lua_newuserdata(L, sizeof(Info));
+	*infop = *info;
+
+	luaL_getmetatable(L, TYPE_NAME_INFO);
+	lua_setmetatable(L, -2);
+}
+
+static int INFO_concat(lua_State *L)
+{
+	const char *s = luaL_checkstring(L, 1);
+	Info *info = (Info*)luaL_checkudata(L, 2, TYPE_NAME_INFO);
+	lua_pushfstring(L, "%s[%s.value=%d]", s, TYPE_NAME_INFO, info->value);
+	return 1;
+}
+
+static const luaL_Reg INFO_methods[] =
+{
+	{0, 0}
+};
+
+static const luaL_Reg INFO_meta[] =
+{
+	{"__concat",    INFO_concat},
+	{0, 0}
+};
+
+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);
+	/* 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);
+
+	lua_pushliteral(L, "__metatable");
+	/* Duplicate methods table. */
+	lua_pushvalue(L, methods_idx);
+	lua_settable(L, metatable_idx);
+	lua_pop(L, 2);
+
+	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));
+	assert_true(strcmp(str, "A[Info.value=7]") == 0);
+
+	/* Cleanup. */
+	lua_settop(L, 0);
+
+	return TEST_EXIT_SUCCESS;
+}
+
+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

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

[Maxim Kokryashkin via Tarantool-patches]

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/
> concatenates *four* items and leaves result at the top:
Typo: s/at the/on the/
> 
> 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/
> implementation of function `lua_concat`. Without a fix `n` is equal to 3
Typo: s/a fix/the fix/
> 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/
> `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/
> 
> The patch fixes incorrect behaviour.
> 
> 1. https://www.lua.org/manual/5.1/manual.html
> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> PR: https://github.com/tarantool/tarantool/pull/9176
> Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat
> Issues:
> 	- https://github.com/LuaJIT/LuaJIT/issues/881
> 	- https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_api.c                                  |   2 +-
>  .../lj-881-fix-lua-concat.test.c              | 116 ++++++++++++++++++
>  2 files changed, 117 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> 
> diff --git a/src/lj_api.c b/src/lj_api.c
> index 05e02029..3bacad33 100644
> --- a/src/lj_api.c
> +++ b/src/lj_api.c
> @@ -801,7 +801,7 @@ LUA_API void lua_concat(lua_State *L, int n)
>  	L->top -= n;
>  	break;
>        }
> -      n -= (int)(L->top - top);
> +      n -= (int)(L->top - (top - 2*LJ_FR2));
>        L->top = top+2;
>        jit_secure_call(L, top, 1+1);
>        L->top -= 1+LJ_FR2;
> diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> new file mode 100644
> index 00000000..464ed27e
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> @@ -0,0 +1,116 @@
> +#include <string.h>
> +
> +#include "lua.h"
> +#include "lauxlib.h"
> +
> +#include "test.h"
> +#include "utils.h"
> +
> +/*
> + * This test demonstrates LuaJIT's incorrect behaviour, when
> + * calling `lua_concat()` with userdata with __concat metamethod.
> + * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
> + */
> +
> +#define TYPE_NAME_INFO "Info"
> +
> +typedef struct Info
> +{
> +	int value;
> +} Info;
> +
> +static void lua_pushinfo(lua_State *L, Info *info)
> +{
> +	Info *infop = (Info *)lua_newuserdata(L, sizeof(Info));
> +	*infop = *info;
> +
> +	luaL_getmetatable(L, TYPE_NAME_INFO);
> +	lua_setmetatable(L, -2);
> +}
> +
> +static int INFO_concat(lua_State *L)
> +{
> +	const char *s = luaL_checkstring(L, 1);
> +	Info *info = (Info*)luaL_checkudata(L, 2, TYPE_NAME_INFO);
> +	lua_pushfstring(L, "%s[%s.value=%d]", s, TYPE_NAME_INFO, info->value);
> +	return 1;
> +}
> +
> +static const luaL_Reg INFO_methods[] =
> +{
> +	{0, 0}
Nit: It is more common to write `NULL` as sentinel.
> +};
This table is redundant.
> +
> +static const luaL_Reg INFO_meta[] =
> +{
> +	{"__concat",    INFO_concat},
> +	{0, 0}
Nit: It is more common to write `NULL` as sentinel.
> +};
> +
> +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.
> +	/* 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.
> +
> +	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);
|

> +
> +	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.
> +	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.

> +
> +	/* Cleanup. */
> +	lua_settop(L, 0);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
Some general comments about that function:
1. Please mark variables as `const` where possible.
2. C style requires you to put variable declarations at the
top of the function.
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.
> +
> +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

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

[Sergey Bronnikov via Tarantool-patches]

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

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, except for the single comment below.
 > > +	Info info;
> > > +	info.value = 7;

I am really sorry, that I didn't noticed that in the first round,
but the test case can be reduced a bit more. We don't really need
the `Info` structure and can use some basic type like `int`, for
example. One might argue that such change can reduce the readability
a bit, so I leave the final decision up to you.
<snipped>
 
> 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 */
This patch LGTM too. However, please re-send the whole
series as `v2` if you'll need to do similar changes next time.

Best regards,
Maxim Kokryashkin

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Max

On 10/4/23 13:48, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, except for the single comment below.
>   > > +	Info info;
>>>> +	info.value = 7;
> I am really sorry, that I didn't noticed that in the first round,
> but the test case can be reduced a bit more. We don't really need
> the `Info` structure and can use some basic type like `int`, for
> example. One might argue that such change can reduce the readability
> a bit, so I leave the final decision up to you.
> <snipped>
>   
>
Thanks for suggestion, it has reduced test a bit.

Below is a patch where struct was replaced with int (changes 
force-pushed and I'll send v2 as you requested):


diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c 
b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
index 83925445..1b1f146f 100644
--- a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
+++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
@@ -12,31 +12,18 @@
   * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
   */

-#define TYPE_NAME_INFO "Info"
-
-typedef struct Info
-{
-    int payload;
-} Info;
-
-static void lua_pushinfo(lua_State *L, Info *info)
-{
-    Info *infop = (Info *)lua_newuserdata(L, sizeof(Info));
-    *infop = *info;
-
-    luaL_getmetatable(L, TYPE_NAME_INFO);
-    lua_setmetatable(L, -2);
-}
+#define MT_NAME "int"

  static int INFO_concat(lua_State *L)
  {
      const char *s = luaL_checkstring(L, 1);
-    Info *info = (Info*)luaL_checkudata(L, 2, TYPE_NAME_INFO);
-    lua_pushfstring(L, "%s[%s.payload=%d]", s, TYPE_NAME_INFO, 
info->payload);
+    int *n = (int *)luaL_checkudata(L, 2, MT_NAME);
+    /* Do non-default concatenation. */
+    lua_pushfstring(L, "%s + %d", s, *n);
      return 1;
  }

-static const luaL_Reg INFO_meta[] =
+static const luaL_Reg mt[] =
  {
      {"__concat",    INFO_concat},
      {NULL, NULL}
@@ -46,17 +33,12 @@ static int lua_concat_testcase(void *test_state)
  {
      /* Setup. */
      lua_State *L = test_state;
-    Info info = {
-        .payload = 7
-    };
      const int top = 4;
-    const int n = 2;
-
-    /* Create metatable and add it to the Lua registry. */
-    luaL_newmetatable(L, TYPE_NAME_INFO);

+    /* Create metatable and put it to the Lua registry. */
+    luaL_newmetatable(L, MT_NAME);
      /* Fill metatable. */
-    luaL_register(L, 0, INFO_meta);
+    luaL_register(L, 0, mt);
      lua_pop(L, 1);

      assert_int_equal(lua_gettop(L), 0);
@@ -64,13 +46,18 @@ static int lua_concat_testcase(void *test_state)
      lua_pushliteral(L, "C");
      lua_pushliteral(L, "B");
      lua_pushliteral(L, "A");
-    lua_pushinfo(L, &info);

-    assert_true(lua_gettop(L) == top);
+    int *n = (int *)lua_newuserdata(L, sizeof(int));
+    *n = 100;
+
+    luaL_getmetatable(L, MT_NAME);
+    lua_setmetatable(L, -2);
+
+    assert_int_equal(lua_gettop(L), top);

      /* Test body. */

-    /**
+    /*
       * void lua_concat (lua_State *L, int n);
       *
       * Concatenates the n values at the top of the stack,
@@ -80,11 +67,13 @@ static int lua_concat_testcase(void *test_state)
       *
       * 1. https://www.lua.org/manual/5.1/manual.html
       */
-    lua_concat(L, n);
+
+    /* Concatenate two elements on top. */
+    lua_concat(L, 2);

      const char *str = lua_tostring(L, -1);
-    assert_true(top - n + 1 == lua_gettop(L));
-    const char expected_str[] = "A[Info.payload=7]";
+    assert_int_equal(lua_gettop(L), top - 2 + 1);
+    const char expected_str[] = "A + 100";
      assert_str_equal(str, expected_str, strlen(expected_str));

      /* Teardown. */


<snipped>

[Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

The patch follows up commit a0483bd214f2 ("test: introduce module for C
tests") and adds additional asserts suitable for comparing strings.
---
 test/tarantool-c-tests/README.md |  2 ++
 test/tarantool-c-tests/test.h    | 17 +++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

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 */
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please, consider my comments below.

On 31.10.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> The patch follows up commit a0483bd214f2 ("test: introduce module for C
> tests") and adds additional asserts suitable for comparing strings.
> ---
>  test/tarantool-c-tests/README.md |  2 ++
>  test/tarantool-c-tests/test.h    | 17 +++++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/test/tarantool-c-tests/README.md b/test/tarantool-c-tests/README.md

<snipped>

> 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

<snipped>

> @@ -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"),			\

Typo: s/int/str/

> +		       __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"),			\

Typo: s/int/str/

> +		       __FILE__, __LINE__, (got), (unexpected)		\
> +	);								\
> +} while (0)

If some test fails we got the following output:

| TAP version 13
| 1..1
| not ok 1 - lua_concat_testcase
|   ---
|   location:     /home/burii/reviews/luajit/lj-881-lua-concat/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c:81
|   failed_assertion:     assert_str_equal
|   got: 1652880104
|   expected: -1138225337
|   ...
| # Failed 1 tests out of 1

Failed assertion field is incorrect (see the comment above).
But the most important is "got" and "expected" fields, that returns the
addresses of strings, which isn't very meaningful.

I suggest dumping the strings instead if they are not long enough (less
than 128 symbols, I suppose). The maxdump string length should be
a customizeable parameter. I suggest defining a macro `MAX_DUMP_STRLEN`
inside the <test.h> header. So the user can redefine it before the
`assert_str_{not_}equal()` and use a custom value.

If the string is too long, we should dump the offset of the mismatched
symbol. Or maybe it's better to always dump it.

Thoughts?

Side note:

Also, this comparing "by eye" isn't very convenient if values aren't
aligned, so maybe it is better to use spaces instead of tabs to align
values? This may be added within the separate patch series later.

> +
> +

Nit: Excess empty line.

>  #endif /* TARANTOOL_LUAJIT_TEST_H */
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Igor Munkin via Tarantool-patches]

Sergey,

On 01.11.23, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!

<snipped>

> 
> If some test fails we got the following output:
> 
> | TAP version 13
> | 1..1
> | not ok 1 - lua_concat_testcase
> |   ---
> |   location:     /home/burii/reviews/luajit/lj-881-lua-concat/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c:81
> |   failed_assertion:     assert_str_equal
> |   got: 1652880104
> |   expected: -1138225337
> |   ...
> | # Failed 1 tests out of 1
> 
> Failed assertion field is incorrect (see the comment above).
> But the most important is "got" and "expected" fields, that returns the
> addresses of strings, which isn't very meaningful.
> 
> I suggest dumping the strings instead if they are not long enough (less
> than 128 symbols, I suppose). The maxdump string length should be
> a customizeable parameter. I suggest defining a macro `MAX_DUMP_STRLEN`
> inside the <test.h> header. So the user can redefine it before the
> `assert_str_{not_}equal()` and use a custom value.
> 
> If the string is too long, we should dump the offset of the mismatched
> symbol. Or maybe it's better to always dump it.
> 
> Thoughts?

What if the different part starts after 128 symbols? I believe the most
valuable part is the one where the difference starts, so you have to
dump the beginning (for convenience), the difference and some numeric
parameters (length, offset where strings differ, etc).

Furthermore, I suggest implementing <*_str_*> helpers for nul-terminated
strings and <*_mem_*> helpers for length limited memory blobs.

> 
> Side note:
> 
> Also, this comparing "by eye" isn't very convenient if values aren't
> aligned, so maybe it is better to use spaces instead of tabs to align
> values? This may be added within the separate patch series later.

I believe, this is quite minor thing (at least for now).

> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Bronnikov via Tarantool-patches]

Hi, Igor

On 11/1/23 11:28, Igor Munkin via Tarantool-patches wrote:
> Sergey,
>
> On 01.11.23, Sergey Kaplun via Tarantool-patches wrote:
>> Hi, Sergey!
> <snipped>
>
>> If some test fails we got the following output:
>>
>> | TAP version 13
>> | 1..1
>> | not ok 1 - lua_concat_testcase
>> |   ---
>> |   location:     /home/burii/reviews/luajit/lj-881-lua-concat/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c:81
>> |   failed_assertion:     assert_str_equal
>> |   got: 1652880104
>> |   expected: -1138225337
>> |   ...
>> | # Failed 1 tests out of 1
>>
>> Failed assertion field is incorrect (see the comment above).
>> But the most important is "got" and "expected" fields, that returns the
>> addresses of strings, which isn't very meaningful.
>>
>> I suggest dumping the strings instead if they are not long enough (less
>> than 128 symbols, I suppose). The maxdump string length should be
>> a customizeable parameter. I suggest defining a macro `MAX_DUMP_STRLEN`
>> inside the <test.h> header. So the user can redefine it before the
>> `assert_str_{not_}equal()` and use a custom value.
>>
>> If the string is too long, we should dump the offset of the mismatched
>> symbol. Or maybe it's better to always dump it.
>>
>> Thoughts?
> What if the different part starts after 128 symbols? I believe the most
> valuable part is the one where the difference starts, so you have to
> dump the beginning (for convenience), the difference and some numeric
> parameters (length, offset where strings differ, etc).
>
> Furthermore, I suggest implementing <*_str_*> helpers for nul-terminated
> strings and <*_mem_*> helpers for length limited memory blobs.
I would postpone implementation of  <*_mem_*> helpers until we will need 
them in tests.
>
>> Side note:
>>
>> Also, this comparing "by eye" isn't very convenient if values aren't
>> aligned, so maybe it is better to use spaces instead of tabs to align
>> values? This may be added within the separate patch series later.
> I believe, this is quite minor thing (at least for now).
>
> <snipped>
>
>> -- 
>> Best regards,
>> Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes.
Please, consider my comments below.

On 10.11.23, Sergey Bronnikov wrote:
> Hi, Igor
> 
> On 11/1/23 11:28, Igor Munkin via Tarantool-patches wrote:
> > Sergey,
> >
> > On 01.11.23, Sergey Kaplun via Tarantool-patches wrote:
> >> Hi, Sergey!
> > <snipped>
> >
> >> If some test fails we got the following output:
> >>
> >> | TAP version 13
> >> | 1..1
> >> | not ok 1 - lua_concat_testcase
> >> |   ---
> >> |   location:     /home/burii/reviews/luajit/lj-881-lua-concat/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c:81
> >> |   failed_assertion:     assert_str_equal
> >> |   got: 1652880104
> >> |   expected: -1138225337
> >> |   ...
> >> | # Failed 1 tests out of 1
> >>
> >> Failed assertion field is incorrect (see the comment above).
> >> But the most important is "got" and "expected" fields, that returns the
> >> addresses of strings, which isn't very meaningful.
> >>
> >> I suggest dumping the strings instead if they are not long enough (less
> >> than 128 symbols, I suppose). The maxdump string length should be
> >> a customizeable parameter. I suggest defining a macro `MAX_DUMP_STRLEN`
> >> inside the <test.h> header. So the user can redefine it before the
> >> `assert_str_{not_}equal()` and use a custom value.
> >>
> >> If the string is too long, we should dump the offset of the mismatched
> >> symbol. Or maybe it's better to always dump it.
> >>
> >> Thoughts?
> > What if the different part starts after 128 symbols? I believe the most
> > valuable part is the one where the difference starts, so you have to
> > dump the beginning (for convenience), the difference and some numeric
> > parameters (length, offset where strings differ, etc).
> >
> > Furthermore, I suggest implementing <*_str_*> helpers for nul-terminated
> > strings and <*_mem_*> helpers for length limited memory blobs.
> I would postpone implementation of  <*_mem_*> helpers until we will need 
> them in tests.

I'm totally OK with it.

> >
> >> Side note:
> >>
> >> Also, this comparing "by eye" isn't very convenient if values aren't
> >> aligned, so maybe it is better to use spaces instead of tabs to align
> >> values? This may be added within the separate patch series later.
> > I believe, this is quite minor thing (at least for now).
> >
> > <snipped>
> >
> >> -- 
> >> Best regards,
> >> Sergey Kaplun

I'll proceed with the review here with the diff below:

> 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..2f2d9ea2 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,18 @@ static inline int todo(const char *reason)
>          );                                                              \
>  } while (0)
>  
> +#define assert_str_equal(got, expected, n) do {                         \
> +        assert_general(strncmp(got, expected, n) == 0,                  \

Maybe it is better to just use `strcmp(got, expected)` instead?
We don't really care about the number of characters to check.

Or we can use `strncmp(got, expected, strlen(expected))`, as the most
common case.

This removes an additional argument, and the description in the
<README.md> and declaration here become the same.

> +                       ASSERT_EQUAL_FMT(str, "%s"),                     \
> +                       __FILE__, __LINE__, (got), (expected)            \
> +        );                                                              \
> +} while (0)
> +
> +#define assert_str_not_equal(got, unexpected, n) do {                   \
> +        assert_general(strncmp(got, expected, n) != 0,                  \

Ditto.

> +                       ASSERT_NOT_EQUAL_FMT(str, "%s"),                 \
> +                       __FILE__, __LINE__, (got), (unexpected)          \
> +        );                                                              \
> +} while (0)
> +
>  #endif /* TARANTOOL_LUAJIT_TEST_H */

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Bronnikov via Tarantool-patches]

Hello, Sergey

On 11/14/23 11:55, Sergey Kaplun wrote:


<snipped>

> I'll proceed with the review here with the diff below:
>
>> 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..2f2d9ea2 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,18 @@ static inline int todo(const char *reason)
>>           );                                                              \
>>   } while (0)
>>   
>> +#define assert_str_equal(got, expected, n) do {                         \
>> +        assert_general(strncmp(got, expected, n) == 0,                  \
> Maybe it is better to just use `strcmp(got, expected)` instead?
> We don't really care about the number of characters to check.
>
> Or we can use `strncmp(got, expected, strlen(expected))`, as the most
> common case.
>
> This removes an additional argument, and the description in the
> <README.md> and declaration here become the same.
>
>> +                       ASSERT_EQUAL_FMT(str, "%s"),                     \
>> +                       __FILE__, __LINE__, (got), (expected)            \
>> +        );                                                              \
>> +} while (0)
>> +
>> +#define assert_str_not_equal(got, unexpected, n) do {                   \
>> +        assert_general(strncmp(got, expected, n) != 0,                  \
> Ditto.

Fixed by patch below:


diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c 
b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
index 1835d273..f028c457 100644
--- a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
+++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
@@ -77,7 +77,7 @@ static int lua_concat_testcase(void *test_state)
      const char *str = lua_tostring(L, -1);
      assert_int_equal(lua_gettop(L), top - 2 + 1);
      const char expected_str[] = CONCAT("A + ", TEST_VALUE);
-    assert_str_equal(str, expected_str, strlen(expected_str));
+    assert_str_equal(str, expected_str);

      /* Teardown. */
      lua_settop(L, 0);
diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
index 2f2d9ea2..5e5c96b2 100644
--- a/test/tarantool-c-tests/test.h
+++ b/test/tarantool-c-tests/test.h
@@ -212,15 +212,15 @@ static inline int todo(const char *reason)
      );                                \
  } while (0)

-#define assert_str_equal(got, expected, n) do {                \
-    assert_general(strncmp(got, expected, n) == 0,            \
+#define assert_str_equal(got, expected) do {                \
+    assert_general(strncmp(got, expected, strlen(expected)) == 0,    \
                 ASSERT_EQUAL_FMT(str, "%s"),            \
                 __FILE__, __LINE__, (got), (expected)        \
      );                                \
  } while (0)

-#define assert_str_not_equal(got, unexpected, n) do { \
-    assert_general(strncmp(got, expected, n) != 0,            \
+#define assert_str_not_equal(got, unexpected) do {            \
+    assert_general(strncmp(got, expected, strlen(expected)) != 0,    \
                 ASSERT_NOT_EQUAL_FMT(str, "%s"),            \
                 __FILE__, __LINE__, (got), (unexpected)        \
      );                                \

>
>> +                       ASSERT_NOT_EQUAL_FMT(str, "%s"),                 \
>> +                       __FILE__, __LINE__, (got), (unexpected)          \
>> +        );                                                              \
>> +} while (0)
>> +
>>   #endif /* TARANTOOL_LUAJIT_TEST_H */

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing my comment below.

On 15.11.23, Sergey Bronnikov wrote:
> Hello, Sergey
> 
> On 11/14/23 11:55, Sergey Kaplun wrote:
> 
> 
> <snipped>
> 
> > I'll proceed with the review here with the diff below:
> >
> >> 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..2f2d9ea2 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,18 @@ static inline int todo(const char *reason)
> >>           );                                                              \
> >>   } while (0)
> >>   
> >> +#define assert_str_equal(got, expected, n) do {                         \
> >> +        assert_general(strncmp(got, expected, n) == 0,                  \
> > Maybe it is better to just use `strcmp(got, expected)` instead?
> > We don't really care about the number of characters to check.
> >
> > Or we can use `strncmp(got, expected, strlen(expected))`, as the most
> > common case.
> >
> > This removes an additional argument, and the description in the
> > <README.md> and declaration here become the same.
> >
> >> +                       ASSERT_EQUAL_FMT(str, "%s"),                     \
> >> +                       __FILE__, __LINE__, (got), (expected)            \
> >> +        );                                                              \
> >> +} while (0)
> >> +
> >> +#define assert_str_not_equal(got, unexpected, n) do {                   \
> >> +        assert_general(strncmp(got, expected, n) != 0,                  \
> > Ditto.
> 
> Fixed by patch below:
> 
> 
> diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c 
> b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> index 1835d273..f028c457 100644
> --- a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> +++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> @@ -77,7 +77,7 @@ static int lua_concat_testcase(void *test_state)
>       const char *str = lua_tostring(L, -1);
>       assert_int_equal(lua_gettop(L), top - 2 + 1);
>       const char expected_str[] = CONCAT("A + ", TEST_VALUE);
> -    assert_str_equal(str, expected_str, strlen(expected_str));
> +    assert_str_equal(str, expected_str);
> 
>       /* Teardown. */
>       lua_settop(L, 0);
> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
> index 2f2d9ea2..5e5c96b2 100644
> --- a/test/tarantool-c-tests/test.h
> +++ b/test/tarantool-c-tests/test.h
> @@ -212,15 +212,15 @@ static inline int todo(const char *reason)
>       );                                \
>   } while (0)
> 
> -#define assert_str_equal(got, expected, n) do {                \
> -    assert_general(strncmp(got, expected, n) == 0,            \
> +#define assert_str_equal(got, expected) do {                \
> +    assert_general(strncmp(got, expected, strlen(expected)) == 0,    \

On the second thought, I insist on using `strcmp()` here since the
expected string is always `\0` terminated, and we use "not safe" the
`strlen()` anyway.

>                  ASSERT_EQUAL_FMT(str, "%s"),            \
>                  __FILE__, __LINE__, (got), (expected)        \
>       );                                \
>   } while (0)
> 
> -#define assert_str_not_equal(got, unexpected, n) do { \
> -    assert_general(strncmp(got, expected, n) != 0,            \
> +#define assert_str_not_equal(got, unexpected) do {            \
> +    assert_general(strncmp(got, expected, strlen(expected)) != 0,    \

Ditto.

>                  ASSERT_NOT_EQUAL_FMT(str, "%s"),            \
>                  __FILE__, __LINE__, (got), (unexpected)        \
>       );                                \
> 
> >
> >> +                       ASSERT_NOT_EQUAL_FMT(str, "%s"),                 \
> >> +                       __FILE__, __LINE__, (got), (unexpected)          \
> >> +        );                                                              \
> >> +} while (0)
> >> +
> >>   #endif /* TARANTOOL_LUAJIT_TEST_H */

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Bronnikov via Tarantool-patches]

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

Hello,


On 11/16/23 11:02, Sergey Kaplun wrote:


<snipped>

>> diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>> b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>> index 1835d273..f028c457 100644
>> --- a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>> +++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>> @@ -77,7 +77,7 @@ static int lua_concat_testcase(void *test_state)
>>        const char *str = lua_tostring(L, -1);
>>        assert_int_equal(lua_gettop(L), top - 2 + 1);
>>        const char expected_str[] = CONCAT("A + ", TEST_VALUE);
>> -    assert_str_equal(str, expected_str, strlen(expected_str));
>> +    assert_str_equal(str, expected_str);
>>
>>        /* Teardown. */
>>        lua_settop(L, 0);
>> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
>> index 2f2d9ea2..5e5c96b2 100644
>> --- a/test/tarantool-c-tests/test.h
>> +++ b/test/tarantool-c-tests/test.h
>> @@ -212,15 +212,15 @@ static inline int todo(const char *reason)
>>        );                                \
>>    } while (0)
>>
>> -#define assert_str_equal(got, expected, n) do {                \
>> -    assert_general(strncmp(got, expected, n) == 0,            \
>> +#define assert_str_equal(got, expected) do {                \
>> +    assert_general(strncmp(got, expected, strlen(expected)) == 0,    \
> On the second thought, I insist on using `strcmp()` here since the
> expected string is always `\0` terminated, and we use "not safe" the
> `strlen()` anyway.


Updated.

diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
index 5e5c96b2..3b22fb92 100644
--- a/test/tarantool-c-tests/test.h
+++ b/test/tarantool-c-tests/test.h
@@ -213,14 +213,14 @@ static inline int todo(const char *reason)
  } while (0)

  #define assert_str_equal(got, expected) do {                           \
-       assert_general(strncmp(got, expected, strlen(expected)) == 0,   \
+       assert_general(strcmp(got, expected) == 0,                      \
                        ASSERT_EQUAL_FMT(str, "%s"),                     \
                        __FILE__, __LINE__, (got), (expected)            \
);                                                              \
  } while (0)

  #define assert_str_not_equal(got, unexpected) do {                     \
-       assert_general(strncmp(got, expected, strlen(expected)) != 0,   \
+       assert_general(strcmp(got, expected) != 0,                      \
                        ASSERT_NOT_EQUAL_FMT(str, "%s"),                 \
                        __FILE__, __LINE__, (got), (unexpected)          \
);                                                              \

>
>>                   ASSERT_EQUAL_FMT(str, "%s"),            \
>>                   __FILE__, __LINE__, (got), (expected)        \
>>        );                                \
>>    } while (0)
>>
>> -#define assert_str_not_equal(got, unexpected, n) do { \
>> -    assert_general(strncmp(got, expected, n) != 0,            \
>> +#define assert_str_not_equal(got, unexpected) do {            \
>> +    assert_general(strncmp(got, expected, strlen(expected)) != 0,    \
> Ditto.
Ditto.
>
>>                   ASSERT_NOT_EQUAL_FMT(str, "%s"),            \
>>                   __FILE__, __LINE__, (got), (unexpected)        \
>>        );                                \
>>
>>>> +                       ASSERT_NOT_EQUAL_FMT(str, "%s"),                 \
>>>> +                       __FILE__, __LINE__, (got), (unexpected)          \
>>>> +        );                                                              \
>>>> +} while (0)
>>>> +
>>>>    #endif /* TARANTOOL_LUAJIT_TEST_H */

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

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Kaplun via Tarantool-patches]

Hello, Sergey!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the updates! LGTM after all.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce asserts assert_str{_not}_equal

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

changes applied and force-pushed.

On 11/1/23 10:40, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please, consider my comments below.
>
> On 31.10.23, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> The patch follows up commit a0483bd214f2 ("test: introduce module for C
>> tests") and adds additional asserts suitable for comparing strings.
>> ---
>>   test/tarantool-c-tests/README.md |  2 ++
>>   test/tarantool-c-tests/test.h    | 17 +++++++++++++++--
>>   2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/test/tarantool-c-tests/README.md b/test/tarantool-c-tests/README.md
> <snipped>
>
>> 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
> <snipped>
>
>> @@ -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"),			\
> Typo: s/int/str/
Fixed, thanks.
>
>> +		       __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"),			\
> Typo: s/int/str/

Fixed, thanks.


>> +		       __FILE__, __LINE__, (got), (unexpected)		\
>> +	);								\
>> +} while (0)
> If some test fails we got the following output:
>
> | TAP version 13
> | 1..1
> | not ok 1 - lua_concat_testcase
> |   ---
> |   location:     /home/burii/reviews/luajit/lj-881-lua-concat/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c:81
> |   failed_assertion:     assert_str_equal
> |   got: 1652880104
> |   expected: -1138225337
> |   ...
> | # Failed 1 tests out of 1
>
> Failed assertion field is incorrect (see the comment above).
> But the most important is "got" and "expected" fields, that returns the
> addresses of strings, which isn't very meaningful.
>
> I suggest dumping the strings instead if they are not long enough (less
> than 128 symbols, I suppose). The maxdump string length should be
> a customizeable parameter. I suggest defining a macro `MAX_DUMP_STRLEN`
> inside the <test.h> header. So the user can redefine it before the
> `assert_str_{not_}equal()` and use a custom value.
I don't like your idea for the same reasons described by Igor.
> If the string is too long, we should dump the offset of the mismatched
> symbol. Or maybe it's better to always dump it.
>
> Thoughts?

Now implementation of `assert_str_{not}_equal` is good enough for 
proposed test.

Don't get why we should think about comparison of long strings.

We could update assert implementation as soon it will be required by 
other tests,

now it is not  required.

>
> Side note:
>
> Also, this comparing "by eye" isn't very convenient if values aren't
> aligned, so maybe it is better to use spaces instead of tabs to align
> values? This may be added within the separate patch series later.
>
>> +
>> +
> Nit: Excess empty line.
Fixed, thanks.
>
>>   #endif /* TARANTOOL_LUAJIT_TEST_H */
>> -- 
>> 2.34.1
>>

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 26.09.23, 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()`
> concatenates *four* items and leaves result at the top:
> 
> 1 [string]
> 2 [string][string][string][userdata] <--- top
> 
> The problem is in incorrect calculation of `n` counter in a loop in
> implementation of function `lua_concat`. Without a fix `n` is equal to 3
> at the end of the first iteration and therefore it goes to the next
> iteration of concatenation. In a fixed implementation of `lua_concat()`
> `n` is equal to 1 at the end of the first loop iteration, decremented in
> a loop postcondition and breaks a loop.
> 
> The patch fixes incorrect behaviour.
> 
> 1. https://www.lua.org/manual/5.1/manual.html
> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> PR: https://github.com/tarantool/tarantool/pull/9176
> Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat
> Issues:
> 	- https://github.com/LuaJIT/LuaJIT/issues/881
> 	- https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_api.c                                  |   2 +-
>  .../lj-881-fix-lua-concat.test.c              | 116 ++++++++++++++++++
>  2 files changed, 117 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM