Hi! Thanks for the review! >  >>Hi, Max! >>Thanks for the fixes! >> >>LGTM, with a minor reminder below. >>I suppose, that Sergey concerned about test name in the `test:ok()` >>routine. >> >>On 14.07.23, Maxim Kokryashkin wrote: >>> >>> 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.lua >>> index 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 do >>>    test:ok(union_type(i).u == i) >> >>Minor: the description of the test is still missing. >Fixed! Here is the diff: >============================================== >diff --git a/test/tarantool-tests/lj-128-fix-union-init.test.lua b/test/tarantool-tests/lj-128-fix-union-init.test.lua >index 65ba0f28..93cbf3af 100644 >--- a/test/tarantool-tests/lj-128-fix-union-init.test.lua >+++ b/test/tarantool-tests/lj-128-fix-union-init.test.lua >@@ -52,7 +52,7 @@ 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) >+  test:ok(union_type(i).u == i, 'first member init only') > end >  > os.exit(test:check() and 0 or 1) >============================================== >  >The branch is force-pushed. >>> ============================================== >> >> >> >>> >>> +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) >>> >  >> >>-- >>Best regards, >>Sergey Kaplun >-- >Best regards, >Maxim Kokryashkin