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

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

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

Test requires jit and it failed on jobs without a JIT

Fixed!

On 7/6/23 14:31, Maxim Kokryashkin wrote:
> Hi!
> Thanks for the fixes!
> A few CI jobs are red, please address them.
> --
> Best regards,
> Maxim Kokryashkin
>
>         Hi, Max!
>
>         Thanks for review! Added more comments to the test and commit
>         message.
>
>         New changes force-pushed to the branch. Please take a look.
>
>         S.
>
>         On 6/7/23 14:35, Maxim Kokryashkin wrote:
>>         Hi, Sergey and Sergey!
>>
>>                 Hi, Sergey!
>>                 Thanks for the patch!
>>                 Please, consider my comments below.
>>
>>                 On 30.05.23, Sergey Bronnikov wrote:
>>                 > From: Sergey Bronnikov <sergeyb@tarantool.org>
>>                 >
>>                 > Contributed by XmiliaH.
>>                 >
>>                 > (cherry-picked from commit
>>                 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e)
>>                 >
>>                 > After emitting bytecode instruction BC_FNEW fixup
>>                 is not required,
>>
>>             Typo: s/bytecode/the bytecode
>>
>         Fixed, thanks!
>
>>                 > because FuncState will set a flag PROTO_CHILD that
>>                 will trigger emitting
>>                 > a pair of instructions BC_UCLO and BC_RET (see
>>                 <src/lj_parse.c:2355>)
>>                 > and BC_RET will close all upvalues from base equal
>>                 to 0.
>>
>>                 This part describes why replacing UCLO with FNEW is
>>                 good enough and
>>                 better than just deleting
>>                 | case BC_UCLO: return;
>>                 But the original problem is that some of BC_RET are
>>                 not fixup-ed, due to
>>                 early return, if UCLO is obtained before, those leads
>>                 to VM
>>                 inconsistency after return from the function. Please,
>>                 mention this too.
>>
>>             Agree here, it is hard to get what the patch is about
>>             from that description,
>>             without diving into the changes.
>>
>         Added more details.
>
>         <snipped>
>>
>>                 Also, before the patch I got the following assertion
>>                 in JIT:
>>
>>                 | 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
>>                 | f = missing_uclo()
>>                 | print(f())
>>                 | f = missing_uclo()
>>                 | print(f())
>>                 | '
>>                 | 3.1002202036551
>>                 | luajit:
>>                 /home/burii/reviews/luajit/lj-819-missing-uclo/src/lj_record.c:135:
>>                 rec_check_slots: Assertion `((((((tr))>>24) &
>>                 IRT_TYPE) - (TRef)(IRT_NUM) <= (TRef)
>>                 | (IRT_INT-IRT_NUM)))' failed.
>>                 | Aborted
>>
>>                 I don't sure that we should test this particular
>>                 failure too, since the
>>                 origin of the problem is the incorrect emitted bytecode.
>>
>>                 Thoughts?
>>
>>             We should not, because it is most likely caused by the issue
>>             that was fixed in the LuaJIT/LuaJIT@5c46f477.
>>
>         assert in rec_check_slots could be for many reasons, so I
>         added a testcase for compiler too.
>
>>
>>                 > --
>>                 > 2.34.1
>>                 >
>>
>>                 --
>>                 Best regards,
>>                 Sergey Kaplun
>>
>>             --
>>             Best regards,
>>             Maxim Kokryashkin
>>

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