[Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.
Sergey Kaplun
skaplun at tarantool.org
Tue Jun 6 15:51:20 MSK 2023
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.
>
> Sergey Bronnikov:
> * added the description and the test for the problem
>
> Signed-off-by: Sergey Bronnikov <sergeyb at tarantool.org>
> Co-authored-by: Sergey Kaplun <skaplun at 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
More information about the Tarantool-patches
mailing list