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

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

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@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,/

> defined `__concat` metamethod. The problem is GC64-specific.

Typo: s/defined/the defined/

> 
> Assuming we have three literals and a userdata with defined "__concat"

Typo: s/defined/the defined/

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

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

> implementation of function `lua_concat`. Without the fix `n` is equal to

Typo: s/in implementation/in the implementation/
Typo: s/fix/fix,/

> 3 at the end of the first iteration and therefore it goes to the next

Typo: s/ and/, and/

> 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()`,/

> decremented in a loop postcondition and breaks the loop.

Typo: s/ and/, and/

> 

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.

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

> ---
> 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/,//

> + * calling `lua_concat()` with userdata with __concat metamethod.

Typo: s/__concat/the __concat/

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

> +	{"__concat",    INFO_concat},

Typo: s/,    /, /

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

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

> +

<snipped>

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

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun