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

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

[Maksim Kokryashkin via Tarantool-patches]

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)
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.39.2 (Apple Git-143)

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

[Sergey Bronnikov via Tarantool-patches]

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)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maksim!
Thanks for the patch!
LGTM, after fixing the Sergey's comment regarding the test description.

On 13.07.23, Sergey Bronnikov wrote:
> 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.
> >

<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

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5080 bytes --]


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)
> 

[-- Attachment #2: Type: text/html, Size: 7299 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

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.

> ==============================================

<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

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3800 bytes --]


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.
>>> ==============================================
>>
>><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

[-- Attachment #2: Type: text/html, Size: 5411 bytes --]

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

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 6118 bytes --]

Hi, Max!

Thanks for your fixes and especially for detailed explanation in a test.

As Sergey said, my comment was addressed to  a missed testcase description.

Now I see that everything is fine.

Thanks, LGTM.


Sergey

On 7/14/23 17:57, 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)
> ==============================================
> --
> 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)
>

[-- Attachment #2: Type: text/html, Size: 11064 bytes --]

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

[Igor Munkin via Tarantool-patches]

Max,

Thanks for the patch! I've checked the patchset into all long-term
branches in tarantool/luajit and bumped a new version in master,
release/2.11 and release/2.10.

On 10.07.23, Maksim Kokryashkin via Tarantool-patches 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
> 

<snipped>

> -- 
> 2.39.2 (Apple Git-143)
> 

-- 
Best regards,
IM