<HTML><BODY><div>Hi!</div><div>Thanks for the review!</div><div>Added a comment with the test description, the branch is force-pushed.</div><div>Here is the diff for the test description:</div><div>==============================================</div><div><div><div>diff --git a/test/tarantool-tests/lj-128-fix-union-init.test.lua b/test/tarantool-tests/lj-128-fix-union-init.test.lua</div><div>index 6a49cec8..65ba0f28 100644</div><div>--- a/test/tarantool-tests/lj-128-fix-union-init.test.lua</div><div>+++ b/test/tarantool-tests/lj-128-fix-union-init.test.lua</div><div>@@ -10,6 +10,46 @@ test:plan(NITERATIONS)</div><div> local ffi = require('ffi')</div><div> local union_type = ffi.typeof('union { uint32_t u; float f; }')</div><div> </div><div>+-- Before the patch, the `union_type` call resulted in the</div><div>+-- initialization of both the integer and the float members</div><div>+-- of the union, leading to undefined behavior since the</div><div>+-- integer member was overwritten.</div><div>+-- The IR was the following:</div><div>+--</div><div>+-- 0031 ------ LOOP ------------</div><div>+-- 0032 u8 XLOAD [0x100684521] V</div><div>+-- 0033 int BAND 0032 +12</div><div>+-- 0034 > int EQ 0033 +0</div><div>+-- 0035 > cdt CNEW +96</div><div>+-- 0036 p64 ADD 0035 +16</div><div>+-- 0037 u32 XSTORE 0036 0029 <--- `u` member init</div><div>+-- 0038 flt XSTORE 0036 0022 <--- `f` member init</div><div>+-- 0039 u32 XLOAD 0036</div><div>+-- 0040 num CONV 0039 num.u32</div><div>+-- 0041 num CONV 0029 num.int</div><div>+-- 0042 > num EQ 0041 0040</div><div>+-- 0043 + int ADD 0029 +1</div><div>+-- 0044 > int LE 0043 0001</div><div>+-- 0045 int PHI 0029 0043</div><div>+--</div><div>+-- After the patch, the initialization is performed only</div><div>+-- for the first member of the union, so now IR looks</div><div>+-- like this:</div><div>+-- 0029 ------ LOOP ------------</div><div>+-- 0030 u8 XLOAD [0x1047c4521] V</div><div>+-- 0031 int BAND 0030 +12</div><div>+-- 0032 > int EQ 0031 +0</div><div>+-- 0033 } cdt CNEW +96</div><div>+-- 0034 p64 ADD 0033 +16</div><div>+-- 0035 } u32 XSTORE 0034 0027 <--- `u` member init</div><div>+-- 0036 u32 CONV 0027 u32.int</div><div>+-- 0037 num CONV 0036 num.u32</div><div>+-- 0038 num CONV 0027 num.int</div><div>+-- 0039 > num EQ 0038 0037</div><div>+-- 0040 + int ADD 0027 +1</div><div>+-- 0041 > int LE 0040 0001</div><div>+-- 0042 int PHI 0027 0040</div><div>+</div><div> jit.opt.start('hotloop=1')</div><div> for i = 1, NITERATIONS do</div><div> test:ok(union_type(i).u == i)</div><div>==============================================</div></div></div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16892403910134860424_BODY">Hi, Max!<br><br><br>thanks for the patch!<br><br>See my comments inline.<br><br><br>Sergey<br><br>On 7/10/23 18:56, Maksim Kokryashkin wrote:<br>> From: Mike Pall <mike><br>><br>> Thanks to Alex Shpilkin.<br>><br>> (cherry-picked from commit 56c04accf975bff2519c34721dccbbdb7b8e6963)<br>><br>> As stated here[1], only the first field of a union can be<br>> initialized with a flat initializer. However, before this<br>> patch, on-trace initialization instructions were emitted<br>> for other union members too, overwriting the previous<br>> initialization values.<br>><br>> This patch fixes the mentioned behavior by preventing<br>> initialization of members other than the first one.<br>><br>> [1]: <a href="https://luajit.org/ext_ffi_semantics.html#init" target="_blank">https://luajit.org/ext_ffi_semantics.html#init</a><br>><br>> Maxim Kokryashkin:<br>> * added the description and the test for the problem<br>><br>> Part of tarantool/tarantool#8825<br>> ---<br>> Branch: <a href="https://github.com/tarantool/luajit/tree/fckxorg/lj-128-fix-union-init" target="_blank">https://github.com/tarantool/luajit/tree/fckxorg/lj-128-fix-union-init</a><br>> PR: <a href="https://github.com/tarantool/tarantool/pull/8867" target="_blank">https://github.com/tarantool/tarantool/pull/8867</a><br>> Original LuaJIT PR: <a href="https://github.com/LuaJIT/LuaJIT/pull/650" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/650</a><br>><br>> src/lj_crecord.c | 5 +++++<br>> .../lj-128-fix-union-init.test.lua | 18 ++++++++++++++++++<br>> 2 files changed, 23 insertions(+)<br>> create mode 100644 test/tarantool-tests/lj-128-fix-union-init.test.lua<br>><br>> diff --git a/src/lj_crecord.c b/src/lj_crecord.c<br>> index 0008a865..ffe995f4 100644<br>> --- a/src/lj_crecord.c<br>> +++ b/src/lj_crecord.c<br>> @@ -1065,6 +1065,11 @@ static void crec_alloc(jit_State *J, RecordFFData *rd, CTypeID id)<br>> dp = emitir(IRT(IR_ADD, IRT_PTR), trcd,<br>> lj_ir_kintp(J, df->size + sizeof(GCcdata)));<br>> crec_ct_tv(J, dc, dp, sp, sval);<br>> + if ((d->info & CTF_UNION)) {<br>> + if (d->size != dc->size) /* NYI: partial init of union. */<br>> + lj_trace_err(J, LJ_TRERR_NYICONV);<br>> + break;<br>> + }<br>> } else if (!ctype_isconstval(df->info)) {<br>> /* NYI: init bitfields and sub-structures. */<br>> lj_trace_err(J, LJ_TRERR_NYICONV);<br>> diff --git a/test/tarantool-tests/lj-128-fix-union-init.test.lua b/test/tarantool-tests/lj-128-fix-union-init.test.lua<br>> new file mode 100644<br>> index 00000000..6a49cec8<br>> --- /dev/null<br>> +++ b/test/tarantool-tests/lj-128-fix-union-init.test.lua<br>> @@ -0,0 +1,18 @@<br>> +local tap = require('tap')<br>> +local test = tap.test('lj-128-fix-union-init'):skipcond({<br>> + ['Test requires JIT enabled'] = not jit.status(),<br>> +})<br>> +<br>> +local NITERATIONS = 4<br>> +<br>> +test:plan(NITERATIONS)<br>> +<br>> +local ffi = require('ffi')<br>> +local union_type = ffi.typeof('union { uint32_t u; float f; }')<br>> +<br>> +jit.opt.start('hotloop=1')<br>> +for i = 1, NITERATIONS do<br>> + test:ok(union_type(i).u == i)<br>testcases description is missed, please add one.<br>> +end<br>> +<br>> +os.exit(test:check() and 0 or 1)</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>