<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi, Max!</p>
<p>Thanks for your fixes and especially for detailed explanation in
a test.</p>
<p>As Sergey said, my comment was addressed to a missed testcase
description.</p>
<p>Now I see that everything is fine.</p>
<p>Thanks, LGTM.</p>
<p><br>
</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 7/14/23 17:57, Maxim Kokryashkin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1689346625.351693821@f380.i.mail.ru">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div>Hi!</div>
<div>Thanks for the review!</div>
<div>Added a comment with the test description, the branch is
force-pushed.</div>
<div>Here is the diff for the test description:</div>
<div>==============================================</div>
<div>
<div>
<div>diff --git
a/test/tarantool-tests/lj-128-fix-union-init.test.lua
b/test/tarantool-tests/lj-128-fix-union-init.test.lua</div>
<div>index 6a49cec8..65ba0f28 100644</div>
<div>--- a/test/tarantool-tests/lj-128-fix-union-init.test.lua</div>
<div>+++ b/test/tarantool-tests/lj-128-fix-union-init.test.lua</div>
<div>@@ -10,6 +10,46 @@ test:plan(NITERATIONS)</div>
<div> local ffi = require('ffi')</div>
<div> local union_type = ffi.typeof('union { uint32_t u; float
f; }')</div>
<div> </div>
<div>+-- Before the patch, the `union_type` call resulted in
the</div>
<div>+-- initialization of both the integer and the float
members</div>
<div>+-- of the union, leading to undefined behavior since the</div>
<div>+-- integer member was overwritten.</div>
<div>+-- The IR was the following:</div>
<div>+--</div>
<div>+-- 0031 ------ LOOP ------------</div>
<div>+-- 0032 u8 XLOAD [0x100684521] V</div>
<div>+-- 0033 int BAND 0032 +12</div>
<div>+-- 0034 > int EQ 0033 +0</div>
<div>+-- 0035 > cdt CNEW +96</div>
<div>+-- 0036 p64 ADD 0035 +16</div>
<div>+-- 0037 u32 XSTORE 0036 0029 <--- `u` member init</div>
<div>+-- 0038 flt XSTORE 0036 0022 <--- `f` member init</div>
<div>+-- 0039 u32 XLOAD 0036</div>
<div>+-- 0040 num CONV 0039 num.u32</div>
<div>+-- 0041 num CONV 0029 num.int</div>
<div>+-- 0042 > num EQ 0041 0040</div>
<div>+-- 0043 + int ADD 0029 +1</div>
<div>+-- 0044 > int LE 0043 0001</div>
<div>+-- 0045 int PHI 0029 0043</div>
<div>+--</div>
<div>+-- After the patch, the initialization is performed only</div>
<div>+-- for the first member of the union, so now IR looks</div>
<div>+-- like this:</div>
<div>+-- 0029 ------ LOOP ------------</div>
<div>+-- 0030 u8 XLOAD [0x1047c4521] V</div>
<div>+-- 0031 int BAND 0030 +12</div>
<div>+-- 0032 > int EQ 0031 +0</div>
<div>+-- 0033 } cdt CNEW +96</div>
<div>+-- 0034 p64 ADD 0033 +16</div>
<div>+-- 0035 } u32 XSTORE 0034 0027 <--- `u` member init</div>
<div>+-- 0036 u32 CONV 0027 u32.int</div>
<div>+-- 0037 num CONV 0036 num.u32</div>
<div>+-- 0038 num CONV 0027 num.int</div>
<div>+-- 0039 > num EQ 0038 0037</div>
<div>+-- 0040 + int ADD 0027 +1</div>
<div>+-- 0041 > int LE 0040 0001</div>
<div>+-- 0042 int PHI 0027 0040</div>
<div>+</div>
<div> jit.opt.start('hotloop=1')</div>
<div> for i = 1, NITERATIONS do</div>
<div> test:ok(union_type(i).u == i)</div>
<div>==============================================</div>
</div>
</div>
<div data-signature-widget="container">
<div data-signature-widget="content">
<div>--<br>
Best regards,</div>
<div>Maxim Kokryashkin</div>
</div>
</div>
<div> </div>
<div> </div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div id="">
<div class="js-helper js-readmsg-msg">
<div>
<div id="style_16892403910134860424_BODY">Hi, Max!<br>
<br>
<br>
thanks for the patch!<br>
<br>
See my comments inline.<br>
<br>
<br>
Sergey<br>
<br>
On 7/10/23 18:56, Maksim Kokryashkin wrote:<br>
> From: Mike Pall <mike><br>
><br>
> Thanks to Alex Shpilkin.<br>
><br>
> (cherry-picked from commit
56c04accf975bff2519c34721dccbbdb7b8e6963)<br>
><br>
> As stated here[1], only the first field of a
union can be<br>
> initialized with a flat initializer. However,
before this<br>
> patch, on-trace initialization instructions
were emitted<br>
> for other union members too, overwriting the
previous<br>
> initialization values.<br>
><br>
> This patch fixes the mentioned behavior by
preventing<br>
> initialization of members other than the first
one.<br>
><br>
> [1]: <a
href="https://luajit.org/ext_ffi_semantics.html#init"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://luajit.org/ext_ffi_semantics.html#init</a><br>
><br>
> Maxim Kokryashkin:<br>
> * added the description and the test for the
problem<br>
><br>
> Part of tarantool/tarantool#8825<br>
> ---<br>
> Branch: <a
href="https://github.com/tarantool/luajit/tree/fckxorg/lj-128-fix-union-init"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://github.com/tarantool/luajit/tree/fckxorg/lj-128-fix-union-init</a><br>
> PR: <a
href="https://github.com/tarantool/tarantool/pull/8867"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://github.com/tarantool/tarantool/pull/8867</a><br>
> Original LuaJIT PR: <a
href="https://github.com/LuaJIT/LuaJIT/pull/650"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://github.com/LuaJIT/LuaJIT/pull/650</a><br>
><br>
> src/lj_crecord.c | 5 +++++<br>
> .../lj-128-fix-union-init.test.lua | 18
++++++++++++++++++<br>
> 2 files changed, 23 insertions(+)<br>
> create mode 100644
test/tarantool-tests/lj-128-fix-union-init.test.lua<br>
><br>
> diff --git a/src/lj_crecord.c
b/src/lj_crecord.c<br>
> index 0008a865..ffe995f4 100644<br>
> --- a/src/lj_crecord.c<br>
> +++ b/src/lj_crecord.c<br>
> @@ -1065,6 +1065,11 @@ static void
crec_alloc(jit_State *J, RecordFFData *rd, CTypeID
id)<br>
> dp = emitir(IRT(IR_ADD, IRT_PTR), trcd,<br>
> lj_ir_kintp(J, df->size + sizeof(GCcdata)));<br>
> crec_ct_tv(J, dc, dp, sp, sval);<br>
> + if ((d->info & CTF_UNION)) {<br>
> + if (d->size != dc->size) /* NYI:
partial init of union. */<br>
> + lj_trace_err(J, LJ_TRERR_NYICONV);<br>
> + break;<br>
> + }<br>
> } else if (!ctype_isconstval(df->info)) {<br>
> /* NYI: init bitfields and sub-structures. */<br>
> lj_trace_err(J, LJ_TRERR_NYICONV);<br>
> diff --git
a/test/tarantool-tests/lj-128-fix-union-init.test.lua
b/test/tarantool-tests/lj-128-fix-union-init.test.lua<br>
> new file mode 100644<br>
> index 00000000..6a49cec8<br>
> --- /dev/null<br>
> +++
b/test/tarantool-tests/lj-128-fix-union-init.test.lua<br>
> @@ -0,0 +1,18 @@<br>
> +local tap = require('tap')<br>
> +local test =
tap.test('lj-128-fix-union-init'):skipcond({<br>
> + ['Test requires JIT enabled'] = not
jit.status(),<br>
> +})<br>
> +<br>
> +local NITERATIONS = 4<br>
> +<br>
> +test:plan(NITERATIONS)<br>
> +<br>
> +local ffi = require('ffi')<br>
> +local union_type = ffi.typeof('union {
uint32_t u; float f; }')<br>
> +<br>
> +jit.opt.start('hotloop=1')<br>
> +for i = 1, NITERATIONS do<br>
> + test:ok(union_type(i).u == i)<br>
testcases description is missed, please add one.<br>
> +end<br>
> +<br>
> +os.exit(test:check() and 0 or 1)</div>
</div>
</div>
</div>
</blockquote>
<div> </div>
</div>
</blockquote>
</blockquote>
</body>
</html>