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

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

[Sergey Bronnikov via Tarantool-patches]

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.

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
@@ -1546,7 +1546,7 @@ static void fs_fixup_ret(FuncState *fs)
 	/* Replace with UCLO plus branch. */
 	fs->bcbase[pc].ins = BCINS_AD(BC_UCLO, 0, offset);
 	break;
-      case BC_UCLO:
+      case BC_FNEW:
 	return;  /* We're done. */
       default:
 	break;
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
+    end
+    local f
+    while true do
+      if f then
+        return f
+      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')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4108 bytes --]


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
>>> 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.
>>>
>>> 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?
>We should not, because it is most likely caused by the issue
>that was fixed in the LuaJIT/LuaJIT@5c46f477.
>>
>>> --
>>> 2.34.1
>>>
>>
>>--
>>Best regards,
>>Sergey Kaplun
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 5800 bytes --]

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

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3091 bytes --]

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
>         </compose?To=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
>

[-- Attachment #2: Type: text/html, Size: 7304 bytes --]

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]


Hi!
Thanks for the fixes!
A few CI jobs are red, please address them.
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>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
>>>> 
> 

[-- Attachment #2: Type: text/html, Size: 4968 bytes --]

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

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4138 bytes --]

Test requires jit and it failed on jobs without a JIT

Fixed!

On 7/6/23 14:31, Maxim Kokryashkin wrote:
> Hi!
> Thanks for the fixes!
> A few CI jobs are red, please address them.
> --
> Best regards,
> Maxim Kokryashkin
>
>         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
>>

[-- Attachment #2: Type: text/html, Size: 12264 bytes --]

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Test requires jit and it failed on jobs without a JIT
>>Fixed!
>>On 7/6/23 14:31, Maxim Kokryashkin wrote:
>>>Hi!
>>>Thanks for the fixes!
>>>A few CI jobs are red, please address them.
>>>--
>>>Best regards,
>>>Maxim Kokryashkin
>>> 
>>> 
>>>> 
>>>>>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
>>>>>>> 
>>>> 
> 

[-- Attachment #2: Type: text/html, Size: 6052 bytes --]

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

[Sergey Bronnikov via Tarantool-patches]

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, except a few nits and rewordings below.

> Fix BC_UCLO insertion for returns.
>
> Contributed by XmiliaH.
>
> (cherry-picked from commit 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e)
>
> Patch fixes a problem when LuaJIT generates a wrong bytecode with a
> missed BC_UCLO instruction. When some of BC_RET bytecode instructions are

Nit: commit line length is more than 72 symbols (see rationale here [1]).

> not fixup-ed, due to an early return, if UCLO is obtained before, those
> leads to VM inconsistency after return from the function.
>
> Patch makes the following changes in bytecode (thats it, emits extra

Typo: s/thats/that's/
Nit: I suggest to drop introductory phrase "thats it".


> BC_UCLO instruction that closes upvalues):
>
> @@ -11,11 +11,12 @@
>  0006 => LOOP     1 => 0012
>  0007    ISF          0
>  0008    JMP      1 => 0010
> -0009    RET1     0   2
> +0009    UCLO     0 => 0014
>  0010 => FNEW     0   0      ; uclo.lua:56
>  0011    JMP      1 => 0006
>  0012 => UCLO     0 => 0001
>  0013 => RET0     0   1
> +0014 => RET1     0   2

Side note: like this diff very much, good idea!

>
> NOTE: After emitting the bytecode instruction BC_FNEW fixup is not

Typo: s/NOTE: //

> 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 a base
> equal to 0.
>
> JIT compilation of missing_uclo() function without a patch with fix is failed:
> src/lj_record.c:135: rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.

I suppose there is no need to cite this assertion failure. So, I suggest
to reword this paragraph in the following way:

| JIT compilation of `missing_uclo()` function without a patch leads to
| assertion failure in `rec_check_slots()` due to Lua stack inconsistency.
| The root cause is still parsing misbehaviour. Nevertheless, the test
| case is added to be sure that the JIT compilation isn't affected.

Also, there is no need to mention me in the commit message :).

> (Thanks to Sergey Kaplun for discovering this!)
> Thus second testcase in a test covers a case with compilation as well.
>
> 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>
>
> 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
> @@ -1546,7 +1546,7 @@ static void fs_fixup_ret(FuncState *fs)
>  	/* Replace with UCLO plus branch. */
>  	fs->bcbase[pc].ins = BCINS_AD(BC_UCLO, 0, offset);
>  	break;
> -      case BC_UCLO:
> +      case BC_FNEW:
>  	return;  /* We're done. */
>        default:
>  	break;
> 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..942c22b2
> --- /dev/null
> +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
> @@ -0,0 +1,115 @@

Please, restrict comment length to the 66 symbols in the test.

> +local tap = require('tap')
> +-- Test contains a reproducer for a problem when LuaJIT generates a wrong
> +-- bytecode with a missed BC_UCLO instruction.
> +local test = tap.test('lj-819-fix-missing-uclo'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +-- Let's take a look at listings Listing 1 and Listing 2 below with bytecode

Typo: s/bytecode/the bytecode/

> +-- generated for a function missing_uclo() with and without a patch.

Minor: I suggest to use `` to line-up that this is functions names in
the code. Feel free to ignore.

> +-- Both listings contains two BC_UCLO instructions:

Typo: s/contains/contain/

> +-- - first one with id 0004 is generated for a statement 'break' inside

Typo: s/first/the first/

> +--   condition, see label BC_UCLO1;
> +-- - second one with id 0009 is generated for a statement 'return' inside

Typo: s/second/the second/

> +--   a nested loop, see label BC_UCLO2;
> +-- Both BC_UCLO's closes upvalues after leaving a function's scope.
> +--
> +-- The problem is happen when fs_fixup_ret() traverses bytecode instructions in

Typo: s/happen/happened/

> +-- a function prototype, meets first BC_UCLO instruction (break) and forgives a

Typo: s/first/the first/

> +-- second one (return). This leads to a wrong result produced by a function

Nit: I suggest the reword this in the following way:
| and misses the second one return to fixup

> +-- returned by missing_uclo() function. This also explains why do we need a
> +-- dead code in reproducer - without first BC_UCLO fs_fixup_ret() successfully

Typo: s/first/the first/

> +-- fixup BC_UCLO and problem does not appear.

Typo: s/problem/the problem/

> +--
> +-- Listing 1. Bytecode with a fix.
> +--
> +-- -- BYTECODE -- uclo.lua:1-59

I suggest to use listing from the test itself, i.e. use
lj-819-fix-missing-uclo.test.lua:79-97 (line numbers are used before
comment rewording and reallignment) here and below in headers.

> +-- 0001 => LOOP     0 => 0013
> +-- 0002    JMP      0 => 0003
> +-- 0003 => JMP      0 => 0005
> +-- 0004    UCLO     0 => 0013
> +-- 0005 => KPRI     0   0
> +-- 0006 => LOOP     1 => 0012
> +-- 0007    ISF          0
> +-- 0008    JMP      1 => 0010
> +-- 0009    UCLO     0 => 0014
> +-- 0010 => FNEW     0   0      ; uclo.lua:54

And lj-819-fix-missing-uclo.test.lua:92 here and below in listings.

> +-- 0011    JMP      1 => 0006
> +-- 0012 => UCLO     0 => 0001
> +-- 0013 => RET0     0   1
> +-- 0014 => RET1     0   2
> +--
> +-- Listing 2. Bytecode without a fix.
> +--
> +-- BYTECODE -- uclo.lua:1-59
> +-- 0001 => LOOP     0 => 0013
> +-- 0002    JMP      0 => 0003
> +-- 0003 => JMP      0 => 0005
> +-- 0004    UCLO     0 => 0013
> +-- 0005 => KPRI     0   0
> +-- 0006 => LOOP     1 => 0012
> +-- 0007    ISF          0
> +-- 0008    JMP      1 => 0010
> +-- 0009    UCLO     0 => 0014
> +-- 0010 => FNEW     0   0      ; uclo.lua:54
> +-- 0011    JMP      1 => 0006
> +-- 0012 => UCLO     0 => 0001
> +-- 0013 => RET0     0   1
> +-- 0014 => RET1     0   2
> +--
> +-- Listing 3. Changes in bytecode before and after a fix.
> +--
> +-- @@ -11,11 +11,12 @@
> +--  0006 => LOOP     1 => 0012
> +--  0007    ISF          0
> +--  0008    JMP      1 => 0010
> +-- -0009    RET1     0   2
> +-- +0009    UCLO     0 => 0014
> +--  0010 => FNEW     0   0      ; uclo.lua:56
> +--  0011    JMP      1 => 0006
> +--  0012 => UCLO     0 => 0001
> +--  0013 => RET0     0   1
> +-- +0014 => RET1     0   2
> +--
> +-- First testcase checks a correct bytecode generation by frontend

Typo: s/First/The first/
Typo: s/frontend/frontend,/

> +-- and the second testcase checks consistency on a JIT compilation.
> +
> +local function missing_uclo()
> +  while true do -- luacheck: ignore
> +    -- Attention: it is not a dead code, it is a part of reproducer.

Typo: s/reproducer/the reproducer/

I suggest the following rewording:
| XXX: it is not a dead code, it is a part of the reproducer, see the
| comment above.


> +    -- label: BC_UCLO1
> +    if false then
> +      break
> +    end
> +    local f
> +    while true do
> +      if f then
> +        -- label: BC_UCLO2
> +        return f
> +      end
> +      f = function()
> +        return f
> +      end
> +    end
> +  end
> +end
> +
> +local f = missing_uclo()
> +local res = f()
> +-- Without a patch we don't get here a function, because upvalue isn't closed

Typo: s/Without a patch/Without a patch,/

> +-- as desirable.
> +test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct')

Code width is more than 80 symbols.
Minor: s/virtual machine/VM/. Feel free to ignore.

> +
> +-- Make JIT compiler aggressive.

I suggest to drop this comment (there is no need to comment what
`hotloop=1` do, but if you want you may explain why we use 1 here (this
is still common practice in our tests, so I suggest just drop the
comment)).

> +jit.opt.start('hotloop=1')
> +
> +f = missing_uclo()
> +f()
> +f = missing_uclo()
> +local _
> +_, res = pcall(f)
> +test:ok(type(res) == 'function', 'consistency on compilation: type of returned value is correct')

Code width is more than 80 symbols.
Do we need pcall here?

Also, the test isn't failed with assertion failure as declared. But the
following one is:
| 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
|
| -- Function to pollute Lua stack.
| local function ret_arg(f) return f end
|
| f = missing_uclo()
| ret_arg(f())
| ret_arg(f())
| '

> +
> +os.exit(test:check() and 0 or 1)

[1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey!


thanks for review!


On 7/9/23 16:15, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, except a few nits and rewordings below.
>
>> Fix BC_UCLO insertion for returns.
>>
>> Contributed by XmiliaH.
>>
>> (cherry-picked from commit 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e)
>>
>> Patch fixes a problem when LuaJIT generates a wrong bytecode with a
>> missed BC_UCLO instruction. When some of BC_RET bytecode instructions are
> Nit: commit line length is more than 72 symbols (see rationale here [1]).


Fixed!

>
>> not fixup-ed, due to an early return, if UCLO is obtained before, those
>> leads to VM inconsistency after return from the function.
>>
>> Patch makes the following changes in bytecode (thats it, emits extra
> Typo: s/thats/that's/
> Nit: I suggest to drop introductory phrase "thats it".

Dropped.


>
>> BC_UCLO instruction that closes upvalues):
>>
>> @@ -11,11 +11,12 @@
>>   0006 => LOOP     1 => 0012
>>   0007    ISF          0
>>   0008    JMP      1 => 0010
>> -0009    RET1     0   2
>> +0009    UCLO     0 => 0014
>>   0010 => FNEW     0   0      ; uclo.lua:56
>>   0011    JMP      1 => 0006
>>   0012 => UCLO     0 => 0001
>>   0013 => RET0     0   1
>> +0014 => RET1     0   2
> Side note: like this diff very much, good idea!
>
>> NOTE: After emitting the bytecode instruction BC_FNEW fixup is not
> Typo: s/NOTE: //

Dropped.


>
>> 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 a base
>> equal to 0.
>>
>> JIT compilation of missing_uclo() function without a patch with fix is failed:
>> src/lj_record.c:135: rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.
> I suppose there is no need to cite this assertion failure. So, I suggest
> to reword this paragraph in the following way:
>
> | JIT compilation of `missing_uclo()` function without a patch leads to
> | assertion failure in `rec_check_slots()` due to Lua stack inconsistency.
> | The root cause is still parsing misbehaviour. Nevertheless, the test
> | case is added to be sure that the JIT compilation isn't affected.
>
> Also, there is no need to mention me in the commit message :).


Replaced with suggested paragraph. Thanks.


>
>> (Thanks to Sergey Kaplun for discovering this!)
>> Thus second testcase in a test covers a case with compilation as well.
>>
>> 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>
>>
>> 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
>> @@ -1546,7 +1546,7 @@ static void fs_fixup_ret(FuncState *fs)
>>   	/* Replace with UCLO plus branch. */
>>   	fs->bcbase[pc].ins = BCINS_AD(BC_UCLO, 0, offset);
>>   	break;
>> -      case BC_UCLO:
>> +      case BC_FNEW:
>>   	return;  /* We're done. */
>>         default:
>>   	break;
>> 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..942c22b2
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> @@ -0,0 +1,115 @@
> Please, restrict comment length to the 66 symbols in the test.

Updated.


+++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
@@ -1,31 +1,33 @@
  local tap = require('tap')
--- Test contains a reproducer for a problem when LuaJIT generates a wrong
--- bytecode with a missed BC_UCLO instruction.
+-- Test contains a reproducer for a problem when LuaJIT generates
+-- a wrong bytecode with a missed BC_UCLO instruction.
  local test = tap.test('lj-819-fix-missing-uclo'):skipcond({
    ['Test requires JIT enabled'] = not jit.status(),
  })

>> +local tap = require('tap')
>> +-- Test contains a reproducer for a problem when LuaJIT generates a wrong
>> +-- bytecode with a missed BC_UCLO instruction.
>> +local test = tap.test('lj-819-fix-missing-uclo'):skipcond({
>> +  ['Test requires JIT enabled'] = not jit.status(),
>> +})
>> +
>> +test:plan(2)
>> +
>> +-- Let's take a look at listings Listing 1 and Listing 2 below with bytecode
> Typo: s/bytecode/the bytecode/

Fixed.


>
>> +-- generated for a function missing_uclo() with and without a patch.
> Minor: I suggest to use `` to line-up that this is functions names in
> the code. Feel free to ignore.

Updated.


>
>> +-- Both listings contains two BC_UCLO instructions:
> Typo: s/contains/contain/

Updated.


>
>> +-- - first one with id 0004 is generated for a statement 'break' inside
> Typo: s/first/the first/

Fixed.


>
>> +--   condition, see label BC_UCLO1;
>> +-- - second one with id 0009 is generated for a statement 'return' inside
> Typo: s/second/the second/


Fixed.

>
>> +--   a nested loop, see label BC_UCLO2;
>> +-- Both BC_UCLO's closes upvalues after leaving a function's scope.
>> +--
>> +-- The problem is happen when fs_fixup_ret() traverses bytecode instructions in
> Typo: s/happen/happened/

Fixed.


>
>> +-- a function prototype, meets first BC_UCLO instruction (break) and forgives a
> Typo: s/first/the first/

Fixed.


>
>> +-- second one (return). This leads to a wrong result produced by a function
> Nit: I suggest the reword this in the following way:
> | and misses the second one return to fixup
>
>> +-- returned by missing_uclo() function. This also explains why do we need a
>> +-- dead code in reproducer - without first BC_UCLO fs_fixup_ret() successfully
> Typo: s/first/the first/

Fixed.


>
>> +-- fixup BC_UCLO and problem does not appear.
> Typo: s/problem/the problem/

Fixed.


>
>> +--
>> +-- Listing 1. Bytecode with a fix.
>> +--
>> +-- -- BYTECODE -- uclo.lua:1-59
> I suggest to use listing from the test itself, i.e. use
> lj-819-fix-missing-uclo.test.lua:79-97 (line numbers are used before
> comment rewording and reallignment) here and below in headers.
>
>> +-- 0001 => LOOP     0 => 0013
>> +-- 0002    JMP      0 => 0003
>> +-- 0003 => JMP      0 => 0005
>> +-- 0004    UCLO     0 => 0013
>> +-- 0005 => KPRI     0   0
>> +-- 0006 => LOOP     1 => 0012
>> +-- 0007    ISF          0
>> +-- 0008    JMP      1 => 0010
>> +-- 0009    UCLO     0 => 0014
>> +-- 0010 => FNEW     0   0      ; uclo.lua:54
> And lj-819-fix-missing-uclo.test.lua:92 here and below in listings.

Fixed here and below.


>
>> +-- 0011    JMP      1 => 0006
>> +-- 0012 => UCLO     0 => 0001
>> +-- 0013 => RET0     0   1
>> +-- 0014 => RET1     0   2
>> +--
>> +-- Listing 2. Bytecode without a fix.
>> +--
>> +-- BYTECODE -- uclo.lua:1-59
>> +-- 0001 => LOOP     0 => 0013
>> +-- 0002    JMP      0 => 0003
>> +-- 0003 => JMP      0 => 0005
>> +-- 0004    UCLO     0 => 0013
>> +-- 0005 => KPRI     0   0
>> +-- 0006 => LOOP     1 => 0012
>> +-- 0007    ISF          0
>> +-- 0008    JMP      1 => 0010
>> +-- 0009    UCLO     0 => 0014
>> +-- 0010 => FNEW     0   0      ; uclo.lua:54
>> +-- 0011    JMP      1 => 0006
>> +-- 0012 => UCLO     0 => 0001
>> +-- 0013 => RET0     0   1
>> +-- 0014 => RET1     0   2
>> +--
>> +-- Listing 3. Changes in bytecode before and after a fix.
>> +--
>> +-- @@ -11,11 +11,12 @@
>> +--  0006 => LOOP     1 => 0012
>> +--  0007    ISF          0
>> +--  0008    JMP      1 => 0010
>> +-- -0009    RET1     0   2
>> +-- +0009    UCLO     0 => 0014
>> +--  0010 => FNEW     0   0      ; uclo.lua:56
>> +--  0011    JMP      1 => 0006
>> +--  0012 => UCLO     0 => 0001
>> +--  0013 => RET0     0   1
>> +-- +0014 => RET1     0   2
>> +--
>> +-- First testcase checks a correct bytecode generation by frontend
> Typo: s/First/The first/
> Typo: s/frontend/frontend,/

Fixed.


>> +-- and the second testcase checks consistency on a JIT compilation.
>> +
>> +local function missing_uclo()
>> +  while true do -- luacheck: ignore
>> +    -- Attention: it is not a dead code, it is a part of reproducer.
> Typo: s/reproducer/the reproducer/

Fixed.


>
> I suggest the following rewording:
> | XXX: it is not a dead code, it is a part of the reproducer, see the
> | comment above.

Replaced.


>
>> +    -- label: BC_UCLO1
>> +    if false then
>> +      break
>> +    end
>> +    local f
>> +    while true do
>> +      if f then
>> +        -- label: BC_UCLO2
>> +        return f
>> +      end
>> +      f = function()
>> +        return f
>> +      end
>> +    end
>> +  end
>> +end
>> +
>> +local f = missing_uclo()
>> +local res = f()
>> +-- Without a patch we don't get here a function, because upvalue isn't closed
> Typo: s/Without a patch/Without a patch,/

Fixed.


>
>> +-- as desirable.
>> +test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct')
> Code width is more than 80 symbols.

Fixed.

@@ -98,11 +102,11 @@ end

  local f = missing_uclo()
  local res = f()
--- Without a patch we don't get here a function, because upvalue isn't 
closed
--- as desirable.
-test:ok(type(res) == 'function', 'virtual machine consistency: type of 
returned value is correct')
+-- Without a patch, we don't get here a function, because upvalue
+-- isn't closed as desirable.
+test:ok(type(res) == 'function',
+        'VM consistency: type of returned value is correct')


> Minor: s/virtual machine/VM/. Feel free to ignore.


Fixed.

>
>> +
>> +-- Make JIT compiler aggressive.
> I suggest to drop this comment (there is no need to comment what
> `hotloop=1` do, but if you want you may explain why we use 1 here (this
> is still common practice in our tests, so I suggest just drop the
> comment)).

Dropped.


>> +jit.opt.start('hotloop=1')
>> +
>> +f = missing_uclo()
>> +f()
>> +f = missing_uclo()
>> +local _
>> +_, res = pcall(f)
>> +test:ok(type(res) == 'function', 'consistency on compilation: type of returned value is correct')
> Code width is more than 80 symbols.

Fixed.

@@ -110,6 +114,7 @@ f()
  f = missing_uclo()
  local _
  _, res = pcall(f)
-test:ok(type(res) == 'function', 'consistency on compilation: type of 
returned value is correct')
+test:ok(type(res) == 'function',
+        'consistency on compilation: type of returned value is correct')

  os.exit(test:check() and 0 or 1)

> Do we need pcall here?


I would use it to avoid breaking test due to assert.

Without a pcall:


TAP version 13
1..2
not ok - VM consistency: type of returned value is correct
     filename:   eval
     line:       -1
     frame #1
       line:     0
       source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
       filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
       what:     main
       namewhat:
       src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
     frame #2
       line:     -1
       source:   =[C]
       filename: eval
       what:     C
       namewhat:
       src:      [C]
luajit: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_record.c:135: 
rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - 
(TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.
Aborted (core dumped)

With pcall:

TAP version 13
1..2
not ok - VM consistency: type of returned value is correct

    filename:   eval
     line:       -1
     frame #1
       line: 0
       source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
       filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
       what:     main
       namewhat:
       src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
     frame #2
       line:     -1
       source:   =[C]
       filename: eval
       what:     C
       namewhat:
       src:      [C]
not ok - consistency on compilation: type of returned value is correct
     filename:   eval
     line:       -1
     frame #1
       line:     0

<snipped>


I like second output more.

>
> Also, the test isn't failed with assertion failure as declared. But the
> following one is:
> | 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
> |
> | -- Function to pollute Lua stack.
> | local function ret_arg(f) return f end
> |
> | f = missing_uclo()
> | ret_arg(f())
> | ret_arg(f())
> | '
>
>> +
>> +os.exit(test:check() and 0 or 1)
> [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
>

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!

Still some thoughts about the `pcall()`.

On 10.07.23, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> 
> thanks for review!
> 
> 
> On 7/9/23 16:15, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the fixes!
> > LGTM, except a few nits and rewordings below.
> >

<snipped>

> @@ -110,6 +114,7 @@ f()
>   f = missing_uclo()
>   local _
>   _, res = pcall(f)
> -test:ok(type(res) == 'function', 'consistency on compilation: type of 
> returned value is correct')
> +test:ok(type(res) == 'function',
> +        'consistency on compilation: type of returned value is correct')
> 
>   os.exit(test:check() and 0 or 1)
> 
> > Do we need pcall here?
> 
> 
> I would use it to avoid breaking test due to assert.
> 
> Without a pcall:
> 
> 
> TAP version 13
> 1..2
> not ok - VM consistency: type of returned value is correct
>      filename:   eval
>      line:       -1
>      frame #1
>        line:     0
>        source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>        filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>        what:     main
>        namewhat:
>        src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>      frame #2
>        line:     -1
>        source:   =[C]
>        filename: eval
>        what:     C
>        namewhat:
>        src:      [C]
> luajit: 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_record.c:135: 
> rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - 
> (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.
> Aborted (core dumped)
> 
> With pcall:
> 
> TAP version 13
> 1..2
> not ok - VM consistency: type of returned value is correct
> 
>     filename:   eval
>      line:       -1
>      frame #1
>        line: 0
>        source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>        filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>        what:     main
>        namewhat:
>        src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>      frame #2
>        line:     -1
>        source:   =[C]
>        filename: eval
>        what:     C
>        namewhat:
>        src:      [C]
> not ok - consistency on compilation: type of returned value is correct
>      filename:   eval
>      line:       -1
>      frame #1
>        line:     0
> 
> <snipped>
> 
> 
> I like second output more.

Yes, but there is no trace related to the `f()` only for
`test:check()`:

| ---- TRACE 1 start tap.lua:33
| ---- TRACE 2 start 1/stitch tap.lua:34
| ---- TRACE 3 start tap.lua:16
| ---- TRACE 3 start tap.lua:80

So, with this `pcall()` we lose the JIT testing.

> 
> >
> > Also, the test isn't failed with assertion failure as declared. But the
> > following one is:
> > | 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
> > |
> > | -- Function to pollute Lua stack.
> > | local function ret_arg(f) return f end
> > |
> > | f = missing_uclo()
> > | ret_arg(f())
> > | ret_arg(f())
> > | '
> >
> >> +
> >> +os.exit(test:check() and 0 or 1)
> > [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
> >

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey!

On 7/13/23 10:57, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
>
> Still some thoughts about the `pcall()`.
>
> On 10.07.23, Sergey Bronnikov wrote:
>> Hi, Sergey!
<snipped>
>> @@ -110,6 +114,7 @@ f()
>>    f = missing_uclo()
>>    local _
>>    _, res = pcall(f)
>> -test:ok(type(res) == 'function', 'consistency on compilation: type of
>> returned value is correct')
>> +test:ok(type(res) == 'function',
>> +        'consistency on compilation: type of returned value is correct')
>>
>>    os.exit(test:check() and 0 or 1)
>>
>>> Do we need pcall here?
>>
>> I would use it to avoid breaking test due to assert.
>>
>> Without a pcall:
>>
>>
>> TAP version 13
>> 1..2
>> not ok - VM consistency: type of returned value is correct
>>       filename:   eval
>>       line:       -1
>>       frame #1
>>         line:     0
>>         source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         what:     main
>>         namewhat:
>>         src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>       frame #2
>>         line:     -1
>>         source:   =[C]
>>         filename: eval
>>         what:     C
>>         namewhat:
>>         src:      [C]
>> luajit:
>> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_record.c:135:
>> rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) -
>> (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.
>> Aborted (core dumped)
>>
>> With pcall:
>>
>> TAP version 13
>> 1..2
>> not ok - VM consistency: type of returned value is correct
>>
>>      filename:   eval
>>       line:       -1
>>       frame #1
>>         line: 0
>>         source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>         what:     main
>>         namewhat:
>>         src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>>       frame #2
>>         line:     -1
>>         source:   =[C]
>>         filename: eval
>>         what:     C
>>         namewhat:
>>         src:      [C]
>> not ok - consistency on compilation: type of returned value is correct
>>       filename:   eval
>>       line:       -1
>>       frame #1
>>         line:     0
>>
>> <snipped>
>>
>>
>> I like second output more.
> Yes, but there is no trace related to the `f()` only for
> `test:check()`:
>
> | ---- TRACE 1 start tap.lua:33
> | ---- TRACE 2 start 1/stitch tap.lua:34
> | ---- TRACE 3 start tap.lua:16
> | ---- TRACE 3 start tap.lua:80
>
> So, with this `pcall()` we lose the JIT testing.

With patch below JIT assertion is triggered:


--- a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
+++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
@@ -101,10 +101,10 @@ local function missing_uclo()
  end

  local f = missing_uclo()
-local res = f()
+local res_interp = f()
  -- Without a patch, we don't get here a function, because upvalue
  -- isn't closed as desirable.
-test:ok(type(res) == 'function',
+test:ok(type(res_interp) == 'function',
          'VM consistency: type of returned value is correct')

  jit.opt.start('hotloop=1')
@@ -112,9 +112,8 @@ jit.opt.start('hotloop=1')
  f = missing_uclo()
  f()
  f = missing_uclo()
-local _
-_, res = pcall(f)
-test:ok(type(res) == 'function',
+local _, res_jit = pcall(f)
+test:ok(type(res_jit) == 'function',
          'consistency on compilation: type of returned value is correct')

  os.exit(test:check() and 0 or 1)


In a 1:1 conversation we agreed that it is ok. I added patch to the 
branch and force-pushed.

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

[Sergey Kaplun via Tarantool-patches]

LGTM!

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! FYI, I changed Co-authored-by tag to Helped-by,
since the only author of the patch is Mike Pall.

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 30.05.23, Sergey Bronnikov via Tarantool-patches 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.
> 
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM