From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>, Sergey Bronnikov <sergeyb@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH luajit] Emit sunk IR_NEWREF only once per key on snapshot replay. Date: Mon, 11 Dec 2023 18:35:20 +0300 [thread overview] Message-ID: <20231211153520.9322-1-skaplun@tarantool.org> (raw) From: Mike Pall <mike> Thanks to Sergey Kaplun and Peter Cawley. (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e) Assume we have the parent trace with the following IRs: | 0001 } tab TNEW #0 #0 | 0002 } p32 NEWREF 0001 "key" | 0003 } fal HSTORE 0002 false | .... SNAP #1 [ ---- ---- 0001 ---- ] | 0004 > num SLOAD #1 T | .... SNAP #2 [ ---- ---- 0001 ] | 0005 > num EQ 0004 0004 | 0006 } tru HSTORE 0002 true | .... SNAP #3 [ ---- ---- 0001 true ] The side trace for the third snapshot emits the following IRs: | 0001 tab TNEW #0 #0 | 0002 p32 NEWREF 0001 "key" | 0003 fal HSTORE 0002 false | 0004 p32 NEWREF 0001 "key" | 0005 tru HSTORE 0004 true As we can see, `NEWREF` is emitted twice. This is a violation of its semantics, so the second store isn't noticeable. This patch prevents the second emitting of IR NEWREF by checking the last one emitted NEWREF IR. There is no need to check NEWREFs beyond since it guarantees the snapshot is taken after it, because it may cause table rehashing, so all prior results are invalidated. Sergey Kaplun: * added the description and the test for the problem Resolves tarantool/tarantool#7937 Part of tarantool/tarantool#9145 --- Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1128-double-ir-newref-on-restore-sunk PR: https://github.com/tarantool/tarantool/pull/9466 Related issues: * https://github.com/LuaJIT/LuaJIT/issues/1128 * https://github.com/tarantool/tarantool/issues/7937 src/lj_snap.c | 16 ++++ ...-double-ir-newref-on-restore-sunk.test.lua | 81 +++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua diff --git a/src/lj_snap.c b/src/lj_snap.c index 3f0fccec..73e18e69 100644 --- a/src/lj_snap.c +++ b/src/lj_snap.c @@ -583,9 +583,25 @@ void lj_snap_replay(jit_State *J, GCtrace *T) if (irr->o == IR_HREFK || irr->o == IR_AREF) { IRIns *irf = &T->ir[irr->op1]; tmp = emitir(irf->ot, tmp, irf->op2); + } else if (irr->o == IR_NEWREF) { + IRRef allocref = tref_ref(tr); + IRRef keyref = tref_ref(key); + IRRef newref_ref = J->chain[IR_NEWREF]; + IRIns *newref = &J->cur.ir[newref_ref]; + lj_assertJ(irref_isk(keyref), + "sunk store for parent IR %04d with bad key %04d", + refp - REF_BIAS, keyref - REF_BIAS); + if (newref_ref > allocref && newref->op2 == keyref) { + lj_assertJ(newref->op1 == allocref, + "sunk store for parent IR %04d with bad tab %04d", + refp - REF_BIAS, allocref - REF_BIAS); + tmp = newref_ref; + goto skip_newref; + } } } tmp = emitir(irr->ot, tmp, key); + skip_newref: val = snap_pref(J, T, map, nent, seen, irs->op2); if (val == 0) { IRIns *irc = &T->ir[irs->op2]; diff --git a/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua new file mode 100644 index 00000000..a89beab6 --- /dev/null +++ b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua @@ -0,0 +1,81 @@ +local tap = require('tap') + +-- Test file to demonstrate LuaJIT incorrect restoring of sunk +-- tables with double usage of IR_NEWREF. +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1128. + +local test = tap.test('lj-1128-double-ir-newref-on-restore-sunk'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +test:plan(3) + +local take_side + +local function trace_base(num) + local tab = {} + tab.key = false + -- This check can't be folded since `num` can be NaN. + tab.key = num == num + -- luacheck: ignore + -- This side trace emits the following IRs: + -- 0001 tab TNEW #0 #0 + -- 0002 p64 NEWREF 0001 "key" + -- 0003 fal HSTORE 0002 false + -- 0004 p64 NEWREF 0001 "key" + -- 0005 tru HSTORE 0004 true + -- As we can see, `NEWREF` is emitted twice. This is a violation + -- of its semantics, so the second store isn't noticeable. + if take_side then end + return tab.key +end + +-- Uncompiled function to end up side trace here. +local function trace_base_wp(num) + return trace_base(num) +end +jit.off(trace_base_wp) + +-- Same function as above, but with two IRs NEWREF emitted. +local function trace_2newref(num) + local tab = {} + tab.key = false + -- This + op can't be folded since `num` can be -0. + tab.key = num + 0 + tab.key2 = false + -- This check can't be folded since `num` can be NaN. + tab.key2 = num == num + -- luacheck: ignore + if take_side then end + return tab.key, tab.key2 +end + +-- Uncompiled function to end up side trace here. +local function trace_2newref_wp(num) + return trace_2newref(num) +end +jit.off(trace_2newref_wp) + +jit.opt.start('hotloop=1', 'hotexit=1', 'tryside=1') + +-- Compile parent traces. +trace_base_wp(0) +trace_base_wp(0) +trace_2newref_wp(0) +trace_2newref_wp(0) + +-- Compile side traces. +take_side = true +trace_base_wp(0) +trace_base_wp(0) +trace_2newref_wp(0) +trace_2newref_wp(0) + +test:is(trace_base(0), true, 'sunk value restored correctly') + +local arg = 0 +local r1, r2 = trace_2newref(arg) +test:is(r1, arg, 'sunk value restored correctly with 2 keys, first key') +test:is(r2, true, 'sunk value restored correctly with 2 keys, second key') + +test:done(true) -- 2.43.0
next reply other threads:[~2023-12-11 15:39 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-12-11 15:35 Sergey Kaplun via Tarantool-patches [this message] 2023-12-12 9:44 ` Maxim Kokryashkin via Tarantool-patches 2023-12-12 10:10 ` Sergey Kaplun via Tarantool-patches 2023-12-12 11:45 ` Maxim Kokryashkin via Tarantool-patches 2024-01-16 11:54 ` Sergey Bronnikov via Tarantool-patches 2024-02-15 13:41 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20231211153520.9322-1-skaplun@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=m.kokryashkin@tarantool.org \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] Emit sunk IR_NEWREF only once per key on snapshot replay.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox