<HTML><BODY><div>Hi!</div><div>Thanks for the review!</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_16894206930266288622_BODY">Hi, Max!<br>Thanks for the fixes!<br><br>LGTM, with a minor reminder below.<br>I suppose, that Sergey concerned about test name in the `test:ok()`<br>routine.<br><br>On 14.07.23, Maxim Kokryashkin wrote:<br>><br>> Hi!<br>> Thanks for the review!<br>> Added a comment with the test description, the branch is force-pushed.<br>> Here is the diff for the test description:<br>> ==============================================<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>> index 6a49cec8..65ba0f28 100644<br>> --- a/test/tarantool-tests/lj-128-fix-union-init.test.lua<br>> +++ b/test/tarantool-tests/lj-128-fix-union-init.test.lua<br>> @@ -10,6 +10,46 @@ test:plan(NITERATIONS)<br>> local ffi = require('ffi')<br>> local union_type = ffi.typeof('union { uint32_t u; float f; }')<br>> <br>> +-- Before the patch, the `union_type` call resulted in the<br>> +-- initialization of both the integer and the float members<br>> +-- of the union, leading to undefined behavior since the<br>> +-- integer member was overwritten.<br>> +-- The IR was the following:<br>> +--<br>> +-- 0031 ------ LOOP ------------<br>> +-- 0032 u8 XLOAD [0x100684521] V<br>> +-- 0033 int BAND 0032 +12<br>> +-- 0034 > int EQ 0033 +0<br>> +-- 0035 > cdt CNEW +96<br>> +-- 0036 p64 ADD 0035 +16<br>> +-- 0037 u32 XSTORE 0036 0029 <--- `u` member init<br>> +-- 0038 flt XSTORE 0036 0022 <--- `f` member init<br>> +-- 0039 u32 XLOAD 0036<br>> +-- 0040 num CONV 0039 num.u32<br>> +-- 0041 num CONV 0029 num.int<br>> +-- 0042 > num EQ 0041 0040<br>> +-- 0043 + int ADD 0029 +1<br>> +-- 0044 > int LE 0043 0001<br>> +-- 0045 int PHI 0029 0043<br>> +--<br>> +-- After the patch, the initialization is performed only<br>> +-- for the first member of the union, so now IR looks<br>> +-- like this:<br>> +-- 0029 ------ LOOP ------------<br>> +-- 0030 u8 XLOAD [0x1047c4521] V<br>> +-- 0031 int BAND 0030 +12<br>> +-- 0032 > int EQ 0031 +0<br>> +-- 0033 } cdt CNEW +96<br>> +-- 0034 p64 ADD 0033 +16<br>> +-- 0035 } u32 XSTORE 0034 0027 <--- `u` member init<br>> +-- 0036 u32 CONV 0027 u32.int<br>> +-- 0037 num CONV 0036 num.u32<br>> +-- 0038 num CONV 0027 num.int<br>> +-- 0039 > num EQ 0038 0037<br>> +-- 0040 + int ADD 0027 +1<br>> +-- 0041 > int LE 0040 0001<br>> +-- 0042 int PHI 0027 0040<br>> +<br>> jit.opt.start('hotloop=1')<br>> for i = 1, NITERATIONS do<br>> test:ok(union_type(i).u == i)<br><br>Minor: the description of the test is still missing.</div></div></div></div></blockquote><div>Fixed! Here is the diff:</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 65ba0f28..93cbf3af 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>@@ -52,7 +52,7 @@ local union_type = ffi.typeof('union { uint32_t u; float f; }')</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>+ test:ok(union_type(i).u == i, 'first member init only')</div><div> end</div><div> </div><div> os.exit(test:check() and 0 or 1)</div><div>==============================================</div><div> </div><div>The branch is force-pushed.</div></div></div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>> ==============================================<br><br><snipped><br><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)<br>> > <br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>