Re: [Tarantool-patches] [PATCH luajit v3 4/4] Fix IR_RENAME snapshot number. Follow-up fix for a32aeadc.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the adding test-case!
LGTM, after fixing comments below.

On 28.03.23, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Reported by Victor Bombi, analyzed by XmiliaH. Thanks!
> 
> (cherry-picked from commit bf51d3535109c4745bfbbe19a5587a9eac00259a)
> 
> If the `snapalloc` flag is set, then the allocation hasn't
> occurred yet, meaning that rename is applied to the next
> snapshot. Otherwise, refs are already allocated and rename
> is applied to the current snapshot.
> 
> Maxim Kokryashkin:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#7745
> Part of tarantool/tarantool#8069
> ---
>  src/lj_asm.c                                  |  9 ++++++++-
>  .../gh-7745-ir-rename.test.lua                | 20 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/gh-7745-ir-rename.test.lua
> 
> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index adfaf286..929a6da6 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c

<snipped>

> diff --git a/test/tarantool-tests/gh-7745-ir-rename.test.lua b/test/tarantool-tests/gh-7745-ir-rename.test.lua
> new file mode 100644
> index 00000000..d03f6da5
> --- /dev/null
> +++ b/test/tarantool-tests/gh-7745-ir-rename.test.lua

Please, rename to mention the original issue, from LuaJIT repo.
Something like the following:
<lj-688-snap-ir-rename.test.lua>

> @@ -0,0 +1,20 @@
> +local tap = require('tap')
> +local test = tap.test('IR_RENAME on snapshot allocation'):skipcond({

Minor: I suggest to name test the same as its filename (just to be
consistent with other tests).

> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +test:plan(1)
> +
> +jit.opt.start('hotloop=1')
> +
> +local vals = {-0.1, 0.1, -0.1, 0.1}
> +local i = 1
> +local _
> +while i < 5 do
> +    assert(i ~= 1.1)
> +    local l1 = vals[i]
> +    _ = l1 > 0
> +    i = i + 1

Please, add comments near the lines emitted to interesting for us SNAP
and RENAME.

> +end
> +
> +test:ok(true, 'IR_RENAME is fine')
> +os.exit(test:check())

Minor: The comment about missing `and 0 or 1` is desirable.

> -- 
> 2.37.1 (Apple Git-137.1)
> 

-- 
Best regards,
Sergey Kaplun