[Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Igor Munkin via Tarantool-patches]

From: Mike Pall <mike>

Reported by Igor Munkin.

(cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)

Side exits with the same exitno use the same snapshot for restoring
guest stack values. This obliges all guards related to the particular
snapshot use the same RegSP mapping for the values to be restored at the
trace exit. RENAME emitted prior to the guard for the same snapshot
leads to the aforementioned invariant violation. The easy way to save
the snapshot consistency is spilling the renamed IR reference, that is
done in scope of <asm_snap_checkrename>.

However, the previous <asm_snap_checkrename> implementation considers
only the IR references explicitly mentioned in the snapshot. E.g. if
there is a sunk[1] object to be restored at the trace exit, and the
renamed reference is a *STORE to that object, the spill slot is not
allocated. As a result an invalid value is stored while unsinking that
object at all corresponding side exits prior to the emitted renaming.

To handle also those IR references implicitly used in the snapshot, all
non-constant and non-sunk references are added to the Bloom filter (it's
worth to mention that two hash functions are used to reduce collisions
for the cases when the number of IR references emitted between two
different snapshots exceeds the filter size). New <asm_snap_checkrename>
implementation tests whether the renamed IR reference is in the filter
and forces a spill slot for it as a result.

[1]: http://wiki.luajit.org/Allocation-Sinking-Optimization

Igor Munkin:
* added the description and the test for the problem

Resolves tarantool/tarantool#5118
Follows up tarantool/tarantool#4252

Signed-off-by: Igor Munkin <imun@tarantool.org>
---

Related issues:
* https://github.com/tarantool/tarantool/issues/5118
* https://github.com/tarantool/tarantool/issues/4252
* https://github.com/LuaJIT/LuaJIT/issues/584
Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
CI: https://github.com/tarantool/tarantool/commit/b35e2ee

 src/lj_asm.c                                  | 25 ++++---
 ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
 2 files changed, 81 insertions(+), 13 deletions(-)
 create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua

diff --git a/src/lj_asm.c b/src/lj_asm.c
index c2cf5a95..9267448e 100644
--- a/src/lj_asm.c
+++ b/src/lj_asm.c
@@ -72,6 +72,7 @@ typedef struct ASMState {
   IRRef snaprename;	/* Rename highwater mark for snapshot check. */
   SnapNo snapno;	/* Current snapshot number. */
   SnapNo loopsnapno;	/* Loop snapshot number. */
+  BloomFilter snapfilt1, snapfilt2;	/* Filled with snapshot refs. */
 
   IRRef fuseref;	/* Fusion limit (loopref, 0 or FUSE_DISABLED). */
   IRRef sectref;	/* Section base reference (loopref or 0). */
@@ -876,7 +877,10 @@ static int asm_sunk_store(ASMState *as, IRIns *ira, IRIns *irs)
 static void asm_snap_alloc1(ASMState *as, IRRef ref)
 {
   IRIns *ir = IR(ref);
-  if (!irref_isk(ref) && (!(ra_used(ir) || ir->r == RID_SUNK))) {
+  if (!irref_isk(ref) && ir->r != RID_SUNK) {
+    bloomset(as->snapfilt1, ref);
+    bloomset(as->snapfilt2, hashrot(ref, ref + HASH_BIAS));
+    if (ra_used(ir)) return;
     if (ir->r == RID_SINK) {
       ir->r = RID_SUNK;
 #if LJ_HASFFI
@@ -933,6 +937,7 @@ static void asm_snap_alloc(ASMState *as)
   SnapShot *snap = &as->T->snap[as->snapno];
   SnapEntry *map = &as->T->snapmap[snap->mapofs];
   MSize n, nent = snap->nent;
+  as->snapfilt1 = as->snapfilt2 = 0;
   for (n = 0; n < nent; n++) {
     SnapEntry sn = map[n];
     IRRef ref = snap_ref(sn);
@@ -955,18 +960,12 @@ static void asm_snap_alloc(ASMState *as)
 */
 static int asm_snap_checkrename(ASMState *as, IRRef ren)
 {
-  SnapShot *snap = &as->T->snap[as->snapno];
-  SnapEntry *map = &as->T->snapmap[snap->mapofs];
-  MSize n, nent = snap->nent;
-  for (n = 0; n < nent; n++) {
-    SnapEntry sn = map[n];
-    IRRef ref = snap_ref(sn);
-    if (ref == ren || (LJ_SOFTFP && (sn & SNAP_SOFTFPNUM) && ++ref == ren)) {
-      IRIns *ir = IR(ref);
-      ra_spill(as, ir);  /* Register renamed, so force a spill slot. */
-      RA_DBGX((as, "snaprensp $f $s", ref, ir->s));
-      return 1;  /* Found. */
-    }
+  if (bloomtest(as->snapfilt1, ren) &&
+      bloomtest(as->snapfilt2, hashrot(ren, ren + HASH_BIAS))) {
+    IRIns *ir = IR(ren);
+    ra_spill(as, ir);  /* Register renamed, so force a spill slot. */
+    RA_DBGX((as, "snaprensp $f $s", ren, ir->s));
+    return 1;  /* Found. */
   }
   return 0;  /* Not found. */
 }
diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
new file mode 100644
index 00000000..8aad3438
--- /dev/null
+++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
@@ -0,0 +1,69 @@
+local tap = require('tap')
+
+local test = tap.test('lj-584-bad-renames-for-sunk-values')
+test:plan(1)
+
+-- Test file to demonstrate LuaJIT assembler misbehaviour.
+-- For more info, proceed to the issues:
+-- * https://github.com/LuaJIT/LuaJIT/issues/584
+-- * https://github.com/tarantool/tarantool/issues/4252
+
+----- Related part of luafun.lua. --------------------------------
+
+local iterator_mt = {
+  __call = function(self, param, state) return self.gen(param, state) end,
+}
+
+local wrap = function(gen, param, state)
+  return setmetatable({
+    gen = gen,
+    param = param,
+    state = state
+  }, iterator_mt), param, state
+end
+
+-- These functions call each other to implement a flat iterator
+-- over the several iterable objects.
+local chain_gen_r1, chain_gen_r2
+
+chain_gen_r2 = function(param, state, state_x, ...)
+  if state_x ~= nil then return { state[1], state_x }, ...  end
+  local i = state[1] + 1
+  if param[3 * i - 1] == nil then return nil end
+  return chain_gen_r1(param, { i, param[3 * i] })
+end
+
+chain_gen_r1 = function(param, state)
+  local i, state_x = state[1], state[2]
+  local gen_x, param_x = param[3 * i - 2], param[3 * i - 1]
+  return chain_gen_r2(param, state, gen_x(param_x, state_x))
+end
+
+local chain = function(...)
+  local param = { }
+  for i = 1, select('#', ...) do
+    -- Put gen, param, state into param table.
+    param[3 * i - 2], param[3 * i - 1], param[3 * i]
+      = wrap(ipairs(select(i, ...)))
+  end
+  return wrap(chain_gen_r1, param, { 1, param[3] })
+end
+
+----- Reproducer. ------------------------------------------------
+
+jit.opt.start(3, 'hotloop=3')
+
+xpcall(function()
+  for _ = 1, 3 do
+    local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' })
+    while true do
+      state_x = gen_x(param_x, state_x)
+      if state_x == nil then break end
+    end
+  end
+  test:ok('All emitted RENAMEs are fine')
+end, function()
+  test:fail('Invalid Lua stack has been restored')
+end)
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Sergey Ostanevich via Tarantool-patches]

Hi! Thanks for the patch!

Just a small nit to the test. I won’t comment Mike’s code :)

LGTM

Sergos

> On 24 Jul 2021, at 20:23, Igor Munkin <imun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> Reported by Igor Munkin.
> 
> (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)
> 
> Side exits with the same exitno use the same snapshot for restoring
> guest stack values. This obliges all guards related to the particular
> snapshot use the same RegSP mapping for the values to be restored at the
> trace exit. RENAME emitted prior to the guard for the same snapshot
> leads to the aforementioned invariant violation. The easy way to save
> the snapshot consistency is spilling the renamed IR reference, that is
> done in scope of <asm_snap_checkrename>.
> 
> However, the previous <asm_snap_checkrename> implementation considers
> only the IR references explicitly mentioned in the snapshot. E.g. if
> there is a sunk[1] object to be restored at the trace exit, and the
> renamed reference is a *STORE to that object, the spill slot is not
> allocated. As a result an invalid value is stored while unsinking that
> object at all corresponding side exits prior to the emitted renaming.
> 
> To handle also those IR references implicitly used in the snapshot, all
> non-constant and non-sunk references are added to the Bloom filter (it's
> worth to mention that two hash functions are used to reduce collisions
> for the cases when the number of IR references emitted between two
> different snapshots exceeds the filter size). New <asm_snap_checkrename>
> implementation tests whether the renamed IR reference is in the filter
> and forces a spill slot for it as a result.
> 
> [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization
> 
> Igor Munkin:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#5118
> Follows up tarantool/tarantool#4252
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Related issues:
> * https://github.com/tarantool/tarantool/issues/5118
> * https://github.com/tarantool/tarantool/issues/4252
> * https://github.com/LuaJIT/LuaJIT/issues/584
> Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
> CI: https://github.com/tarantool/tarantool/commit/b35e2ee
> 
> src/lj_asm.c                                  | 25 ++++---
> ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
> 2 files changed, 81 insertions(+), 13 deletions(-)
> create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> 
> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index c2cf5a95..9267448e 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c
> @@ -72,6 +72,7 @@ typedef struct ASMState {
>   IRRef snaprename;	/* Rename highwater mark for snapshot check. */
>   SnapNo snapno;	/* Current snapshot number. */
>   SnapNo loopsnapno;	/* Loop snapshot number. */
> +  BloomFilter snapfilt1, snapfilt2;	/* Filled with snapshot refs. */
> 
>   IRRef fuseref;	/* Fusion limit (loopref, 0 or FUSE_DISABLED). */
>   IRRef sectref;	/* Section base reference (loopref or 0). */
> @@ -876,7 +877,10 @@ static int asm_sunk_store(ASMState *as, IRIns *ira, IRIns *irs)
> static void asm_snap_alloc1(ASMState *as, IRRef ref)
> {
>   IRIns *ir = IR(ref);
> -  if (!irref_isk(ref) && (!(ra_used(ir) || ir->r == RID_SUNK))) {
> +  if (!irref_isk(ref) && ir->r != RID_SUNK) {
> +    bloomset(as->snapfilt1, ref);
> +    bloomset(as->snapfilt2, hashrot(ref, ref + HASH_BIAS));
> +    if (ra_used(ir)) return;

>     if (ir->r == RID_SINK) {
>       ir->r = RID_SUNK;
> #if LJ_HASFFI
> @@ -933,6 +937,7 @@ static void asm_snap_alloc(ASMState *as)
>   SnapShot *snap = &as->T->snap[as->snapno];
>   SnapEntry *map = &as->T->snapmap[snap->mapofs];
>   MSize n, nent = snap->nent;
> +  as->snapfilt1 = as->snapfilt2 = 0;
>   for (n = 0; n < nent; n++) {
>     SnapEntry sn = map[n];
>     IRRef ref = snap_ref(sn);
> @@ -955,18 +960,12 @@ static void asm_snap_alloc(ASMState *as)
> */
> static int asm_snap_checkrename(ASMState *as, IRRef ren)
> {
> -  SnapShot *snap = &as->T->snap[as->snapno];
> -  SnapEntry *map = &as->T->snapmap[snap->mapofs];
> -  MSize n, nent = snap->nent;
> -  for (n = 0; n < nent; n++) {
> -    SnapEntry sn = map[n];
> -    IRRef ref = snap_ref(sn);
> -    if (ref == ren || (LJ_SOFTFP && (sn & SNAP_SOFTFPNUM) && ++ref == ren)) {
> -      IRIns *ir = IR(ref);
> -      ra_spill(as, ir);  /* Register renamed, so force a spill slot. */
> -      RA_DBGX((as, "snaprensp $f $s", ref, ir->s));
> -      return 1;  /* Found. */
> -    }
> +  if (bloomtest(as->snapfilt1, ren) &&
> +      bloomtest(as->snapfilt2, hashrot(ren, ren + HASH_BIAS))) {
> +    IRIns *ir = IR(ren);
> +    ra_spill(as, ir);  /* Register renamed, so force a spill slot. */
> +    RA_DBGX((as, "snaprensp $f $s", ren, ir->s));
> +    return 1;  /* Found. */
>   }
>   return 0;  /* Not found. */
> }
> diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> new file mode 100644
> index 00000000..8aad3438
> --- /dev/null
> +++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> @@ -0,0 +1,69 @@
> +local tap = require('tap')
> +
> +local test = tap.test('lj-584-bad-renames-for-sunk-values')
> +test:plan(1)
> +
> +-- Test file to demonstrate LuaJIT assembler misbehaviour.
> +-- For more info, proceed to the issues:
> +-- * https://github.com/LuaJIT/LuaJIT/issues/584
> +-- * https://github.com/tarantool/tarantool/issues/4252
> +
> +----- Related part of luafun.lua. --------------------------------
> +
> +local iterator_mt = {
> +  __call = function(self, param, state) return self.gen(param, state) end,
> +}
> +
> +local wrap = function(gen, param, state)
> +  return setmetatable({
> +    gen = gen,
> +    param = param,
> +    state = state
> +  }, iterator_mt), param, state
> +end
> +
> +-- These functions call each other to implement a flat iterator
> +-- over the several iterable objects.
> +local chain_gen_r1, chain_gen_r2
> +
> +chain_gen_r2 = function(param, state, state_x, ...)
> +  if state_x ~= nil then return { state[1], state_x }, ...  end
> +  local i = state[1] + 1
> +  if param[3 * i - 1] == nil then return nil end
> +  return chain_gen_r1(param, { i, param[3 * i] })
> +end
> +
> +chain_gen_r1 = function(param, state)
> +  local i, state_x = state[1], state[2]
> +  local gen_x, param_x = param[3 * i - 2], param[3 * i - 1]
> +  return chain_gen_r2(param, state, gen_x(param_x, state_x))
> +end
> +
> +local chain = function(...)
> +  local param = { }
> +  for i = 1, select('#', ...) do
> +    -- Put gen, param, state into param table.
> +    param[3 * i - 2], param[3 * i - 1], param[3 * i]
> +      = wrap(ipairs(select(i, ...)))
> +  end
> +  return wrap(chain_gen_r1, param, { 1, param[3] })
> +end
> +
> +----- Reproducer. ------------------------------------------------
> +
> +jit.opt.start(3, 'hotloop=3')

I don’t like both numbers here. opt_level is 3 by default - why bother setting it?
And the second one should be factored out as an argument for both opt.start and the
loop below?

> +
> +xpcall(function()
> +  for _ = 1, 3 do
> +    local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' })
> +    while true do
> +      state_x = gen_x(param_x, state_x)
> +      if state_x == nil then break end
> +    end
> +  end
> +  test:ok('All emitted RENAMEs are fine')
> +end, function()
> +  test:fail('Invalid Lua stack has been restored')
> +end)
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.25.0
> 

Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Igor Munkin via Tarantool-patches]

Sergos,

Thanks for your review!

On 27.07.21, Sergey Ostanevich wrote:
> Hi! Thanks for the patch!
> 
> Just a small nit to the test. I won’t comment Mike’s code :)

The fix is much more clearer and simpler than the test for it...

> 
> LGTM

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> Sergos
> 
> > On 24 Jul 2021, at 20:23, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > Reported by Igor Munkin.
> > 
> > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)
> > 
> > Side exits with the same exitno use the same snapshot for restoring
> > guest stack values. This obliges all guards related to the particular
> > snapshot use the same RegSP mapping for the values to be restored at the
> > trace exit. RENAME emitted prior to the guard for the same snapshot
> > leads to the aforementioned invariant violation. The easy way to save
> > the snapshot consistency is spilling the renamed IR reference, that is
> > done in scope of <asm_snap_checkrename>.
> > 
> > However, the previous <asm_snap_checkrename> implementation considers
> > only the IR references explicitly mentioned in the snapshot. E.g. if
> > there is a sunk[1] object to be restored at the trace exit, and the
> > renamed reference is a *STORE to that object, the spill slot is not
> > allocated. As a result an invalid value is stored while unsinking that
> > object at all corresponding side exits prior to the emitted renaming.
> > 
> > To handle also those IR references implicitly used in the snapshot, all
> > non-constant and non-sunk references are added to the Bloom filter (it's
> > worth to mention that two hash functions are used to reduce collisions
> > for the cases when the number of IR references emitted between two
> > different snapshots exceeds the filter size). New <asm_snap_checkrename>
> > implementation tests whether the renamed IR reference is in the filter
> > and forces a spill slot for it as a result.
> > 
> > [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization
> > 
> > Igor Munkin:
> > * added the description and the test for the problem
> > 
> > Resolves tarantool/tarantool#5118
> > Follows up tarantool/tarantool#4252
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > Related issues:
> > * https://github.com/tarantool/tarantool/issues/5118
> > * https://github.com/tarantool/tarantool/issues/4252
> > * https://github.com/LuaJIT/LuaJIT/issues/584
> > Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
> > CI: https://github.com/tarantool/tarantool/commit/b35e2ee
> > 
> > src/lj_asm.c                                  | 25 ++++---
> > ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
> > 2 files changed, 81 insertions(+), 13 deletions(-)
> > create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> > 

<snipped>

> > diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> > new file mode 100644
> > index 00000000..8aad3438
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> > @@ -0,0 +1,69 @@
> > +local tap = require('tap')
> > +
> > +local test = tap.test('lj-584-bad-renames-for-sunk-values')
> > +test:plan(1)
> > +
> > +-- Test file to demonstrate LuaJIT assembler misbehaviour.
> > +-- For more info, proceed to the issues:
> > +-- * https://github.com/LuaJIT/LuaJIT/issues/584
> > +-- * https://github.com/tarantool/tarantool/issues/4252
> > +
> > +----- Related part of luafun.lua. --------------------------------
> > +
> > +local iterator_mt = {
> > +  __call = function(self, param, state) return self.gen(param, state) end,
> > +}
> > +
> > +local wrap = function(gen, param, state)
> > +  return setmetatable({
> > +    gen = gen,
> > +    param = param,
> > +    state = state
> > +  }, iterator_mt), param, state
> > +end
> > +
> > +-- These functions call each other to implement a flat iterator
> > +-- over the several iterable objects.
> > +local chain_gen_r1, chain_gen_r2
> > +
> > +chain_gen_r2 = function(param, state, state_x, ...)
> > +  if state_x ~= nil then return { state[1], state_x }, ...  end
> > +  local i = state[1] + 1
> > +  if param[3 * i - 1] == nil then return nil end
> > +  return chain_gen_r1(param, { i, param[3 * i] })
> > +end
> > +
> > +chain_gen_r1 = function(param, state)
> > +  local i, state_x = state[1], state[2]
> > +  local gen_x, param_x = param[3 * i - 2], param[3 * i - 1]
> > +  return chain_gen_r2(param, state, gen_x(param_x, state_x))
> > +end
> > +
> > +local chain = function(...)
> > +  local param = { }
> > +  for i = 1, select('#', ...) do
> > +    -- Put gen, param, state into param table.
> > +    param[3 * i - 2], param[3 * i - 1], param[3 * i]
> > +      = wrap(ipairs(select(i, ...)))
> > +  end
> > +  return wrap(chain_gen_r1, param, { 1, param[3] })
> > +end
> > +
> > +----- Reproducer. ------------------------------------------------
> > +
> > +jit.opt.start(3, 'hotloop=3')
> 
> I don’t like both numbers here. opt_level is 3 by default - why bother setting it?
> And the second one should be factored out as an argument for both opt.start and the
> loop below?

Oops, this is the only place, that I didn't clean up...

Yes, <opt_level> is excess: it is an artefact of juggling with options
for reproducing. Now it's quite clear that the issue relates to
allocation sinking optimization, that requires all flags to be enabled.

Regarding the <hotloop> value, I dropped a verbose comment. Hope it
makes the situation clearer, diff is below:

================================================================================

diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
index 8aad3438..f037c898 100644
--- a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
+++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
@@ -51,7 +51,35 @@ end
 
 ----- Reproducer. ------------------------------------------------
 
-jit.opt.start(3, 'hotloop=3')
+-- XXX: Here one can find the rationale for the 'hotloop' value.
+-- 1. The most inner while loop on the line 86 starts recording
+--    for the third element (i.e. 'c') and successfully compiles
+--    as TRACE 1. However, its execution stops, since type guard
+--    for <gen_x> result value on line 39 is violated (nil is
+--    returned from <ipairs_aux>) and trace execution is stopped.
+-- 2. Next time TRACE 1 enters the field is iterating through the
+--    second table given to <chain>. Its execution also stops at
+--    the similar assertion but in the variant part this time.
+-- 3. <wrap> function becomes hot enough while building new
+--    <chain> iterator, and it is compiled as TRACE 2.
+--    There are also other attempts, but all of them failed.
+-- 4. Again, TRACE 1 reigns while iterating through the first
+--    table given to <chain> and finishes at the same guard the
+--    previous run does. Anyway, everything above is just an
+--    auxiliary activity preparing the JIT environment for the
+--    following result.
+-- 5. Here we finally come: <chain_gen_r1> is finally ready to be
+--    recorded. It successfully compiles as TRACE 3. However, the
+--    boundary case is recorded, so the trace execution stops
+--    since nil *is not* returned from <ipairs_aux> on the next
+--    iteration.
+--
+-- JIT fine tuning via 'hotloop' option allows to catch this
+-- elusive case, we achieved in a last bullet. The reason, why
+-- this case leads to a misbehaviour while restoring the guest
+-- stack at the trace exit, is described in the following LuaJIT
+-- issue: https://github.com/LuaJIT/LuaJIT/issues/584.
+jit.opt.start('hotloop=3')
 
 xpcall(function()
   for _ = 1, 3 do

================================================================================

If you're OK with the comment, I'll proceed with the patch.

> 
> > +
> > +xpcall(function()
> > +  for _ = 1, 3 do
> > +    local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' })
> > +    while true do
> > +      state_x = gen_x(param_x, state_x)
> > +      if state_x == nil then break end
> > +    end
> > +  end
> > +  test:ok('All emitted RENAMEs are fine')
> > +end, function()
> > +  test:fail('Invalid Lua stack has been restored')
> > +end)
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.25.0
> > 
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Vitaliia Ioffe via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 8847 bytes --]


Hi all, 
 
QA LGTM
 
 
--
Vitaliia Ioffe
 
  
>Вторник, 3 августа 2021, 23:52 +03:00 от Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Sergos,
>
>Thanks for your review!
>
>On 27.07.21, Sergey Ostanevich wrote:
>> Hi! Thanks for the patch!
>>
>> Just a small nit to the test. I won’t comment Mike’s code :)
>
>The fix is much more clearer and simpler than the test for it...
>
>>
>> LGTM
>
>Added your tag:
>| Reviewed-by: Sergey Ostanevich < sergos@tarantool.org >
>
>>
>> Sergos
>>
>> > On 24 Jul 2021, at 20:23, Igor Munkin < imun@tarantool.org > wrote:
>> >
>> > From: Mike Pall <mike>
>> >
>> > Reported by Igor Munkin.
>> >
>> > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)
>> >
>> > Side exits with the same exitno use the same snapshot for restoring
>> > guest stack values. This obliges all guards related to the particular
>> > snapshot use the same RegSP mapping for the values to be restored at the
>> > trace exit. RENAME emitted prior to the guard for the same snapshot
>> > leads to the aforementioned invariant violation. The easy way to save
>> > the snapshot consistency is spilling the renamed IR reference, that is
>> > done in scope of <asm_snap_checkrename>.
>> >
>> > However, the previous <asm_snap_checkrename> implementation considers
>> > only the IR references explicitly mentioned in the snapshot. E.g. if
>> > there is a sunk[1] object to be restored at the trace exit, and the
>> > renamed reference is a *STORE to that object, the spill slot is not
>> > allocated. As a result an invalid value is stored while unsinking that
>> > object at all corresponding side exits prior to the emitted renaming.
>> >
>> > To handle also those IR references implicitly used in the snapshot, all
>> > non-constant and non-sunk references are added to the Bloom filter (it's
>> > worth to mention that two hash functions are used to reduce collisions
>> > for the cases when the number of IR references emitted between two
>> > different snapshots exceeds the filter size). New <asm_snap_checkrename>
>> > implementation tests whether the renamed IR reference is in the filter
>> > and forces a spill slot for it as a result.
>> >
>> > [1]:  http://wiki.luajit.org/Allocation-Sinking-Optimization
>> >
>> > Igor Munkin:
>> > * added the description and the test for the problem
>> >
>> > Resolves tarantool/tarantool#5118
>> > Follows up tarantool/tarantool#4252
>> >
>> > Signed-off-by: Igor Munkin < imun@tarantool.org >
>> > ---
>> >
>> > Related issues:
>> > *  https://github.com/tarantool/tarantool/issues/5118
>> > *  https://github.com/tarantool/tarantool/issues/4252
>> > *  https://github.com/LuaJIT/LuaJIT/issues/584
>> > Branch:  https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
>> > CI:  https://github.com/tarantool/tarantool/commit/b35e2ee
>> >
>> > src/lj_asm.c | 25 ++++---
>> > ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
>> > 2 files changed, 81 insertions(+), 13 deletions(-)
>> > create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
>> >
>
><snipped>
>
>> > diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
>> > new file mode 100644
>> > index 00000000..8aad3438
>> > --- /dev/null
>> > +++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
>> > @@ -0,0 +1,69 @@
>> > +local tap = require('tap')
>> > +
>> > +local test = tap.test('lj-584-bad-renames-for-sunk-values')
>> > +test:plan(1)
>> > +
>> > +-- Test file to demonstrate LuaJIT assembler misbehaviour.
>> > +-- For more info, proceed to the issues:
>> > +-- *  https://github.com/LuaJIT/LuaJIT/issues/584
>> > +-- *  https://github.com/tarantool/tarantool/issues/4252
>> > +
>> > +----- Related part of luafun.lua. --------------------------------
>> > +
>> > +local iterator_mt = {
>> > + __call = function(self, param, state) return self.gen(param, state) end,
>> > +}
>> > +
>> > +local wrap = function(gen, param, state)
>> > + return setmetatable({
>> > + gen = gen,
>> > + param = param,
>> > + state = state
>> > + }, iterator_mt), param, state
>> > +end
>> > +
>> > +-- These functions call each other to implement a flat iterator
>> > +-- over the several iterable objects.
>> > +local chain_gen_r1, chain_gen_r2
>> > +
>> > +chain_gen_r2 = function(param, state, state_x, ...)
>> > + if state_x ~= nil then return { state[1], state_x }, ... end
>> > + local i = state[1] + 1
>> > + if param[3 * i - 1] == nil then return nil end
>> > + return chain_gen_r1(param, { i, param[3 * i] })
>> > +end
>> > +
>> > +chain_gen_r1 = function(param, state)
>> > + local i, state_x = state[1], state[2]
>> > + local gen_x, param_x = param[3 * i - 2], param[3 * i - 1]
>> > + return chain_gen_r2(param, state, gen_x(param_x, state_x))
>> > +end
>> > +
>> > +local chain = function(...)
>> > + local param = { }
>> > + for i = 1, select('#', ...) do
>> > + -- Put gen, param, state into param table.
>> > + param[3 * i - 2], param[3 * i - 1], param[3 * i]
>> > + = wrap(ipairs(select(i, ...)))
>> > + end
>> > + return wrap(chain_gen_r1, param, { 1, param[3] })
>> > +end
>> > +
>> > +----- Reproducer. ------------------------------------------------
>> > +
>> > +jit.opt.start(3, 'hotloop=3')
>>
>> I don’t like both numbers here. opt_level is 3 by default - why bother setting it?
>> And the second one should be factored out as an argument for both opt.start and the
>> loop below?
>
>Oops, this is the only place, that I didn't clean up...
>
>Yes, <opt_level> is excess: it is an artefact of juggling with options
>for reproducing. Now it's quite clear that the issue relates to
>allocation sinking optimization, that requires all flags to be enabled.
>
>Regarding the <hotloop> value, I dropped a verbose comment. Hope it
>makes the situation clearer, diff is below:
>
>================================================================================
>
>diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
>index 8aad3438..f037c898 100644
>--- a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
>+++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
>@@ -51,7 +51,35 @@ end
> 
> ----- Reproducer. ------------------------------------------------
> 
>-jit.opt.start(3, 'hotloop=3')
>+-- XXX: Here one can find the rationale for the 'hotloop' value.
>+-- 1. The most inner while loop on the line 86 starts recording
>+-- for the third element (i.e. 'c') and successfully compiles
>+-- as TRACE 1. However, its execution stops, since type guard
>+-- for <gen_x> result value on line 39 is violated (nil is
>+-- returned from <ipairs_aux>) and trace execution is stopped.
>+-- 2. Next time TRACE 1 enters the field is iterating through the
>+-- second table given to <chain>. Its execution also stops at
>+-- the similar assertion but in the variant part this time.
>+-- 3. <wrap> function becomes hot enough while building new
>+-- <chain> iterator, and it is compiled as TRACE 2.
>+-- There are also other attempts, but all of them failed.
>+-- 4. Again, TRACE 1 reigns while iterating through the first
>+-- table given to <chain> and finishes at the same guard the
>+-- previous run does. Anyway, everything above is just an
>+-- auxiliary activity preparing the JIT environment for the
>+-- following result.
>+-- 5. Here we finally come: <chain_gen_r1> is finally ready to be
>+-- recorded. It successfully compiles as TRACE 3. However, the
>+-- boundary case is recorded, so the trace execution stops
>+-- since nil *is not* returned from <ipairs_aux> on the next
>+-- iteration.
>+--
>+-- JIT fine tuning via 'hotloop' option allows to catch this
>+-- elusive case, we achieved in a last bullet. The reason, why
>+-- this case leads to a misbehaviour while restoring the guest
>+-- stack at the trace exit, is described in the following LuaJIT
>+-- issue:  https://github.com/LuaJIT/LuaJIT/issues/584 .
>+jit.opt.start('hotloop=3')
> 
> xpcall(function()
>   for _ = 1, 3 do
>
>================================================================================
>
>If you're OK with the comment, I'll proceed with the patch.
>
>>
>> > +
>> > +xpcall(function()
>> > + for _ = 1, 3 do
>> > + local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' })
>> > + while true do
>> > + state_x = gen_x(param_x, state_x)
>> > + if state_x == nil then break end
>> > + end
>> > + end
>> > + test:ok('All emitted RENAMEs are fine')
>> > +end, function()
>> > + test:fail('Invalid Lua stack has been restored')
>> > +end)
>> > +
>> > +os.exit(test:check() and 0 or 1)
>> > --
>> > 2.25.0
>> >
>>
>
>--
>Best regards,
>IM
 

[-- Attachment #2: Type: text/html, Size: 11611 bytes --]

Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
Thanks for the patch!

Is there no point to simplify the test -- we have 5 different traces,
when really need the only one (with RENAME between two possible jump
to fallback branches with restoration from snapshot)?
Feel free to ignore.

Side note: also, we should test that sunk optimization still works,
shouldn't we?
Feel free to ignore.

Otherwise, LGTM.

On 24.07.21, Igor Munkin wrote:
> From: Mike Pall <mike>
> 
> Reported by Igor Munkin.
> 
> (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)
> 
> Side exits with the same exitno use the same snapshot for restoring
> guest stack values. This obliges all guards related to the particular
> snapshot use the same RegSP mapping for the values to be restored at the
> trace exit. RENAME emitted prior to the guard for the same snapshot
> leads to the aforementioned invariant violation.

This sentence a little bit unclear to me:

1) Leads how?
2) Do you mean it in terms of recording (i. e. the reverse instrucions
recording order) or not?

>                                                  The easy way to save

> the snapshot consistency is spilling the renamed IR reference, that is
> done in scope of <asm_snap_checkrename>.
> 
> However, the previous <asm_snap_checkrename> implementation considers
> only the IR references explicitly mentioned in the snapshot. E.g. if
> there is a sunk[1] object to be restored at the trace exit, and the
> renamed reference is a *STORE to that object, the spill slot is not
> allocated. As a result an invalid value is stored while unsinking that
> object at all corresponding side exits prior to the emitted renaming.
> 
> To handle also those IR references implicitly used in the snapshot, all
> non-constant and non-sunk references are added to the Bloom filter (it's
> worth to mention that two hash functions are used to reduce collisions
> for the cases when the number of IR references emitted between two
> different snapshots exceeds the filter size). New <asm_snap_checkrename>
> implementation tests whether the renamed IR reference is in the filter
> and forces a spill slot for it as a result.
> 
> [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization
> 
> Igor Munkin:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#5118
> Follows up tarantool/tarantool#4252
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Related issues:
> * https://github.com/tarantool/tarantool/issues/5118
> * https://github.com/tarantool/tarantool/issues/4252
> * https://github.com/LuaJIT/LuaJIT/issues/584
> Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
> CI: https://github.com/tarantool/tarantool/commit/b35e2ee
> 
>  src/lj_asm.c                                  | 25 ++++---
>  ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
>  2 files changed, 81 insertions(+), 13 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> 
> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index c2cf5a95..9267448e 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c

<snipped>

> diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> new file mode 100644
> index 00000000..8aad3438
> --- /dev/null
> +++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> @@ -0,0 +1,69 @@
> +local tap = require('tap')
> +
> +local test = tap.test('lj-584-bad-renames-for-sunk-values')
> +test:plan(1)
> +
> +-- Test file to demonstrate LuaJIT assembler misbehaviour.
> +-- For more info, proceed to the issues:
> +-- * https://github.com/LuaJIT/LuaJIT/issues/584
> +-- * https://github.com/tarantool/tarantool/issues/4252
> +
> +----- Related part of luafun.lua. --------------------------------
> +
> +local iterator_mt = {
> +  __call = function(self, param, state) return self.gen(param, state) end,
> +}
> +
> +local wrap = function(gen, param, state)
> +  return setmetatable({
> +    gen = gen,
> +    param = param,
> +    state = state
> +  }, iterator_mt), param, state
> +end
> +
> +-- These functions call each other to implement a flat iterator
> +-- over the several iterable objects.
> +local chain_gen_r1, chain_gen_r2
> +
> +chain_gen_r2 = function(param, state, state_x, ...)
> +  if state_x ~= nil then return { state[1], state_x }, ...  end
> +  local i = state[1] + 1
> +  if param[3 * i - 1] == nil then return nil end
> +  return chain_gen_r1(param, { i, param[3 * i] })
> +end
> +
> +chain_gen_r1 = function(param, state)
> +  local i, state_x = state[1], state[2]
> +  local gen_x, param_x = param[3 * i - 2], param[3 * i - 1]
> +  return chain_gen_r2(param, state, gen_x(param_x, state_x))
> +end
> +
> +local chain = function(...)
> +  local param = { }
> +  for i = 1, select('#', ...) do
> +    -- Put gen, param, state into param table.
> +    param[3 * i - 2], param[3 * i - 1], param[3 * i]
> +      = wrap(ipairs(select(i, ...)))
> +  end
> +  return wrap(chain_gen_r1, param, { 1, param[3] })
> +end
> +
> +----- Reproducer. ------------------------------------------------
> +
> +jit.opt.start(3, 'hotloop=3')
> +
> +xpcall(function()
> +  for _ = 1, 3 do
> +    local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' })
> +    while true do
> +      state_x = gen_x(param_x, state_x)
> +      if state_x == nil then break end
> +    end
> +  end
> +  test:ok('All emitted RENAMEs are fine')
> +end, function()
> +  test:fail('Invalid Lua stack has been restored')
> +end)
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 01.08.21, Sergey Kaplun wrote:
> Hi, Igor!
> Thanks for the patch!
> 
> Is there no point to simplify the test -- we have 5 different traces,
> when really need the only one (with RENAME between two possible jump
> to fallback branches with restoration from snapshot)?

Well... It was a tough issue to provide a stable reproducer to Mike.
Reducing this one is a much more complex issue: we need to compile a
trace for a loop with a rename emitted between two guards with the same
exitno in a variant part and leave the compiled loop via the guard at
the first iteration before the emitted RENAME. Sounds more complex than
even the existing test, doesn't it?

> Feel free to ignore.

Ignoring.

> 
> Side note: also, we should test that sunk optimization still works,
> shouldn't we?

Emm, nothing in the patch affects sink optimization per se, so if it's
OK without the patch, it's still OK with it.

> Feel free to ignore.

Ignoring.

> 
> Otherwise, LGTM.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> On 24.07.21, Igor Munkin wrote:
> > From: Mike Pall <mike>
> > 
> > Reported by Igor Munkin.
> > 
> > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)
> > 
> > Side exits with the same exitno use the same snapshot for restoring
> > guest stack values. This obliges all guards related to the particular
> > snapshot use the same RegSP mapping for the values to be restored at the
> > trace exit. RENAME emitted prior to the guard for the same snapshot
> > leads to the aforementioned invariant violation.
> 
> This sentence a little bit unclear to me:
> 
> 1) Leads how?

RENAME changes the effective register to be used prior to the particular
snapshot[1]. Hence if RENAME is emitted between the guards with the same
exitno, RegSP mapping is inconsistent for the former one.

> 2) Do you mean it in terms of recording (i. e. the reverse instrucions
> recording order) or not?

The trace is recorded in a direct order, but *assembled* in a reverse
order. I implies the latter one.

> 
> >                                                  The easy way to save
> 
> > the snapshot consistency is spilling the renamed IR reference, that is
> > done in scope of <asm_snap_checkrename>.
> > 
> > However, the previous <asm_snap_checkrename> implementation considers
> > only the IR references explicitly mentioned in the snapshot. E.g. if
> > there is a sunk[1] object to be restored at the trace exit, and the
> > renamed reference is a *STORE to that object, the spill slot is not
> > allocated. As a result an invalid value is stored while unsinking that
> > object at all corresponding side exits prior to the emitted renaming.
> > 
> > To handle also those IR references implicitly used in the snapshot, all
> > non-constant and non-sunk references are added to the Bloom filter (it's
> > worth to mention that two hash functions are used to reduce collisions
> > for the cases when the number of IR references emitted between two
> > different snapshots exceeds the filter size). New <asm_snap_checkrename>
> > implementation tests whether the renamed IR reference is in the filter
> > and forces a spill slot for it as a result.
> > 
> > [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization
> > 
> > Igor Munkin:
> > * added the description and the test for the problem
> > 
> > Resolves tarantool/tarantool#5118
> > Follows up tarantool/tarantool#4252
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > Related issues:
> > * https://github.com/tarantool/tarantool/issues/5118
> > * https://github.com/tarantool/tarantool/issues/4252
> > * https://github.com/LuaJIT/LuaJIT/issues/584
> > Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
> > CI: https://github.com/tarantool/tarantool/commit/b35e2ee
> > 
> >  src/lj_asm.c                                  | 25 ++++---
> >  ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
> >  2 files changed, 81 insertions(+), 13 deletions(-)
> >  create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> > 

<snipped>

> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: http://wiki.luajit.org/SSA-IR-2.0#miscellaneous-ops

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Igor Munkin via Tarantool-patches]

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in 1.10, 2.7, 2.8 and master.

On 24.07.21, Igor Munkin wrote:
> From: Mike Pall <mike>
> 
> Reported by Igor Munkin.
> 
> (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)
> 
> Side exits with the same exitno use the same snapshot for restoring
> guest stack values. This obliges all guards related to the particular
> snapshot use the same RegSP mapping for the values to be restored at the
> trace exit. RENAME emitted prior to the guard for the same snapshot
> leads to the aforementioned invariant violation. The easy way to save
> the snapshot consistency is spilling the renamed IR reference, that is
> done in scope of <asm_snap_checkrename>.
> 
> However, the previous <asm_snap_checkrename> implementation considers
> only the IR references explicitly mentioned in the snapshot. E.g. if
> there is a sunk[1] object to be restored at the trace exit, and the
> renamed reference is a *STORE to that object, the spill slot is not
> allocated. As a result an invalid value is stored while unsinking that
> object at all corresponding side exits prior to the emitted renaming.
> 
> To handle also those IR references implicitly used in the snapshot, all
> non-constant and non-sunk references are added to the Bloom filter (it's
> worth to mention that two hash functions are used to reduce collisions
> for the cases when the number of IR references emitted between two
> different snapshots exceeds the filter size). New <asm_snap_checkrename>
> implementation tests whether the renamed IR reference is in the filter
> and forces a spill slot for it as a result.
> 
> [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization
> 
> Igor Munkin:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#5118
> Follows up tarantool/tarantool#4252
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Related issues:
> * https://github.com/tarantool/tarantool/issues/5118
> * https://github.com/tarantool/tarantool/issues/4252
> * https://github.com/LuaJIT/LuaJIT/issues/584
> Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values
> CI: https://github.com/tarantool/tarantool/commit/b35e2ee
> 
>  src/lj_asm.c                                  | 25 ++++---
>  ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++
>  2 files changed, 81 insertions(+), 13 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua
> 

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM