[Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jul 6 12:40:13 MSK 2023
Hi, Sergey!
Thanks for review! See my answers below.
Updated version force-pushed to the branch.
S.
On 6/6/23 15:51, Sergey Kaplun wrote:
> 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,
>> 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.
Added a small explanation as well. More detailed explanation added to
the file with test.
<snipped>
>> diff --git a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> new file mode 100644
>> index 00000000..b3f1f78a
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> @@ -0,0 +1,27 @@
>> +local tap = require('tap')
>> +local test = tap.test('lj-819-fix-missing-uclo')
>> +
>> +test:plan(1)
>> +
>> +local function missing_uclo()
>> + while true do -- luacheck: ignore
>> + if false then
>> + break
> Please, comment why do we need this always false branch for reproducer
> (the aforementioned BC_UCLO).
>
> Also, examples of bytecode listings for this function before and after
> the patch are desirable.
Added a comment at the beginning of the test.
>
>> + end
>> + local f
>> + while true do
>> + if f then
>> + return f
> Please, comment, that exactly here we got not fixupped RET before the
> patch.
Added.
>
>> + end
>> + f = function()
>> + return f
>> + end
>> + end
>> + end
>> +end
>> +
>> +local f = missing_uclo()
>> +local res = f()
>> +test:ok(type(res) == 'function', 'type of returned value is correct')
> Minor: the comment why we don't get here a function, when upvalue isn't
> closed is desirable.
>
>> +
>> +os.exit(test:check() and 0 or 1)
> 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?
Added a second testcase and description for it.
>
>> --
>> 2.34.1
>>
More information about the Tarantool-patches
mailing list