[Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jul 6 16:45:21 MSK 2023
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 at 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 at 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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20230706/371a8971/attachment.htm>
More information about the Tarantool-patches
mailing list