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

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

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
> 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>
> index 00000000..ec28b274
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
> @@ -0,0 +1,151 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT misbehaviour for NEWREF IR
> +-- taken NaN value.
> +-- See also, https://github.com/LuaJIT/LuaJIT/issues/1069.
> +
> +local test = tap.test('lj-1069-newref-nan-key'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +local table_new = require('table.new')
> +local ffi = require('ffi')
> +
> +local NaN = tonumber('nan')
> +
> +test:plan(4)
> +
> +test:test('NaN on trace in the non-constant IR', function(subtest)
> +  local NKEYS = 3
> +
> +  -- XXX: NaN isn't stored, so the number of tests is:
> +  -- (NKEYS - 1) + test status + test error message + test keys
> +  -- amount.
> +  subtest:plan(NKEYS + 2)
> +
> +  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?
> +  local NKEYS = 3
> +
> +  -- XXX: NaN isn't stored, so the number of tests is:
> +  -- (NKEYS - 1) + test status + test error message + test keys
> +  -- amount.
> +  subtest:plan(NKEYS + 2)

BTW TAP reports "bad plan" with reverted fix due to a reason described 
in the comment above:

<snipped>

not ok - bad plan
   planned: 5
   run:  6

<snipped>

Nothing to do, just an observation.


<snipped>