From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <tarantool-patches-bounces@dev.tarantool.org> Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id D6C65D82141; Tue, 18 Jun 2024 13:44:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D6C65D82141 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1718707477; bh=QI7SS+J1mTy2arfJLTj9nfmlCWbQxfbDQf7OzP1Z33A=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Z4cCah3UKhvUH30+bdIDCnjnMbfmLM7M4Yxn04AtGtCY0XqMJa8R4k9zi8eS8GPOs IwfXFjSYqc1Q9b8TlqA8V1yeAro3ftKQUJBRnNSmmRfhNP5K4Km7ohTRwJZRGH2aPD A7QQHZ82Xbfxi3y6xQ4bKcu//232ktEYcC6vpVUs= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [95.163.41.65]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 54F96D82141 for <tarantool-patches@dev.tarantool.org>; Tue, 18 Jun 2024 13:44:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 54F96D82141 Received: by exim-smtp-dbbd7b44b-7mms2 with esmtpa (envelope-from <sergeyb@tarantool.org>) id 1sJWKI-00000000MBZ-1pyI; Tue, 18 Jun 2024 13:44:34 +0300 Content-Type: multipart/alternative; boundary="------------J1AMuSKa0mQHW2o0cGYkQP7B" Message-ID: <3d7669c8-0ad9-4714-9093-5a8213442124@tarantool.org> Date: Tue, 18 Jun 2024 13:44:34 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun <skaplun@tarantool.org>, Maxim Kokryashkin <m.kokryashkin@tarantool.org> Cc: tarantool-patches@dev.tarantool.org References: <20240424103720.9464-1-skaplun@tarantool.org> In-Reply-To: <20240424103720.9464-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9EB52AADBDB11C14927CFFAF1C6212F6AB55D068E3B3A9FE4182A05F53808504025D77450BD84A5CE3DE06ABAFEAF6705298650EA9C3A7C78E86B39BB833B052C209524BCEF20D7BA X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE746D93DAA4671895CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372521E7C1CE72986C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89E5830A6CD46D20C687B43258BEF969F7D62C68954EC0B29CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC81D471462564A2E19F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2D01283D1ACF37BAC0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C36E36DCD5FF651F90BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7AEA1580DED4E70E3731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A56198F92094C375CF5002B1117B3ED6968E4843F94AE410A4A13BD6A4B0E00B96823CB91A9FED034534781492E4B8EEAD220496FFA5CD4785BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFEDDB41815C847BE8274E475DE122FA7A682BFA313192C5FACBC8ABE51650C671110A92F5D16EB67F1521D91D49C923C7EB2B3DB2ECEBFC710DCC4675CE65115BBC32F018DFB5CBD85F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJvJpr60SBvLb4jlVc97XpA== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140B6D03DEB629BE434EAE04967732FC4582272F5BDF971F09D0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Check for IR_HREF vs. IR_HREFK aliasing in non-nil store check. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org> List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe> List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/> List-Post: <mailto:tarantool-patches@dev.tarantool.org> List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help> List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe> From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> Reply-To: Sergey Bronnikov <sergeyb@tarantool.org> Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" <tarantool-patches-bounces@dev.tarantool.org> This is a multi-part message in MIME format. --------------J1AMuSKa0mQHW2o0cGYkQP7B Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey thanks for the patch! LGTM On 24.04.2024 13:37, Sergey Kaplun wrote: > From: Mike Pall <mike> > > Thanks to Peter Cawley. > > (cherry picked from commit 658530562c2ac7ffa8e4ca5d18856857471244e9) > > The `lj_opt_fwd_wasnonnil()` skips the check for HREF and HREFK that may > alias. Hence, the guard for the non-nil value may be skipped, and the > `__newindex` metamethod call is omitted too. > > This patch adds the aforementioned check for different reference types > (HREF vs. HREFK), which were not detected by the previous analysis. > Also, the helper macro `irt_isp32()` is introduced to check that the IR > type is `IRT_P32` (KSLOT type). > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#9924 > --- > > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1133-fwd-href-hrefk-alias > Related issues: > *https://github.com/tarantool/tarantool/issues/9924 > *https://github.com/LuaJIT/LuaJIT/issues/1133 > > src/lj_ir.h | 1 + > src/lj_opt_mem.c | 2 + > .../lj-1133-fwd-href-hrefk-alias.test.lua | 94 +++++++++++++++++++ > 3 files changed, 97 insertions(+) > create mode 100644 test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua > > diff --git a/src/lj_ir.h b/src/lj_ir.h > index e9b8328e..27c66f63 100644 > --- a/src/lj_ir.h > +++ b/src/lj_ir.h > @@ -367,6 +367,7 @@ typedef struct IRType1 { uint8_t irt; } IRType1; > #define irt_isu32(t) (irt_type(t) == IRT_U32) > #define irt_isi64(t) (irt_type(t) == IRT_I64) > #define irt_isu64(t) (irt_type(t) == IRT_U64) > +#define irt_isp32(t) (irt_type(t) == IRT_P32) > > #define irt_isfp(t) (irt_isnum(t) || irt_isfloat(t)) > #define irt_isinteger(t) (irt_typerange((t), IRT_I8, IRT_INT)) > diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c > index 9044f09a..c9f1216c 100644 > --- a/src/lj_opt_mem.c > +++ b/src/lj_opt_mem.c > @@ -916,6 +916,8 @@ int lj_opt_fwd_wasnonnil(jit_State *J, IROpT loadop, IRRef xref) > if (skref == xkref || !irref_isk(skref) || !irref_isk(xkref)) > return 0; /* A nil store with same const key or var key MAY alias. */ > /* Different const keys CANNOT alias. */ > + } else if (irt_isp32(IR(skref)->t) != irt_isp32(IR(xkref)->t)) { > + return 0; /* HREF and HREFK MAY alias. */ > } /* Different key types CANNOT alias. */ > } /* Other non-nil stores MAY alias. */ > ref = store->prev; > diff --git a/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua > new file mode 100644 > index 00000000..6b72c97a > --- /dev/null > +++ b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua > @@ -0,0 +1,94 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate the LuaJIT's incorrect aliasing check > +-- for HREFK and HREF IRs during the non-nil check. > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1133. > + > +local test = tap.test('lj-1133-fwd-href-hrefk-alias'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > +test:plan(1) > + > +local rawset = rawset > + > +-- The maximum value that can be stored in a 16-bit `op2` > +-- field in HREFK IR. > +local HASH_NODES = 65535 > + > +-- Amount of iteration to compile and execute the trace. > +local LOOP_LIMIT = 4 > + > +-- Function to be called twice to emit the trace and take the side > +-- exit. > +local function trace_aliased_tables(t1, t2) > + -- The criteria is the number of new index creations. > + local count = 0 > + local mt = {__newindex = function(t, k, v) > + count = count + 1 > + rawset(t, k, v) > + end} > + setmetatable(t1, mt) > + setmetatable(t2, mt) > + > + for _ = 1, LOOP_LIMIT do > + -- XXX: Keys values have no special meaning here, just be sure > + -- that they are HREF/HREFK and not in the array table part. > + -- `t1` is empty, emitting HREFK. > + t1[10] = 1 > + -- `t2` on recording has more than `HASH_NODES` table nodes, > + -- so this emits HREF. > + t2[10] = nil > + -- Resolve `__newindex` if t1 == t2. > + -- `lj_opt_fwd_wasnonnil()` missed the check that HREFK and > + -- HREF may alias before the patch, so the guarded HLOAD IR > + -- with the corresponding snapshot is skipped. > + -- The difference in the emitted IR before and afterthe patch > + -- is the following: > + -- | 0004 > tab SLOAD #1 T > + -- | ... > + -- | 0009 p32 FLOAD 0004 tab.node > + -- | 0010 > p32 HREFK 0009 +10 @0 > + -- | 0011 > num HLOAD 0010 > + -- | 0012 num HSTORE 0010 +1 > + -- | .... SNAP #1 > + -- | 0013 > tab SLOAD #2 T > + -- | 0014 int FLOAD 0013 tab.asize > + -- | 0015 > int ULE 0014 +10 > + -- | 0016 p32 HREF 0013 +10 > + -- | 0017 > p32 NE 0016 [0x415554e8] > + -- | 0018 > num HLOAD 0016 > + -- | 0019 nil HSTORE 0016 nil > + -- | -0020 num HSTORE 0010 +30 > + -- | .... SNAP #2 > + -- | +0020 > num HLOAD 0010 > + -- | +0021 num HSTORE 0010 +30 > + -- | +.... SNAP #3 > + -- > + -- Hence, the taken exit is not resolving `__newindex` before > + -- the patch. > + t1[10] = 1 > + -- The exit 2 of the trace is here. > + -- Resolve `__newindex` if t1 ~= t2. > + t2[10] = 1 > + end > + -- `__newindex` is called twice on the first iteration and once > + -- on each other. > + return count == LOOP_LIMIT + 1 > +end > + > +-- Create a big table to emit HREF IR (not HREFK) to trick > +-- the alias checking. > +local bigt = {} > +for i = 1, HASH_NODES + 1 do > + bigt[-i] = true > +end > + > +jit.opt.start('hotloop=1') > + > +trace_aliased_tables({}, bigt) > + > +-- Now use tables that are aliased. > +local smallt = {} > +test:ok(trace_aliased_tables(smallt, smallt), 'aliasing check is correct') > + > +test:done(true) --------------J1AMuSKa0mQHW2o0cGYkQP7B Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit <!DOCTYPE html> <html data-lt-installed="true"> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body style="padding-bottom: 1px;"> <p>Hi, Sergey</p> <p>thanks for the patch! LGTM<br> </p> <div class="moz-cite-prefix">On 24.04.2024 13:37, Sergey Kaplun wrote:<br> </div> <blockquote type="cite" cite="mid:20240424103720.9464-1-skaplun@tarantool.org"> <pre class="moz-quote-pre" wrap="">From: Mike Pall <mike> Thanks to Peter Cawley. (cherry picked from commit 658530562c2ac7ffa8e4ca5d18856857471244e9) The `lj_opt_fwd_wasnonnil()` skips the check for HREF and HREFK that may alias. Hence, the guard for the non-nil value may be skipped, and the `__newindex` metamethod call is omitted too. This patch adds the aforementioned check for different reference types (HREF vs. HREFK), which were not detected by the previous analysis. Also, the helper macro `irt_isp32()` is introduced to check that the IR type is `IRT_P32` (KSLOT type). Sergey Kaplun: * added the description and the test for the problem Part of tarantool/tarantool#9924 --- Branch: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/skaplun/lj-1133-fwd-href-hrefk-alias">https://github.com/tarantool/luajit/tree/skaplun/lj-1133-fwd-href-hrefk-alias</a> Related issues: * <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/9924">https://github.com/tarantool/tarantool/issues/9924</a> * <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1133">https://github.com/LuaJIT/LuaJIT/issues/1133</a> src/lj_ir.h | 1 + src/lj_opt_mem.c | 2 + .../lj-1133-fwd-href-hrefk-alias.test.lua | 94 +++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua diff --git a/src/lj_ir.h b/src/lj_ir.h index e9b8328e..27c66f63 100644 --- a/src/lj_ir.h +++ b/src/lj_ir.h @@ -367,6 +367,7 @@ typedef struct IRType1 { uint8_t irt; } IRType1; #define irt_isu32(t) (irt_type(t) == IRT_U32) #define irt_isi64(t) (irt_type(t) == IRT_I64) #define irt_isu64(t) (irt_type(t) == IRT_U64) +#define irt_isp32(t) (irt_type(t) == IRT_P32) #define irt_isfp(t) (irt_isnum(t) || irt_isfloat(t)) #define irt_isinteger(t) (irt_typerange((t), IRT_I8, IRT_INT)) diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c index 9044f09a..c9f1216c 100644 --- a/src/lj_opt_mem.c +++ b/src/lj_opt_mem.c @@ -916,6 +916,8 @@ int lj_opt_fwd_wasnonnil(jit_State *J, IROpT loadop, IRRef xref) if (skref == xkref || !irref_isk(skref) || !irref_isk(xkref)) return 0; /* A nil store with same const key or var key MAY alias. */ /* Different const keys CANNOT alias. */ + } else if (irt_isp32(IR(skref)->t) != irt_isp32(IR(xkref)->t)) { + return 0; /* HREF and HREFK MAY alias. */ } /* Different key types CANNOT alias. */ } /* Other non-nil stores MAY alias. */ ref = store->prev; diff --git a/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua new file mode 100644 index 00000000..6b72c97a --- /dev/null +++ b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua @@ -0,0 +1,94 @@ +local tap = require('tap') + +-- Test file to demonstrate the LuaJIT's incorrect aliasing check +-- for HREFK and HREF IRs during the non-nil check. +-- See also: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1133">https://github.com/LuaJIT/LuaJIT/issues/1133</a>. + +local test = tap.test('lj-1133-fwd-href-hrefk-alias'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) +test:plan(1) + +local rawset = rawset + +-- The maximum value that can be stored in a 16-bit `op2` +-- field in HREFK IR. +local HASH_NODES = 65535 + +-- Amount of iteration to compile and execute the trace. +local LOOP_LIMIT = 4 + +-- Function to be called twice to emit the trace and take the side +-- exit. +local function trace_aliased_tables(t1, t2) + -- The criteria is the number of new index creations. + local count = 0 + local mt = {__newindex = function(t, k, v) + count = count + 1 + rawset(t, k, v) + end} + setmetatable(t1, mt) + setmetatable(t2, mt) + + for _ = 1, LOOP_LIMIT do + -- XXX: Keys values have no special meaning here, just be sure + -- that they are HREF/HREFK and not in the array table part. + -- `t1` is empty, emitting HREFK. + t1[10] = 1 + -- `t2` on recording has more than `HASH_NODES` table nodes, + -- so this emits HREF. + t2[10] = nil + -- Resolve `__newindex` if t1 == t2. + -- `lj_opt_fwd_wasnonnil()` missed the check that HREFK and + -- HREF may alias before the patch, so the guarded HLOAD IR + -- with the corresponding snapshot is skipped. + -- The difference in the emitted IR before and afterthe patch + -- is the following: + -- | 0004 > tab SLOAD #1 T + -- | ... + -- | 0009 p32 FLOAD 0004 tab.node + -- | 0010 > p32 HREFK 0009 +10 @0 + -- | 0011 > num HLOAD 0010 + -- | 0012 num HSTORE 0010 +1 + -- | .... SNAP #1 + -- | 0013 > tab SLOAD #2 T + -- | 0014 int FLOAD 0013 tab.asize + -- | 0015 > int ULE 0014 +10 + -- | 0016 p32 HREF 0013 +10 + -- | 0017 > p32 NE 0016 [0x415554e8] + -- | 0018 > num HLOAD 0016 + -- | 0019 nil HSTORE 0016 nil + -- | -0020 num HSTORE 0010 +30 + -- | .... SNAP #2 + -- | +0020 > num HLOAD 0010 + -- | +0021 num HSTORE 0010 +30 + -- | +.... SNAP #3 + -- + -- Hence, the taken exit is not resolving `__newindex` before + -- the patch. + t1[10] = 1 + -- The exit 2 of the trace is here. + -- Resolve `__newindex` if t1 ~= t2. + t2[10] = 1 + end + -- `__newindex` is called twice on the first iteration and once + -- on each other. + return count == LOOP_LIMIT + 1 +end + +-- Create a big table to emit HREF IR (not HREFK) to trick +-- the alias checking. +local bigt = {} +for i = 1, HASH_NODES + 1 do + bigt[-i] = true +end + +jit.opt.start('hotloop=1') + +trace_aliased_tables({}, bigt) + +-- Now use tables that are aliased. +local smallt = {} +test:ok(trace_aliased_tables(smallt, smallt), 'aliasing check is correct') + +test:done(true) </pre> </blockquote> </body> <lt-container></lt-container> </html> --------------J1AMuSKa0mQHW2o0cGYkQP7B--