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

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

Hi, Sergey!
Thanks for the fixes!

Still some thoughts about the `pcall()`.

On 10.07.23, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> 
> thanks for review!
> 
> 
> On 7/9/23 16:15, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the fixes!
> > LGTM, except a few nits and rewordings below.
> >

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

> 
> >
> > Also, the test isn't failed with assertion failure as declared. But the
> > following one is:
> > | LUA_PATH="src/?.lua;;" src/luajit -Ohotloop=1 -e '
> > |
> > | local function missing_uclo()
> > |     while true do -- luacheck: ignore
> > |         local f
> > |         if false then break end
> > |         while true do
> > |             if f then
> > |                 return f
> > |             end
> > |             f = function()
> > |                 return f
> > |             end
> > |         end
> > |     end
> > | end
> > |
> > | -- Function to pollute Lua stack.
> > | local function ret_arg(f) return f end
> > |
> > | f = missing_uclo()
> > | ret_arg(f())
> > | ret_arg(f())
> > | '
> >
> >> +
> >> +os.exit(test:check() and 0 or 1)
> > [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
> >

-- 
Best regards,
Sergey Kaplun