Hi, Max!
Thanks for your fixes and especially for detailed explanation in a test.
As Sergey said, my comment was addressed to a missed testcase description.
Now I see that everything is fine.
Thanks, LGTM.
Sergey
Hi!Thanks for the review!Added a comment with the test description, the branch is force-pushed.Here is the diff for the test description:==============================================diff --git a/test/tarantool-tests/lj-128-fix-union-init.test.lua b/test/tarantool-tests/lj-128-fix-union-init.test.luaindex 6a49cec8..65ba0f28 100644--- a/test/tarantool-tests/lj-128-fix-union-init.test.lua+++ b/test/tarantool-tests/lj-128-fix-union-init.test.lua@@ -10,6 +10,46 @@ test:plan(NITERATIONS)local ffi = require('ffi')local union_type = ffi.typeof('union { uint32_t u; float f; }')+-- Before the patch, the `union_type` call resulted in the+-- initialization of both the integer and the float members+-- of the union, leading to undefined behavior since the+-- integer member was overwritten.+-- The IR was the following:+--+-- 0031 ------ LOOP ------------+-- 0032 u8 XLOAD [0x100684521] V+-- 0033 int BAND 0032 +12+-- 0034 > int EQ 0033 +0+-- 0035 > cdt CNEW +96+-- 0036 p64 ADD 0035 +16+-- 0037 u32 XSTORE 0036 0029 <--- `u` member init+-- 0038 flt XSTORE 0036 0022 <--- `f` member init+-- 0039 u32 XLOAD 0036+-- 0040 num CONV 0039 num.u32+-- 0041 num CONV 0029 num.int+-- 0042 > num EQ 0041 0040+-- 0043 + int ADD 0029 +1+-- 0044 > int LE 0043 0001+-- 0045 int PHI 0029 0043+--+-- After the patch, the initialization is performed only+-- for the first member of the union, so now IR looks+-- like this:+-- 0029 ------ LOOP ------------+-- 0030 u8 XLOAD [0x1047c4521] V+-- 0031 int BAND 0030 +12+-- 0032 > int EQ 0031 +0+-- 0033 } cdt CNEW +96+-- 0034 p64 ADD 0033 +16+-- 0035 } u32 XSTORE 0034 0027 <--- `u` member init+-- 0036 u32 CONV 0027 u32.int+-- 0037 num CONV 0036 num.u32+-- 0038 num CONV 0027 num.int+-- 0039 > num EQ 0038 0037+-- 0040 + int ADD 0027 +1+-- 0041 > int LE 0040 0001+-- 0042 int PHI 0027 0040+jit.opt.start('hotloop=1')for i = 1, NITERATIONS dotest:ok(union_type(i).u == i)==============================================--
Best regards,Maxim KokryashkinHi, 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)