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

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

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

On 17.11.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a single nit and a question below.
> On Thu, Nov 16, 2023 at 11:49:59AM +0300, 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
> > 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>
> 
> > diff --git a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
> > new file mode 100644
> > index 00000000..ec28b274
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
> > @@ -0,0 +1,151 @@
> 
> > +-- Test the constant IR NaN value on the trace.
> Nit: This comment seems a bit redundant, as it duplicates the test name. Feel
> free to ignore.

It's also related to the second test. It's just like the section header
for both of the tests. I prefer to leave it as is.

> > +test:test('constant NaN on the trace', function(subtest)
> > +  -- Test the status and the error message.
> > +  subtest:plan(2)
> > +  local function protected()
> > +    local counter = 0
> > +    -- Use a number key to emit NEWREF.
> > +    local key = 0.1
> > +
> > +    jit.opt.start('hotloop=1')
> > +
> > +    while counter < 2 do
> > +      counter = counter + 1
> > +      -- luacheck: ignore
> > +      local tab = {_ = 'unused'}
> > +      tab[key] = 'NaN'
> > +      -- XXX: Use the constant NaN value on the trace.
> > +      key = 0/0
> > +    end
> > +  end
> > +
> > +  local ok, err = pcall(protected)
> > +  subtest:ok(not ok, 'function returns an error')
> > +  subtest:like(err, 'table index is NaN', 'NaN index error message')
> > +end)
> > +
> > +test:test('constant NaN on the trace and rehash of the table', function(subtest)
> > +  -- A little bit different test case: after rehashing the table,
> > +  -- the node is lost, and a hash part of the table isn't freed.
> > +  -- This leads to the assertion failure, which checks memory
> > +  -- leaks on shutdown.
> > +  -- XXX: The test didn't fail even before the patch. Check the
> > +  -- same values as in the previous test for consistency.
> What do you mean by "didn't fail"? AFAICS, it leads to the assertion failure.
> If you've meant no fails for a build with no assertions, then it worth
> clarifying it in this comment.

I mean that the checks below (`subtest:ok`, etc.) aren't fail.
I've rephrased as the following:

===================================================================
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 ec28b274..22553423 100644
--- a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
+++ b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
@@ -121,8 +121,8 @@ test:test('constant NaN on the trace and rehash of the table', function(subtest)
   -- the node is lost, and a hash part of the table isn't freed.
   -- This leads to the assertion failure, which checks memory
   -- leaks on shutdown.
-  -- XXX: The test didn't fail even before the patch. Check the
-  -- same values as in the previous test for consistency.
+  -- XXX: The test checks didn't fail even before the patch. Check
+  -- the same values as in the previous test for consistency.
   subtest:plan(2)
   local function protected()
     local counter = 0
===================================================================

Branch is force-pushed.

> 
> <snipped>

-- 
Best regards,
Sergey Kaplun