Re: [Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.

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

Hi, Sergey!

On 7/13/23 10:57, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
>
> Still some thoughts about the `pcall()`.
>
> On 10.07.23, Sergey Bronnikov wrote:
>> Hi, Sergey!
<snipped>
>> @@ -110,6 +114,7 @@ f()
>>    f = missing_uclo()
>>    local _
>>    _, res = pcall(f)
>> -test:ok(type(res) == 'function', 'consistency on compilation: type of
>> returned value is correct')
>> +test:ok(type(res) == 'function',
>> +        'consistency on compilation: type of returned value is correct')
>>
>>    os.exit(test:check() and 0 or 1)
>>
>>> Do we need pcall here?
>>
>> I would use it to avoid breaking test due to assert.
>>
>> Without a pcall:
>>
>>
>> TAP version 13
>> 1..2
>> not ok - VM consistency: type of returned value is correct
>>       filename:   eval
>>       line:       -1
>>       frame #1
>>         line:     0
>>         source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         what:     main
>>         namewhat:
>>         src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>       frame #2
>>         line:     -1
>>         source:   =[C]
>>         filename: eval
>>         what:     C
>>         namewhat:
>>         src:      [C]
>> luajit:
>> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_record.c:135:
>> rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) -
>> (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.
>> Aborted (core dumped)
>>
>> With pcall:
>>
>> TAP version 13
>> 1..2
>> not ok - VM consistency: type of returned value is correct
>>
>>      filename:   eval
>>       line:       -1
>>       frame #1
>>         line: 0
>>         source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         what:     main
>>         namewhat:
>>         src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>       frame #2
>>         line:     -1
>>         source:   =[C]
>>         filename: eval
>>         what:     C
>>         namewhat:
>>         src:      [C]
>> not ok - consistency on compilation: type of returned value is correct
>>       filename:   eval
>>       line:       -1
>>       frame #1
>>         line:     0
>>
>> <snipped>
>>
>>
>> I like second output more.
> Yes, but there is no trace related to the `f()` only for
> `test:check()`:
>
> | ---- TRACE 1 start tap.lua:33
> | ---- TRACE 2 start 1/stitch tap.lua:34
> | ---- TRACE 3 start tap.lua:16
> | ---- TRACE 3 start tap.lua:80
>
> So, with this `pcall()` we lose the JIT testing.

With patch below JIT assertion is triggered:


--- a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
+++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
@@ -101,10 +101,10 @@ local function missing_uclo()
  end

  local f = missing_uclo()
-local res = f()
+local res_interp = f()
  -- Without a patch, we don't get here a function, because upvalue
  -- isn't closed as desirable.
-test:ok(type(res) == 'function',
+test:ok(type(res_interp) == 'function',
          'VM consistency: type of returned value is correct')

  jit.opt.start('hotloop=1')
@@ -112,9 +112,8 @@ jit.opt.start('hotloop=1')
  f = missing_uclo()
  f()
  f = missing_uclo()
-local _
-_, res = pcall(f)
-test:ok(type(res) == 'function',
+local _, res_jit = pcall(f)
+test:ok(type(res_jit) == 'function',
          'consistency on compilation: type of returned value is correct')

  os.exit(test:check() and 0 or 1)


In a 1:1 conversation we agreed that it is ok. I added patch to the 
branch and force-pushed.