Re: [Tarantool-patches] [PATCH luajit] FFI: Fix recording of union initialization.

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

Hi, Max!


thanks for the patch!

See my comments inline.


Sergey

On 7/10/23 18:56, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> Thanks to Alex Shpilkin.
>
> (cherry-picked from commit 56c04accf975bff2519c34721dccbbdb7b8e6963)
>
> As stated here[1], only the first field of a union can be
> initialized with a flat initializer. However, before this
> patch, on-trace initialization instructions were emitted
> for other union members too, overwriting the previous
> initialization values.
>
> This patch fixes the mentioned behavior by preventing
> initialization of members other than the first one.
>
> [1]: https://luajit.org/ext_ffi_semantics.html#init
>
> Maxim Kokryashkin:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#8825
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-128-fix-union-init
> PR: https://github.com/tarantool/tarantool/pull/8867
> Original LuaJIT PR: https://github.com/LuaJIT/LuaJIT/pull/650
>
>   src/lj_crecord.c                               |  5 +++++
>   .../lj-128-fix-union-init.test.lua             | 18 ++++++++++++++++++
>   2 files changed, 23 insertions(+)
>   create mode 100644 test/tarantool-tests/lj-128-fix-union-init.test.lua
>
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index 0008a865..ffe995f4 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -1065,6 +1065,11 @@ static void crec_alloc(jit_State *J, RecordFFData *rd, CTypeID id)
>   	  dp = emitir(IRT(IR_ADD, IRT_PTR), trcd,
>   		      lj_ir_kintp(J, df->size + sizeof(GCcdata)));
>   	  crec_ct_tv(J, dc, dp, sp, sval);
> +	  if ((d->info & CTF_UNION)) {
> +	    if (d->size != dc->size)  /* NYI: partial init of union. */
> +	      lj_trace_err(J, LJ_TRERR_NYICONV);
> +	    break;
> +	  }
>   	} else if (!ctype_isconstval(df->info)) {
>   	  /* NYI: init bitfields and sub-structures. */
>   	  lj_trace_err(J, LJ_TRERR_NYICONV);
> diff --git a/test/tarantool-tests/lj-128-fix-union-init.test.lua b/test/tarantool-tests/lj-128-fix-union-init.test.lua
> new file mode 100644
> index 00000000..6a49cec8
> --- /dev/null
> +++ b/test/tarantool-tests/lj-128-fix-union-init.test.lua
> @@ -0,0 +1,18 @@
> +local tap = require('tap')
> +local test = tap.test('lj-128-fix-union-init'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +local NITERATIONS = 4
> +
> +test:plan(NITERATIONS)
> +
> +local ffi = require('ffi')
> +local union_type = ffi.typeof('union { uint32_t u; float f; }')
> +
> +jit.opt.start('hotloop=1')
> +for i = 1, NITERATIONS do
> +  test:ok(union_type(i).u == i)
testcases description is missed, please add one.
> +end
> +
> +os.exit(test:check() and 0 or 1)