[Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jul 6 12:43:22 MSK 2023
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
> </compose?To=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/ea107a54/attachment.htm>
More information about the Tarantool-patches
mailing list