[Tarantool-patches] [PATCH luajit] FFI: Fix recording of union initialization.

[Maxim Kokryashkin]

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)
==============================================
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, 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)
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20230714/792b8ebf/attachment.htm>