Re: [Tarantool-patches] [PATCH luajit 2/3] DUALNUM: Fix narrowing of unary minus.

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

[-- Attachment #1: Type: text/plain, Size: 4161 bytes --]

Hi, Sergey!

thanks for the fix! Bug is reproduced without fix (I've used incorrect C 
flag).

LGTM

Sergey

On 3/4/26 13:34, Sergey Kaplun wrote:
> Hi, Sergey!
>
> Thanks for the review!
> See my answers below.
>
> On 04.03.26, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for the patch! See my comments.
>>
>> Sergey
>>
>> On 3/2/26 10:52, Sergey Kaplun wrote:
>>> From: Mike Pall <mike>
>>>
>>> Reported by Sergey Kaplun.
>>>
>>> (cherry picked from commit b1cd2f83b5d085bb71368b87c91a461be77d4364)
>>>
>>> `lj_opt_narrow_unm()` in the DUALNUM mode narrows doubles too
>>> optimistic, missing 0 check. In that case, the narrowing of 0 is
>>> incorrect. This leads to the assertion failure in `rec_check_slots()`
>>> for the string obtained from the corresponding number.
>>>
>>> This patch fixes it by restricting the check of the given TValue.
>>>
>>> Sergey Kaplun:
>>> * added the description and the test for the problem
>>>
>>> Part of tarantool/tarantool#12134
>>> ---
>>>    src/lj_opt_narrow.c                           |  4 +-
>>>    ...lj-1418-dualnum-narrowing-minus-0.test.lua | 49 +++++++++++++++++++
>>>    2 files changed, 51 insertions(+), 2 deletions(-)
>>>    create mode 100644 test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
>>>
>>> diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
>>> index 6b6f20d3..6e3e9533 100644
>>> --- a/src/lj_opt_narrow.c
>>> +++ b/src/lj_opt_narrow.c
>>> @@ -553,9 +553,9 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
>>>      rc = conv_str_tonum(J, rc, vc);
>>>      if (tref_isinteger(rc)) {
>>>        uint32_t k = (uint32_t)numberVint(vc);
>>> -    if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) {
>>> +    if ((tvisint(vc) || k != 0) && k != 0x80000000u) {
>>>          TRef zero = lj_ir_kint(J, 0);
>>> -      if (!LJ_DUALNUM)
>>> +      if (!tvisint(vc))
>>>    	emitir(IRTGI(IR_NE), rc, zero);
>>>          return emitir(IRTGI(IR_SUBOV), zero, rc);
>>>        }
>>> diff --git a/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua b/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
>>> new file mode 100644
>>> index 00000000..84f17953
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
>>> @@ -0,0 +1,49 @@
>>> +local tap = require('tap')
>>> +
>>> +-- This test demonstrates LuaJIT's incorrect narrowing
>>> +-- optimization in the DUALNUM mode for 0.
>>> +-- Seealsohttps://github.com/LuaJIT/LuaJIT/issues/1418.
>>> +
>>> +local test = tap.test('lj-1418-dualnum-narrowing-minus-0'):skipcond({
>>> +  ['Test requires JIT enabled'] = not jit.status(),
>>> +})
>>> +
>> cannot reproduce an original bug with reverted fix.
>>
>> CMake configuration: CFLAGS=-DDUALNUM cmake -S . -B build
>> -DCMAKE_BUILD_TYPE=Debug
> LuaJIT should be configured like:
> | cmake -DLUAJIT_NUMMODE=2 # ...
>
> <snipped>
>
>>> +-- Reset hotcounts.
>>> +jit.opt.start('hotloop=1')
>>> +
>>> +-- Hot trace.
>>> +test_non_const_on_trace(2, 3)
>>> +-- Record trace, use non zero result value to record.
>> s/non zero/non-zero/
> Fixed, branch is force-pushed:
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua b/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
> index 84f17953..8f4185ef 100644
> --- a/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
> +++ b/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
> @@ -41,7 +41,7 @@ jit.opt.start('hotloop=1')
>   
>   -- Hot trace.
>   test_non_const_on_trace(2, 3)
> --- Record trace, use non zero result value to record.
> +-- Record trace, use non-zero result value to record.
>   test_non_const_on_trace(2, 3)
>   -- Misbehaviour on trace with result zero value.
>   test:is(test_non_const_on_trace(2, 1), '-0', 'correct non-const value on trace')
> ===================================================================
>
>>> +test_non_const_on_trace(2, 3)
>>> +-- Misbehaviour on trace with result zero value.
>>> +test:is(test_non_const_on_trace(2, 1), '-0', 'correct non-const value on trace')
>>> +
>>> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 5226 bytes --]