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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>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: 6052 bytes --]