Re: [Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

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

Hi!
Thanks for the patch!

Just a minor message update and a test extension - otherwise LGTM.

Sergos

> On 23 Aug 2022, at 17:27, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> Thanks to Egor Skriptunoff.
> 
> (cherry picked from commit cc4bbec483d3f3250b519ccb7cc22f1a8e6fe6f0)
> 
> When user tries to concatenate 2 cdata objects without declared
> metamethod, the assertion is raised in `carith_int64()`, due to
> concatenation operation is not specified and default (assert) branch is
> taken.

The above doesn’t explain the behavior - the default branch leads to what?
Neither it explains the expected behavior.

> 
> This patch forcifies usage of metamethod for concatenation on cdata
> objects. Also, as far as the behaviour for length operation is the same,
> the `lj_carith_len()` routine is removed, its call is replaced with
> `ffi_arith()`.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#7230
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/7230
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-cdata-ll-concat-full-ci
> PR: https://github.com/tarantool/tarantool/pull/7598
> ML: https://www.freelists.org/post/luajit/cdata-concatenation
> 
> src/lj_carith.c                                |  3 +--
> src/lj_crecord.c                               |  6 ++++--
> test/tarantool-tests/fix-cdata-concat.test.lua | 15 +++++++++++++++
> 3 files changed, 20 insertions(+), 4 deletions(-)
> create mode 100644 test/tarantool-tests/fix-cdata-concat.test.lua
> 
> diff --git a/src/lj_carith.c b/src/lj_carith.c
> index 218abd26..04c18054 100644
> --- a/src/lj_carith.c
> +++ b/src/lj_carith.c
> @@ -265,7 +265,7 @@ int lj_carith_op(lua_State *L, MMS mm)
> {
>   CTState *cts = ctype_cts(L);
>   CDArith ca;
> -  if (carith_checkarg(L, cts, &ca)) {
> +  if (carith_checkarg(L, cts, &ca) && mm != MM_len && mm != MM_concat) {
>     if (carith_int64(L, cts, &ca, mm) || carith_ptr(L, cts, &ca, mm)) {
>       copyTV(L, &G(L)->tmptv2, L->top-1);  /* Remember for trace recorder. */
>       return 1;
> @@ -347,7 +347,6 @@ uint64_t lj_carith_check64(lua_State *L, int narg, CTypeID *id)
>   }
> }
> 
> -
> /* -- 64 bit integer arithmetic helpers ----------------------------------- */
> 
> #if LJ_32 && LJ_HASJIT
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index 0d7b71f0..3d562d9a 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -1546,8 +1546,10 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>   }
>   {
>     TRef tr;
> -    if (!(tr = crec_arith_int64(J, sp, s, (MMS)rd->data)) &&
> -	!(tr = crec_arith_ptr(J, sp, s, (MMS)rd->data)) &&
> +    MMS mm = (MMS)rd->data;
> +    if ((mm == MM_len || mm == MM_concat ||
> +	 (!(tr = crec_arith_int64(J, sp, s, mm)) &&
> +	  !(tr = crec_arith_ptr(J, sp, s, mm)))) &&
> 	!(tr = crec_arith_meta(J, sp, s, cts, rd)))
>       return;
>     J->base[0] = tr;
> diff --git a/test/tarantool-tests/fix-cdata-concat.test.lua b/test/tarantool-tests/fix-cdata-concat.test.lua
> new file mode 100644
> index 00000000..aaeb36fa
> --- /dev/null
> +++ b/test/tarantool-tests/fix-cdata-concat.test.lua
> @@ -0,0 +1,15 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate incorrect behaviour of cdata
> +-- concatenation in LuaJIT.
> +-- See also
> +-- https://www.freelists.org/post/luajit/cdata-concatenation.
> +local test = tap.test('cdata-concat')
> +test:plan(1)
> +
> +local r, e = pcall(function()
> +  return 1LL .. 2LL
> +end)
> +test:ok(not r and e:match('attempt to concatenate'), 'cdata concatenation')

As with programmer’s joke about full and empty glass - let’s have a second case
for the existent metamethod? 

> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
>