[Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
Sergey Kaplun
skaplun at tarantool.org
Mon Oct 9 17:11:09 MSK 2023
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,/
> 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
More information about the Tarantool-patches
mailing list