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 2/2] Fix base register coalescing in side trace. Date: Wed, 11 Oct 2023 18:04:10 +0300 [thread overview] Message-ID: <2756cdf8d8a4be9aa55dfdadbd3453da067d8969.1697034851.git.skaplun@tarantool.org> (raw) In-Reply-To: <cover.1697034851.git.skaplun@tarantool.org> From: Mike Pall <mike> Thanks to Sergey Kaplun, NiLuJe and Peter Cawley. (cherry-picked from commit aa2db7ebd1267836af5221336ccb4e9b4aa8372d) The previous patch fixed just part of the problem with the register coalesing. For example, the parent base register may be used inside the parent or child register sets when it shouldn't. This leads to incorrect register allocations, which may lead to crashes or undefined behaviour. This patch fixes it by excluding the parent base register from both register sets. The test case for this patch doesn't fail before the commit since it requires specific register allocation, which is hard to construct and very fragile. Due to the lack of ideal sync with the upstream repository, the test is passed before the patch. It should become correct in future patches. Resolves tarantool/tarantool#8767 Part of tarantool/tarantool#9145 Sergey Kaplun: * added the description and the test for the problem --- src/lj_asm.c | 7 +- src/lj_asm_arm.h | 7 +- src/lj_asm_arm64.h | 7 +- src/lj_asm_mips.h | 8 +- src/lj_asm_ppc.h | 8 +- src/lj_asm_x86.h | 8 +- .../lj-1031-asm-head-side-base-reg.test.lua | 139 ++++++++++++++++++ 7 files changed, 163 insertions(+), 21 deletions(-) create mode 100644 test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua diff --git a/src/lj_asm.c b/src/lj_asm.c index ca06860a..5137d05c 100644 --- a/src/lj_asm.c +++ b/src/lj_asm.c @@ -1860,6 +1860,7 @@ static void asm_head_side(ASMState *as) RegSet allow = RSET_ALL; /* Inverse of all coalesced registers. */ RegSet live = RSET_EMPTY; /* Live parent registers. */ RegSet pallow = RSET_GPR; /* Registers needed by the parent stack check. */ + Reg pbase; IRIns *irp = &as->parent->ir[REF_BASE]; /* Parent base. */ int32_t spadj, spdelta; int pass2 = 0; @@ -1870,7 +1871,11 @@ static void asm_head_side(ASMState *as) /* Force snap #0 alloc to prevent register overwrite in stack check. */ asm_snap_alloc(as, 0); } - allow = asm_head_side_base(as, irp, allow); + pbase = asm_head_side_base(as, irp); + if (pbase != RID_NONE) { + rset_clear(allow, pbase); + rset_clear(pallow, pbase); + } /* Scan all parent SLOADs and collect register dependencies. */ for (i = as->stopins; i > REF_BASE; i--) { diff --git a/src/lj_asm_arm.h b/src/lj_asm_arm.h index 47564d2e..5a0f925f 100644 --- a/src/lj_asm_arm.h +++ b/src/lj_asm_arm.h @@ -2107,7 +2107,7 @@ static void asm_head_root_base(ASMState *as) } /* Coalesce BASE register for a side trace. */ -static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) +static Reg asm_head_side_base(ASMState *as, IRIns *irp) { IRIns *ir; asm_head_lreg(as); @@ -2115,16 +2115,15 @@ static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) if (ra_hasreg(ir->r) && (rset_test(as->modset, ir->r) || irt_ismarked(ir->t))) ra_spill(as, ir); if (ra_hasspill(irp->s)) { - rset_clear(allow, ra_dest(as, ir, allow)); + return ra_dest(as, ir, RSET_GPR); } else { Reg r = irp->r; lj_assertA(ra_hasreg(r), "base reg lost"); - rset_clear(allow, r); if (r != ir->r && !rset_test(as->freeset, r)) ra_restore(as, regcost_ref(as->cost[r])); ra_destreg(as, ir, r); + return r; } - return allow; } /* -- Tail of trace ------------------------------------------------------- */ diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h index d1d4237b..88b47ceb 100644 --- a/src/lj_asm_arm64.h +++ b/src/lj_asm_arm64.h @@ -1873,7 +1873,7 @@ static void asm_head_root_base(ASMState *as) } /* Coalesce BASE register for a side trace. */ -static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) +static Reg asm_head_side_base(ASMState *as, IRIns *irp) { IRIns *ir; asm_head_lreg(as); @@ -1881,16 +1881,15 @@ static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) if (ra_hasreg(ir->r) && (rset_test(as->modset, ir->r) || irt_ismarked(ir->t))) ra_spill(as, ir); if (ra_hasspill(irp->s)) { - rset_clear(allow, ra_dest(as, ir, allow)); + return ra_dest(as, ir, RSET_GPR); } else { Reg r = irp->r; lj_assertA(ra_hasreg(r), "base reg lost"); - rset_clear(allow, r); if (r != ir->r && !rset_test(as->freeset, r)) ra_restore(as, regcost_ref(as->cost[r])); ra_destreg(as, ir, r); + return r; } - return allow; } /* -- Tail of trace ------------------------------------------------------- */ diff --git a/src/lj_asm_mips.h b/src/lj_asm_mips.h index ac9090f2..597c6d62 100644 --- a/src/lj_asm_mips.h +++ b/src/lj_asm_mips.h @@ -2595,7 +2595,7 @@ static void asm_head_root_base(ASMState *as) } /* Coalesce BASE register for a side trace. */ -static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) +static Reg asm_head_side_base(ASMState *as, IRIns *irp) { IRIns *ir = IR(REF_BASE); Reg r = ir->r; @@ -2605,15 +2605,15 @@ static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) if (rset_test(as->modset, r) || irt_ismarked(ir->t)) ir->r = RID_INIT; /* No inheritance for modified BASE register. */ if (irp->r == r) { - rset_clear(allow, r); /* Mark same BASE register as coalesced. */ + return r; /* Same BASE register already coalesced. */ } else if (ra_hasreg(irp->r) && rset_test(as->freeset, irp->r)) { - rset_clear(allow, irp->r); emit_move(as, r, irp->r); /* Move from coalesced parent reg. */ + return irp->r; } else { emit_getgl(as, r, jit_base); /* Otherwise reload BASE. */ } } - return allow; + return RID_NONE; } /* -- Tail of trace ------------------------------------------------------- */ diff --git a/src/lj_asm_ppc.h b/src/lj_asm_ppc.h index 971dcc88..7bba71b3 100644 --- a/src/lj_asm_ppc.h +++ b/src/lj_asm_ppc.h @@ -2130,7 +2130,7 @@ static void asm_head_root_base(ASMState *as) } /* Coalesce BASE register for a side trace. */ -static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) +static Reg asm_head_side_base(ASMState *as, IRIns *irp) { IRIns *ir = IR(REF_BASE); Reg r = ir->r; @@ -2139,15 +2139,15 @@ static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) if (rset_test(as->modset, r) || irt_ismarked(ir->t)) ir->r = RID_INIT; /* No inheritance for modified BASE register. */ if (irp->r == r) { - rset_clear(allow, r); /* Mark same BASE register as coalesced. */ + return r; /* Same BASE register already coalesced. */ } else if (ra_hasreg(irp->r) && rset_test(as->freeset, irp->r)) { - rset_clear(allow, irp->r); emit_mr(as, r, irp->r); /* Move from coalesced parent reg. */ + return irp->r; } else { emit_getgl(as, r, jit_base); /* Otherwise reload BASE. */ } } - return allow; + return RID_NONE; } /* -- Tail of trace ------------------------------------------------------- */ diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h index 2b810c8d..86ce3937 100644 --- a/src/lj_asm_x86.h +++ b/src/lj_asm_x86.h @@ -2856,7 +2856,7 @@ static void asm_head_root_base(ASMState *as) } /* Coalesce or reload BASE register for a side trace. */ -static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) +static Reg asm_head_side_base(ASMState *as, IRIns *irp) { IRIns *ir = IR(REF_BASE); Reg r = ir->r; @@ -2865,16 +2865,16 @@ static RegSet asm_head_side_base(ASMState *as, IRIns *irp, RegSet allow) if (rset_test(as->modset, r) || irt_ismarked(ir->t)) ir->r = RID_INIT; /* No inheritance for modified BASE register. */ if (irp->r == r) { - rset_clear(allow, r); /* Mark same BASE register as coalesced. */ + return r; /* Same BASE register already coalesced. */ } else if (ra_hasreg(irp->r) && rset_test(as->freeset, irp->r)) { /* Move from coalesced parent reg. */ - rset_clear(allow, irp->r); emit_rr(as, XO_MOV, r|REX_GC64, irp->r); + return irp->r; } else { emit_getgl(as, r, jit_base); /* Otherwise reload BASE. */ } } - return allow; + return RID_NONE; } /* -- Tail of trace ------------------------------------------------------- */ diff --git a/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua b/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua new file mode 100644 index 00000000..fc5efaaa --- /dev/null +++ b/test/tarantool-tests/lj-1031-asm-head-side-base-reg.test.lua @@ -0,0 +1,139 @@ +local tap = require('tap') +-- Test file to demonstrate incorrect side trace head assembling +-- when use parent trace register holding base (`RID_BASE`). +-- See also, https://github.com/LuaJIT/LuaJIT/issues/1031. +-- +-- XXX: For now, the test doesn't fail even on arm64 due to the +-- different from upstream register allocation. Nevertheless, this +-- issue should be gone with future backporting, so just leave the +-- test case as is. +local test = tap.test('lj-1031-asm-head-side-base-reg'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +local ffi = require 'ffi' +local int64_t = ffi.typeof('int64_t') + +test:plan(1) + +-- To reproduce the issue (reproduced only on arm64) we need a +-- little bit of a tricky structure for the traces: +-- +-> + -- start TRACE 1 +-- | | ... +-- | |---> + -- start TRACE 3 (side trace for the 1st) +-- | | | ... +-- | | v (link with TRACE 2) +-- | | +-> + -- start TRACE 2 (some loop that wasn't recorded +-- | | | | before due to the loop limit) +-- | | +---+ +-- +<--+ +-- Also, we need high register pressure to be sure that register +-- holding value of the base will be spilled. +-- So, during the assembly of "TRACE 3" (#6 in our test), +-- `RID_BASE` is spilled and restored via 32-bit fpr. This leads +-- to an incorrect value because it contains a 64-bit value. +-- See the original ticket for the details. + +-- XXX: Reduce amount of IR. +local tonumber = tonumber +local ffi_new = ffi.new + +-- Resulting slots for values calculated on trace. +-- luacheck: ignore +local r1 +local r2 +local r3 +local r4 +local r5 +local r6 +local r7 +local r8 +local r9 +local r10 +local r11 +local r12 +local r13 +local r14 +local r15 +local r16 +local r17 +local r18 +local r19 + +local ARR_SIZE = 1e2 +local lim_arr = {} + +-- XXX: Prevent irrelevant output in jit.dump(). +jit.off() + +local INNER_TRACE_LIMIT = 20 +local INNER_LIMIT1 = 1 +local INNER_LIMIT2 = 4 +for i = 1, INNER_TRACE_LIMIT do lim_arr[i] = INNER_LIMIT1 end +for i = INNER_TRACE_LIMIT + 1, ARR_SIZE + 1 do lim_arr[i] = INNER_LIMIT2 end + +-- Enable compilation back. +jit.on() + +jit.opt.start('hotloop=1', 'hotexit=2') + +-- XXX: Trace numbers are given with the respect of using +-- `jit.dump`. +-- Start TRACE 2 (1 is inside dump bc). +local k = 0 +while k < ARR_SIZE do + k = k + 1 + -- Forcify register pressure. + local l1 = ffi_new(int64_t, k + 1) + local l2 = ffi_new(int64_t, k + 2) + local l3 = ffi_new(int64_t, k + 3) + local l4 = ffi_new(int64_t, k + 4) + local l5 = ffi_new(int64_t, k + 5) + local l6 = ffi_new(int64_t, k + 6) + local l7 = ffi_new(int64_t, k + 7) + local l8 = ffi_new(int64_t, k + 8) + local l9 = ffi_new(int64_t, k + 9) + local l10 = ffi_new(int64_t, k + 10) + local l11 = ffi_new(int64_t, k + 11) + local l12 = ffi_new(int64_t, k + 12) + local l13 = ffi_new(int64_t, k + 13) + local l14 = ffi_new(int64_t, k + 14) + local l15 = ffi_new(int64_t, k + 15) + local l16 = ffi_new(int64_t, k + 16) + local l17 = ffi_new(int64_t, k + 17) + local l18 = ffi_new(int64_t, k + 18) + local l19 = ffi_new(int64_t, k + 19) + -- Side exit for TRACE 6 start 2/1. + -- luacheck: ignore + if k > 55 then else end + r1 = tonumber(l1) + r2 = tonumber(l2) + r3 = tonumber(l3) + r4 = tonumber(l4) + r5 = tonumber(l5) + r6 = tonumber(l6) + r7 = tonumber(l7) + r8 = tonumber(l8) + r9 = tonumber(l9) + r10 = tonumber(l10) + r11 = tonumber(l11) + r12 = tonumber(l12) + r13 = tonumber(l13) + r14 = tonumber(l14) + r15 = tonumber(l15) + r15 = tonumber(l15) + r16 = tonumber(l16) + r17 = tonumber(l17) + r18 = tonumber(l18) + r19 = tonumber(l19) + local lim_inner = lim_arr[k] + -- TRACE 3. Loop is not taken at the moment of recording TRACE 2 + -- (lim_inner == 1). + for _ = 1, lim_inner do + -- TRACE 6 stop -> 3. + end +end + +test:ok(true, 'correct side trace head assembling') + +test:done(true) -- 2.42.0
next prev parent reply other threads:[~2023-10-11 15:09 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-10-11 15:04 [Tarantool-patches] [PATCH luajit 0/2] Fix assembling of the head of side traces Sergey Kaplun via Tarantool-patches 2023-10-11 15:04 ` [Tarantool-patches] [PATCH luajit 1/2] Fix register mask for stack check in head of side trace Sergey Kaplun via Tarantool-patches 2023-10-13 5:48 ` Maxim Kokryashkin via Tarantool-patches 2023-10-23 9:27 ` Sergey Kaplun via Tarantool-patches 2023-10-24 13:51 ` Maxim Kokryashkin via Tarantool-patches 2023-11-07 18:42 ` Igor Munkin via Tarantool-patches 2023-10-11 15:04 ` Sergey Kaplun via Tarantool-patches [this message] 2023-10-13 5:59 ` [Tarantool-patches] [PATCH luajit 2/2] Fix base register coalescing in " Maxim Kokryashkin via Tarantool-patches 2023-10-23 9:50 ` Sergey Kaplun via Tarantool-patches 2023-10-24 13:50 ` Maxim Kokryashkin via Tarantool-patches 2023-11-07 18:42 ` Igor Munkin via Tarantool-patches 2023-11-23 6:30 ` [Tarantool-patches] [PATCH luajit 0/2] Fix assembling of the head of side traces 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=2756cdf8d8a4be9aa55dfdadbd3453da067d8969.1697034851.git.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 2/2] Fix base register coalescing in side trace.' \ /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