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.luaindex 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')endos.exit(test:check() and 0 or 1)==============================================The branch is force-pushed.> ==============================================
<snipped>
> >>> +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