Re: [Tarantool-patches] [PATCH luajit] Add NaN check to IR_NEWREF.

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

Hi, Sergey!
Thanks for the review!
Fixed your comment and force-pushed the branch.

On 18.11.23, Sergey Bronnikov wrote:
> Hello, Sergey,
> 
> 
> thanks for the patch. LGTM
> 
> See my comments below.
> 
> 
> On 11/16/23 11:49, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Peter Cawley.
> >
> > (cherry-picked from commit 7f9907b4ed0870ba64342bcc4b26cff0a94540da)
> >
> > When emitting IR NEWREF, there is no check for a non-NaN stored key
> > value. Thus, when the NaN number value is given to trace, it may be
> > stored as a key. This patch adds the corresponding check. If fold
> > optimization is enabled, this IR EQ check is dropped if it references
> > CONV IR from any (unsigned) integer type since NaN can be created via
> > conversion from an integer.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9145
> > ---
> >
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1069-newref-nan-key
> > Tarantool PR: https://github.com/tarantool/tarantool/pull/9374
> > Fuzzer link: https://oss-fuzz.com/testcase-detail/5251574662037504
> Updated https://github.com/tarantool/tarantool/wiki/Fuzzing

Thanks!

> > Relate issues:
> > * https://github.com/LuaJIT/LuaJIT/issues/1069
> > * https://github.com/tarantool/tarantool/issues/9145
> >
> >   src/lj_opt_fold.c                             |   5 +-
> >   src/lj_record.c                               |  12 +-
> >   .../lj-1069-newref-nan-key.test.lua           | 151 ++++++++++++++++++
> >   3 files changed, 164 insertions(+), 4 deletions(-)
> >   create mode 100644 test/tarantool-tests/lj-1069-newref-nan-key.test.lua
> >

<snipped>

> > +
> > +  local tset_table = table_new(0, NKEYS)
> > +
> > +  local function tset(t, k)
> > +    -- Value doesn't matter.
> > +    t[k] = true
> > +  end
> > +
> > +  -- Compile the function.
> > +  jit.opt.start('hotloop=1')
> > +
> > +  -- Use number keys to emit NEWREF.
> > +  tset(tset_table, 0.1)
> > +  tset(tset_table, 0.2)
> > +
> > +  -- Insert NaN on the trace.
> > +  local ok, err = pcall(tset, tset_table, NaN)
> > +
> > +  subtest:ok(not ok, 'function returns an error')
> > +  subtest:like(err, 'table index is NaN', 'correct error message')
> > +
> > +  local nkeys = 0
> > +  for k in pairs(tset_table) do
> > +    nkeys = nkeys + 1
> > +    subtest:ok(k == k, ('not NaN key by number %d'):format(nkeys))
> > +  end
> > +  subtest:is(nkeys, NKEYS - 1, 'correct amount of keys')
> > +end)
> > +
> > +test:test('NaN on trace in the non-constant IR CONV', function(subtest)
> > +  local tonumber = tonumber
> What for do you need this?

Added the following comment for clarification:

===================================================================
diff --git a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
index 22553423..67b4f5b3 100644
--- a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
+++ b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
@@ -52,7 +52,9 @@ test:test('NaN on trace in the non-constant IR', function(subtest)
 end)
 
 test:test('NaN on trace in the non-constant IR CONV', function(subtest)
+  -- XXX: simplify `jit.dump()` output.
   local tonumber = tonumber
+
   local NKEYS = 3
 
   -- XXX: NaN isn't stored, so the number of tests is:
===================================================================

Branch is force-pushed.

> > +  local NKEYS = 3
> > +

<snipped>

> 

-- 
Best regards,
Sergey Kaplun