From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E73ED52F276; Tue, 18 Jul 2023 18:11:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E73ED52F276 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1689693116; bh=qiPkCi4Oz34WZIjY/5KxmRst/8fyUsE0A07SZjSOLrI=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=JN+hcrM4xfwOYGvhddc2Ql0i0JdFWmf0zS7UQLWfKGNpYB6Mg+fwaqB4F9KxxUo+F 8stILUZj9Bj3DANOmk9XiLTWbZlAxjdeIPLrasdzI69F2aqXAEbVQjmpDXE3WxGSc4 pjaFYbxXdSnNBKZReZwAVvMed4L4BWy08diyXAqU= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [95.163.41.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0270A52ED7D for ; Tue, 18 Jul 2023 18:11:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0270A52ED7D Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1qLmMj-00HHyn-SE; Tue, 18 Jul 2023 18:11:54 +0300 Content-Type: multipart/alternative; boundary="------------eosHbsVdmxh0xCiy0ol9dK01" Message-ID: Date: Tue, 18 Jul 2023 18:11:52 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 To: Maxim Kokryashkin References: <20230710155624.70639-1-max.kokryashkin@gmail.com> <1689346625.351693821@f380.i.mail.ru> Content-Language: en-US In-Reply-To: <1689346625.351693821@f380.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD97569A0FE902DCB3DE03C3B2C77D958899BDBCA020D4EBE681867C24CE74E72BB5FE18E9CBFBE7C07654F187004E3DC468650E55D724367AD170ABBCC58F3F0AF96BFE15B8ADB4371 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70CB15FA6C489297DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B8CD1D12629438CE8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8FEC9C7F036DF3524F401805ABE1647C8117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B0F49EF363AAD6E82A471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC57C18602FF70A7313AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F79006374B64FF5809E74304D81D268191BDAD3D3666184CF4C3C14F3FC91FA280E0CE3D1A620F70A64A45A98AA50765F7900637AC4ECE65C2AF15826D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637C970FD8DF19C51D2EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5F1CAFAE11FDDB50776D8BB708BB5DD8514A5B20C8C156AEDF87CCE6106E1FC07E67D4AC08A07B9B0A816C540FC8EEC30CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF4B6646E9A7B18C771C99183D204F3C63B20C9D73408B38A6A10F18F048CEAF42614F3626C4CAF433DA6E887A44188BD6C2178726025F5BE841666BDD36078562A74DFFEFA5DC0E7F02C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojLxQD/rvAZi5K5JnwyCrsGA== X-Mailru-Sender: 0A26D9779F8DDEABEA155B740E6E76705EA77C1F21DD786DD393A50AC8D21B4A2A0E4F39E0275F47645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] FFI: Fix recording of union initialization. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: Maksim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------eosHbsVdmxh0xCiy0ol9dK01 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > > > 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) > --------------eosHbsVdmxh0xCiy0ol9dK01 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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