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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.
> +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.

<snipped>