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 BAE9C1453E9A; Wed, 25 Jun 2025 17:42:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BAE9C1453E9A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1750862533; bh=8fmYo040jymrmr0Gc2+OqCw7bPVjObxdG2MFnDEiF+I=; 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=Nq+gsFbKEgPHKCoBooab5d6lWvGR7SqegTNHDSlIlqpz3CEk2O/GrMJcnI73r6uxo U3CQgPPs5T0sNwFsiOwkd2e8zUM4yrFZNpHOXhidMablJS8bshRCXKRNbXchwHSQQe uW5j7rVq51tGly5dt4QWm9Z7XX5AaYJTS2E4q7h0= Received: from send264.i.mail.ru (send264.i.mail.ru [95.163.59.103]) (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 493D61453E9E for ; Wed, 25 Jun 2025 17:42:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 493D61453E9E Received: by exim-smtp-5f9ff66d98-gq767 with esmtpa (envelope-from ) id 1uURKF-0000000079i-1dsJ; Wed, 25 Jun 2025 17:42:11 +0300 Date: Wed, 25 Jun 2025 17:41:42 +0300 To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Message-ID: References: <20250612093651.7552-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D919194CF4FC660463A1748EEF353670DFA714FA02D430EB00894C459B0CD1B9BA3CB5B056FF3E5791417EB218679B82F34EFF50F766DAB020586835DE1D005ABE99E8247124EAAE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78887611F2F2455C9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB553375669C5B23AD11DEB7B918B6C5C75DB55619CF91805146586DDE3F04AE77F09CB292389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0D9442B0B5983000E8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B652D31B9D28593E51CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C224943F02D679E232DDC76E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B432225C0CEA351033AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B2FFDA4F57982C5F435872C767BF85DA227C277FBC8AE2E8B53A69B3AC30C7B9475ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A55E70A24C3A0C1CA45002B1117B3ED696748DEE446935973772305013E4AE841E823CB91A9FED034534781492E4B8EEADF5E532225D4D775BBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3444047AE358B40754AC15EB1A22652F9FEA4B6CF573051FC9721DF6640C62448779AD07442DF069381D7E09C32AA3244CAFCEB0EE0C56E1CE77DD89D51EBB77424264A3932255F933EA455F16B58544A2E30DDF7C44BCB90DA5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVVWXk7QTiVzHggo1MZXFC4M= X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B590259B94F1AC424043DE06ABAFEAF67056143EAFE254DFF77B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix assembly of HREFK. 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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the review! Please consider my answers below. On 25.06.25, Sergey Bronnikov wrote: > Hi, Sergey, > > thanks for the patch! Please see my comments below. > > Sergey > > On 6/12/25 12:36, Sergey Kaplun wrote: > > From: Mike Pall > > > > Reported by caohongqing. > > Fix contributed by Peter Cawley. > > > > (cherry picked from commit 8fbd576fb9414a5fa70dfa6069733d3416a78269) > > > > `asm_hrefk()` uses the check for the offset for the corresponding node > > structure. However, the target load is performed from its inner `key` > > field with the offset 8. In the case of a huge table, it is possible > > that the offset of the node (4095 * 8) is less than 4096 * 8 and can be > > emitted via the corresponding instruction as an immediate offset, but > > the offset of the `key` field is not. This leads to the corresponding > > assertion failure in `emit_lso()`. > > The issue [1] contains yet another fix in the same place [2]. We decided > to backport the patch > > separately. But please mention this in commit message. > > > 1. https://github.com/LuaJIT/LuaJIT/issues/1026 > > 2. > https://github.com/LuaJIT/LuaJIT/commit/93ce12ee15abf28ef4cb24ae7e4b8a5b73d75c85 These issues are completely independent, IMO. I would rather not mention it. Otherwise, by this logic, we should mention every problem related to the HREFK here. > > This patch fixes this behaviour by the correct check. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#11278 > > --- > > > > Related issues: > > *https://github.com/LuaJIT/LuaJIT/issues/1026 > > *https://github.com/tarantool/tarantool/issues/11278 > > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1026-arm64-invalid-hrefk-offset-check > > > > src/lj_asm_arm64.h | 2 +- > > ...-arm64-invalid-hrefk-offset-check.test.lua | 48 +++++++++++++++++++ > > 2 files changed, 49 insertions(+), 1 deletion(-) > > create mode 100644 test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua > > > > diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h > > index 6c7b011f..a7f059a2 100644 > > --- a/src/lj_asm_arm64.h > > +++ b/src/lj_asm_arm64.h > > @@ -885,7 +885,7 @@ static void asm_hrefk(ASMState *as, IRIns *ir) > > diff --git a/test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua b/test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua > > new file mode 100644 > > index 00000000..de243814 > > --- /dev/null > > +++ b/test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua > > @@ -0,0 +1,48 @@ > > +local tap = require('tap') > > + > > +-- Test file to demonstrate LuaJIT misbehaviour when assembling > > +-- HREFK instruction on arm64 with the huge offset. > > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1026. > > +local test = tap.test('lj-1026-arm64-invalid-hrefk-offset-check'):skipcond({ > > + ['Test requires JIT enabled'] = not jit.status(), > It is an ARM-specific patch, should we add a condition for ARM here? It is a good question. It was once discussed, and we decided not to add the skip condition to make other architectures more covered by tests too (for example, we may check MIPS/PPC in the same test if we want to support them). > > +}) > > + > > +test:plan(1) > > + > > +-- The assertion fails since in HREFK we are checking the offset > > +-- from the hslots of the table of the Node structure itself > s/Node/`Node`/ Fixed. See the iterative patch below. =================================================================== diff --git a/test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua b/test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua index de243814..caa6291d 100644 --- a/test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua +++ b/test/tarantool-tests/lj-1026-arm64-invalid-hrefk-offset-check.test.lua @@ -10,7 +10,7 @@ local test = tap.test('lj-1026-arm64-invalid-hrefk-offset-check'):skipcond({ test:plan(1) -- The assertion fails since in HREFK we are checking the offset --- from the hslots of the table of the Node structure itself +-- from the hslots of the table of the `Node` structure itself -- instead of its inner field `key` (with additional 8 bytes). -- So to test this, we generate a big table with constant keys -- and compile a trace for each HREFK possible. =================================================================== Branch is force-pushed. > > +-- instead of its inner field `key` (with additional 8 bytes). > > +-- So to test this, we generate a big table with constant keys > > +-- and compile a trace for each HREFK possible. > > + > > +local big_tab = {} > > +-- The map of the characters to generate constant string keys. > > +-- The offset of the node should be 4096 * 8. It takes at least > > +-- 1365 keys to hit this value. The maximum possible slots in the > > +-- hash part is 2048, so to fill it with the maximum density (with > > +-- the way below), we need 45 * 45 = 2025 keys. > > +local chars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRS' > > +for c1 inchars:gmatch('.') do > > + for c2 inchars:gmatch('.') do > > + big_tab[c1 .. c2] = 1 > > + end > > +end > > + > > +jit.opt.start('hotloop=1') > > + > > +-- Generate bunch of traces. > > +for c1 inchars:gmatch('.') do > > + for c2 inchars:gmatch('.') do > > + loadstring([=[ > > + local t = ... > > + for i = 1, 4 do > > + -- HREFK generation. > > + t[ ']=] .. c1 .. c2 .. [=[' ] = i > > + end > > + ]=])(big_tab) > > + end > > +end > > + > > +test:ok(true, 'no assertion failed') > > I would replace testcase description to something like "emitted assembly > is correct". > > Feel free to ignore. It triggers the assertion in the first place, so ignoring. > > > + > > +test:done(true) -- Best regards, Sergey Kaplun