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

[Sergey Bronnikov]

Hi, Sergey

On 09.10.2023 17:11, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, after fixing a few nits mentioned below.
>
> On 06.10.23, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb at 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
> Typo: s/patch/patch,/
Fixed.
>
>> defined `__concat` metamethod. The problem is GC64-specific.
> Typo: s/defined/the defined/

Fixed.

>
>> Assuming we have three literals and a userdata with defined "__concat"
> Typo: s/defined/the defined/
Fixed.
>
>> 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 of the Lua stack,
>> `lua_concat()` concatenates *four* items and leaves result on the top:
> Typo: s/result/the result/
> Typo: s/the top/top of the Lua stack/
>     or s/on the top/at the top of the Lua stack/
Fixed.
>
>> 1 [string]
>> 2 [string][string][string][userdata] <--- top
>>
>> The problem is in incorrect calculation of `n` counter in the loop in
> Typo: s/in incorrect/in the incorrect/
Fixed.
>
>> implementation of function `lua_concat`. Without the fix `n` is equal to
> Typo: s/in implementation/in the implementation/
> Typo: s/fix/fix,/

Fixed.


>
>> 3 at the end of the first iteration and therefore it goes to the next
> Typo: s/ and/, and/
Fixed.
>
>> iteration of concatenation. In the fixed implementation of
>> `lua_concat()` `n` is equal to 1 at the end of the first loop iteration,
> Typo: s/`lua_concat()`/`lua_concat()`,/

Fixed.


>
>> decremented in a loop postcondition and breaks the loop.
> Typo: s/ and/, and/

Fixed.


> Minor: Also, we can mention that for details, the reader can see
> `lj_meta_cat()` implementation in <src/lj_meta.c>.
> Feel free to ignore.

Added.


>> 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
> I suppose this should be #9145.

Done.


>
>> ---
>> 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
> <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
>> new file mode 100644
>> index 00000000..1b1f146f
>> --- /dev/null
>> +++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>> @@ -0,0 +1,95 @@
>> +#include <string.h>
>> +
>> +#include "lua.h"
>> +#include "lauxlib.h"
>> +
>> +#include "test.h"
>> +#include "utils.h"
>> +
>> +/*
>> + * This test demonstrates LuaJIT's incorrect behaviour, when
> Typo: s/,//

Fixed.


>
>> + * calling `lua_concat()` with userdata with __concat metamethod.
> Typo: s/__concat/the __concat/
Fixed.
>
>> + * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
>> + */
>> +
>> +#define MT_NAME "int"
>> +
>> +static int INFO_concat(lua_State *L)
>> +{
>> +	const char *s = luaL_checkstring(L, 1);
>> +	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 mt[] =
>> +{
> Minor: I suppose this line may be joined with the previous one, as far
> as this is not a function's body.

Joined.


>
>> +	{"__concat",    INFO_concat},
> Typo: s/,    /, /

Fixed.


>
>> +	{NULL, NULL}
>> +};
>> +
>> +static int lua_concat_testcase(void *test_state)
>> +{
> <snipped>
>
>> +
>> +	int *n = (int *)lua_newuserdata(L, sizeof(int));
> I suggest to use `sizeof(*n)` here to avoid type duplication.

Fixed.


>
>> +	*n = 100;
> I suppose we can use something like the following:
>
> | #define TEST_VALUE 100
> | #define STR(s) #s
> | #define XSTR(s) STR(s)
> | #define TEST_VALUE_STR XSTR(TEST_VALUE)
>
> or just use #TEST_VALUE in the `expected_str[]` declaration.
> This helps to avoid the duplication of the magic number `100`.

Updated:



  #define TYPE_NAME "int"

+#define TEST_VALUE 100
+#define TOSTR(s) #s
+#define CONCAT(A, B) A TOSTR(B)
+
  static int __concat(lua_State *L)
  {
         const char *s = luaL_checkstring(L, 1);


@@ -73,7 +76,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[] = "A + 100";
+       const char expected_str[] = CONCAT("A + ", TEST_VALUE);
         assert_str_equal(str, expected_str, strlen(expected_str));

         /* Teardown. */

>
>> +
> <snipped>
>
>> +	const char expected_str[] = "A + 100";
>> +	assert_str_equal(str, expected_str, strlen(expected_str));
> <snipped>
>
>> -- 
>> 2.34.1
>>