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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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@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
>>