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 E62A55C2EEF; Thu, 17 Aug 2023 17:51:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E62A55C2EEF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692283892; bh=xMSXRZTrrXt09chlKiqENooGlWL2lADC4VzFoLfQJAk=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=FYadN69Yk1reBc5+VnsxsKIl9BY9ZlwboZR34OQW4fvo3jGvCo18va+jFMXW1VR5d CPI43jV2yir9HdC9pTkW9DlYK0sRQX6CUryQf6nnVbiT2vZhu31T7cugXTwVmlkYlW jBhPmmlNY/hldZuzjaztB9d+uZIqZuYW2+xx6TQQ= Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [95.163.41.94]) (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 351315C2EEF for ; Thu, 17 Aug 2023 17:51:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 351315C2EEF Received: by smtp56.i.mail.ru with esmtpa (envelope-from ) id 1qWeLQ-0000nI-2r; Thu, 17 Aug 2023 17:51:29 +0300 To: Maxim Kokryashkin , Sergey Bronnikov Date: Thu, 17 Aug 2023 17:46:41 +0300 Message-ID: <20230817144641.20088-1-skaplun@tarantool.org> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9700E0DCE2907754D9B1C46D604AD036F8866CBFA63B460F7182A05F538085040020F5A780D543F81614809AE38B8467525251CD986E3AD90D1D4045A2B921A16 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CA8E915ACC910FBDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CF20B9B7F5DD35A68638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C80A6E8B055194B66C3ADC5C04B8AED6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC4AFB60FD1831C04CA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18BDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE0AC5B80A05675ACD985B8ACC81218E19D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE3DA7BFA4571439BB2040F9FF01DFDA4A8C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407978DA827A17800CE7BD5E21D9D46740942DBA43225CD8A89F616AD31D0D18CD5C35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A571978463C5294827F536CFE48898A5C062F2D3617429C4BFF87CCE6106E1FC07E67D4AC08A07B9B00A6B3CD6EB70C818C79554A2A72441328621D336A7BC284946AD531847A6065A535571D14F44ED41 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742D3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFD4A005EFBB0C4527C51065D202080697E2F55FCFCF04FA21AA40C9020FF87007D2D1C8B3CBCE0EE824F7434E9608BF9F2E1BA011D6E43E42ED473FF1C8C3FD5AA74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojbL9S8ysBdXiooNktVJQJhjN+W37TSFMb X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769531AB7A8049E546A614809AE38B84675FA95C477BC5130F8DEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: [Tarantool-patches] [PATCH luajit] Fix TDUP load forwarding after table rehash. 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" From: Mike Pall Reported by Sergey Kaplun. (cherry-picked from commit c7db8255e1eb59f933fac7bc9322f0e4f8ddc6e6) After table rehashing number keys loaded via ALOAD may be placed in the hash part of the table. So, load forwarding analysis missed the corresponding stores like they are never existed. In such case, either we faced an assertion failure in `fwd_ahload()` due to values types mismatch, either we faced an assertion failure in `rec_check_slots()` since forwarded value by the JIT compiler isn't the same as it is in the interpreter. This patch adds a check that there is no any `IR_NEWREF` after table creation, so it can't be rehashed. Sergey Kaplun: * added the description and the test for the problem Part of tarantool/tarantool#8825 --- Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-980-load-fwd-after-table-rehash PR: https://github.com/tarantool/tarantool/pull/8998 Related issues: * https://github.com/LuaJIT/LuaJIT/issues/980 * https://github.com/tarantool/tarantool/issues/8825 src/lj_opt_mem.c | 6 + ...j-980-load-fwd-after-table-rehash.test.lua | 166 ++++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100644 test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c index 59fddbdd..7b610506 100644 --- a/src/lj_opt_mem.c +++ b/src/lj_opt_mem.c @@ -157,6 +157,7 @@ static TRef fwd_ahload(jit_State *J, IRRef xref) if (ir->o == IR_TNEW || (ir->o == IR_TDUP && irref_isk(xr->op2))) { /* A NEWREF with a number key may end up pointing to the array part. ** But it's referenced from HSTORE and not found in the ASTORE chain. + ** Or a NEWREF may rehash the table and move unrelated number keys. ** For now simply consider this a conflict without forwarding anything. */ if (xr->o == IR_AREF) { @@ -167,6 +168,11 @@ static TRef fwd_ahload(jit_State *J, IRRef xref) goto cselim; ref2 = newref->prev; } + } else { + IRIns *key = IR(xr->op2); + if (key->o == IR_KSLOT) key = IR(key->op1); + if (irt_isnum(key->t) && J->chain[IR_NEWREF] > tab) + goto cselim; } /* NEWREF inhibits CSE for HREF, and dependent FLOADs from HREFK/AREF. ** But the above search for conflicting stores was limited by xref. diff --git a/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua new file mode 100644 index 00000000..a27932df --- /dev/null +++ b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua @@ -0,0 +1,166 @@ +local tap = require('tap') + +-- Test file to demonstrate LuaJIT misbehaviour during load +-- forwarding optimization for HLOAD after table rehashing. +-- See also https://github.com/LuaJIT/LuaJIT/issues/980. + +local test = tap.test('lj-980-load-fwd-after-table-rehash'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +test:plan(6) + +jit.opt.start('hotloop=1') + +local result +-- The test for TNEW load forwarding. It doesn't trigger an assert +-- since the commit "Fix TNEW load forwarding with instable +-- types.". But still add it to avoid regressions in future. +for i = 6, 9 do + -- Need big enough table to see rehashing. + -- Also, to simplify logic with AREF, HREF don't use default + -- 1, 4 (start, stop) values here. + local t = {i, i, i, i, i, i, i} + -- Insert via ASTORE. + t[i] = i + t[1] = nil + t[2] = nil + t[3] = nil + t[4] = nil + t[5] = nil + -- Rehash table. Array part is empty. + t['1000'] = 1000 + -- Load via HLOAD. + result = t[i] +end + +test:is(result, 9, 'TNEW load forwarding') + +-- TNEW load forwarding, aliased table. +local alias_store = {{}, {}, {}, {}, {}} +for i = 6, 9 do + local t = {i, i, i, i, i, i, i} + alias_store[#alias_store + 1] = t + local alias = alias_store[i] + -- Insert via ASTORE. + alias[i] = i + alias[1] = nil + alias[2] = nil + alias[3] = nil + alias[4] = nil + alias[5] = nil + -- Rehash table. Array part is empty. + alias['1000'] = 1000 + -- Load via HLOAD. + result = t[i] +end + +test:is(result, 9, 'TNEW load forwarding, aliased table') + +local expected = 'result' + +-- TDUP different types. +for i = 6, 9 do + local t = {1, 2, 3, 4, 5, 6, 7, 8} + t[i] = expected + t[i + 1] = expected + t[1] = nil + t[2] = nil + t[3] = nil + t[4] = nil + t[5] = nil + t[6] = nil + -- Rehash table. Array part is empty. + t['1000'] = 1000 + -- Result on the recording (i == 8) iteration is 'result'. + -- Nevertheless, on the last (i == 9) iteration it is 8. + -- Just check that there is no assert failure here. + -- Load via HLOAD. + result = t[8] +end + +-- Checked for assertion guard, on the last iteration we get +-- the value on initializatoin. +test:is(result, 8, 'TDUP load forwarding different types') + +-- TDUP different types, aliased table. +alias_store = {{}, {}, {}, {}, {}} +for i = 6, 9 do + local t = {1, 2, 3, 4, 5, 6, 7, 8} + -- Store table, to be aliased later. + alias_store[#alias_store + 1] = t + local alias = alias_store[i] + alias[i] = expected + alias[i + 1] = expected + alias[1] = nil + alias[2] = nil + alias[3] = nil + alias[4] = nil + alias[5] = nil + alias[6] = nil + -- Rehash table. Array part is empty. + alias['1000'] = 1000 + -- Result on the recording (i == 8) iteration is 'result'. + -- Nevertheless, on the last (i == 9) iteration it is 8. + -- Just check that there is no assert failure here. + -- Load via HLOAD. + result = t[8] +end + +-- Checked for assertion guard, on the last iteration we get +-- the value on initializatoin. +test:is(result, 8, 'TDUP load forwarding different types, aliased table') + +-- TDUP same type, different values. +for i = 6, 9 do + local t = {1, 2, 3, 4, 5, 6, '7', '8'} + t[i] = expected + t[i + 1] = expected + t[1] = nil + t[2] = nil + t[3] = nil + t[4] = nil + t[5] = nil + t[6] = nil + -- Rehash table. Array part is empty. + t['1000'] = 1000 + -- Result on the recording (i == 8) iteration is 'result'. + -- Nevertheless, on the last (i == 9) iteration it is '8'. + -- Just check that there is no assert failure here. + -- Load via HLOAD. + result = t[8] +end + +-- Checked for assertion guard, on the last iteration we get +-- the value on initializatoin. +test:is(result, '8', 'TDUP load forwarding same type, different values') + +alias_store = {{}, {}, {}, {}, {}} +for i = 6, 9 do + local t = {1, 2, 3, 4, 5, 6, '7', '8'} + -- Store table, to be aliased later. + alias_store[#alias_store + 1] = t + local alias = alias_store[i] + alias[i] = expected + alias[i + 1] = expected + alias[1] = nil + alias[2] = nil + alias[3] = nil + alias[4] = nil + alias[5] = nil + alias[6] = nil + -- Rehash table. Array part is empty. + alias['1000'] = 1000 + -- Result on the recording (i == 8) iteration is 'result'. + -- Nevertheless, on the last (i == 9) iteration it is '8'. + -- Just check that there is no assert failure here. + -- Load via HLOAD. + result = t[8] +end + +-- Checked for assertion guard, on the last iteration we get +-- the value on initializatoin. +test:is(result, '8', + 'TDUP load forwarding same type, different values, aliased table') + +test:done(true) -- 2.41.0