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

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

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.

> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Signed-off-by: Sergey Bronnikov <sergeyb@tarantool.org>
> Co-authored-by: Sergey Kaplun <skaplun@tarantool.org>
> ---
> Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-819-fix-missing-uclo
> PR: https://github.com/tarantool/tarantool/pull/8689
> 
>  src/lj_parse.c                                |  2 +-
>  .../lj-819-fix-missing-uclo.test.lua          | 27 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
> 
> diff --git a/src/lj_parse.c b/src/lj_parse.c
> index af0dc53f..343fa797 100644
> --- a/src/lj_parse.c
> +++ b/src/lj_parse.c

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

> +    end
> +    local f
> +    while true do
> +      if f then
> +        return f

Please, comment, that exactly here we got not fixupped RET before the
patch.

> +      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?

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun