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