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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- 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 --]