Re: [Tarantool-patches] [PATCH luajit 2/2] Fix canonicalization of +-0.0 keys for IR_NEWREF.

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

Hi, Sergey!
Thanks for the review!
Please consider my comments below.

On 16.05.23, Sergey Bronnikov wrote:
> Hello, Sergey!
> 
> Thanks for the patch! To be honest I'm not proficient enough
> 
> for reviewing such patches, but I don't have objections, only a couple 

Never too old to learn!:)
Still, it is good for figuring out how LuaJIT works.

> of minor comments. See below.
> 
> To give more confidence with fix I have started fuzzer for luaL_loadbuffer
> 
> with applied patch (no crashes for a about 5 hours).

Thanks!

> 
> 
> Probably it is worth to mention that PUC Rio Lua has the same behaviour 
> when table has indices "-0" and "0":

I suppose this isn't related to the patch itself:
This is particularity of the parser.
Also, nether patch nor test is about -0 vs 0 parsing as table keys. (*)

> 
> [0] ~/sources/MRG/tarantool/third_party/luajit$ lua
> Lua 5.2.4  Copyright (C) 1994-2015 Lua.org, PUC-Rio
>  > a = {[0] = 1, [2] = 4, [-0] = 7}
>  > a[0]
> 7
>  >
> 
> I believe it is important because you will say that Lua semantics will 
> not broken after your patch.

Yes, it is good to have such test, I sure. But this isn't related to the
patch and its backporting.
So, ignoring for now.

> 
> Sergey
> 
> 

<snipped>

> > diff --git a/test/tarantool-tests/lj-981-folding-0.test.lua b/test/tarantool-tests/lj-981-folding-0.test.lua
> > new file mode 100644
> > index 00000000..251da24d
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-981-folding-0.test.lua
> > @@ -0,0 +1,57 @@
> > +local tap = require('tap')
> > +local test = tap.test('lj-981-folding-0'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> > +})
> > +
> > +-- Test file to demonstrate LuaJIT misbehaviour on load forwarding
> > +-- for -0 IR constant as table index.
> > +-- See also, https://github.com/LuaJIT/LuaJIT/issues/981.
> > +
> > +local jparse = require('utils.jit_parse')
> > +
> > +jit.opt.start('hotloop=1')
> 
> You changed global JIT settings,
> 
> it is a good habit to put everything back when test is finished.

No, actually -- its common approach for bugfix tests:
1) We know, that this file will be executed stand-alone, without
chain-evaluation with other tests, so this setting is applied only for
this particular test.
2) OTOH, for <test/tarantool-tests/misclib-getmetrics-lapi.test.lua> we
use exactly the suggested approach, since there are several JIT settings
to check for different subtests.

> 
> > +
> > +test:plan(4)
> > +
> > +-- Reset traces.
> > +jit.flush()
> > +
> > +jparse.start('i')
> > +local result
> > +local expected = 'result'
> > +-- TNEW:
> > +-- -0 isn't folded during parsing, so it will be set with KSHORT,
> > +-- UNM bytecodes. See <src/lj_parse.c> and bytecode listing
> > +-- for details.
> > +-- Because of it, empty table is created via TNEW.
> > +for _ = 1, 4 do
> > +  result = ({[-0] = expected})[0]
> > +end
> > +
> > +local traces = jparse.finish()
> > +
> > +-- Test that there is no any assertion failure.
> > +test:ok(result == expected, 'TNEW and -0 folding')
> > +-- Test that there is no NEWREF -0 IR.
> > +test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TNEW tab')
> > +
> > +-- Reset traces.
> > +jit.flush()
> > +
> > +jparse.start('i')
> > +-- TDUP:
> > +-- Now just add a constant field for the table to use TDUP with
> > +-- template table instead TNEW before -0 is set.
> > +for _ = 1, 4 do
> > +  result = ({[-0] = expected, [1] = 1})[0]
> > +end
> > +
> > +traces = jparse.finish()
> 
> 
> To be honest I think that chosen tables in tests are not representative. 
> I propose to take this one:
> 
> 
> local expected = 1
> local result
> for _ = 1, 4 do
>    result = ({[0] = 1, [-0] = 2})[0]
> end

I very strong against this approach, this is unrelated to the bug (*)
and confusing for reading.

> 
> assert(result == 2)
> 
> This example clearly demonstrates that element with index "0" was 
> superseded by element with index "-0".

As I said before, this is the good thing to do for checking parser or
Lua semantics correctness, not this particular JIT bug.

> 
> 
> > +
> > +-- Test that there is no any assertion failure.
> > +test:ok(result == expected, 'TDUP and -0 folding')
> > +-- Test that there is no NEWREF -0 IR.
> > +test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TDUP tab')
> > +
> > +os.exit(test:check() and 0 or 1)

-- 
Best regards,
Sergey Kaplun