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