

<HTML><BODY><div>Hi, Sergey and Sergey!</div><div> </div><div><div> </div></div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16860561301802465383_BODY">Hi, Sergey!<br>Thanks for the patch!<br>Please, consider my comments below.<br><br>On 30.05.23, Sergey Bronnikov wrote:<br>> From: Sergey Bronnikov <<a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a>><br>><br>> Contributed by XmiliaH.<br>><br>> (cherry-picked from commit 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e)<br>><br>> After emitting bytecode instruction BC_FNEW fixup is not required,</div></div></div></div></blockquote><div>Typo: s/bytecode/the bytecode</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>> because FuncState will set a flag PROTO_CHILD that will trigger emitting<br>> a pair of instructions BC_UCLO and BC_RET (see <src/lj_parse.c:2355>)<br>> and BC_RET will close all upvalues from base equal to 0.<br><br>This part describes why replacing UCLO with FNEW is good enough and<br>better than just deleting<br>| case BC_UCLO: return;<br>But the original problem is that some of BC_RET are not fixup-ed, due to<br>early return, if UCLO is obtained before, those leads to VM<br>inconsistency after return from the function. Please, mention this too.</div></div></div></div></blockquote><div>Agree here, it is hard to get what the patch is about from that description,</div><div>without diving into the changes.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>><br>> Sergey Bronnikov:<br>> * added the description and the test for the problem<br>><br>> Signed-off-by: Sergey Bronnikov <<a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a>><br>> Co-authored-by: Sergey Kaplun <<a href="/compose?To=skaplun@tarantool.org">skaplun@tarantool.org</a>><br>> ---<br>> Branch: <a href="https://github.com/tarantool/luajit/tree/ligurio/gh-819-fix-missing-uclo" target="_blank">https://github.com/tarantool/luajit/tree/ligurio/gh-819-fix-missing-uclo</a><br>> PR: <a href="https://github.com/tarantool/tarantool/pull/8689" target="_blank">https://github.com/tarantool/tarantool/pull/8689</a><br>><br>> src/lj_parse.c | 2 +-<br>> .../lj-819-fix-missing-uclo.test.lua | 27 +++++++++++++++++++<br>> 2 files changed, 28 insertions(+), 1 deletion(-)<br>> create mode 100644 test/tarantool-tests/lj-819-fix-missing-uclo.test.lua<br>><br>> diff --git a/src/lj_parse.c b/src/lj_parse.c<br>> index af0dc53f..343fa797 100644<br>> --- a/src/lj_parse.c<br>> +++ b/src/lj_parse.c<br><br><snipped><br><br>> diff --git a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua<br>> new file mode 100644<br>> index 00000000..b3f1f78a<br>> --- /dev/null<br>> +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua<br>> @@ -0,0 +1,27 @@<br>> +local tap = require('tap')<br>> +local test = tap.test('lj-819-fix-missing-uclo')<br>> +<br>> +test:plan(1)<br>> +<br>> +local function missing_uclo()<br>> + while true do -- luacheck: ignore<br>> + if false then<br>> + break<br><br>Please, comment why do we need this always false branch for reproducer<br>(the aforementioned BC_UCLO).<br><br>Also, examples of bytecode listings for this function before and after<br>the patch are desirable.<br><br>> + end<br>> + local f<br>> + while true do<br>> + if f then<br>> + return f<br><br>Please, comment, that exactly here we got not fixupped RET before the<br>patch.<br><br>> + end<br>> + f = function()<br>> + return f<br>> + end<br>> + end<br>> + end<br>> +end<br>> +<br>> +local f = missing_uclo()<br>> +local res = f()<br>> +test:ok(type(res) == 'function', 'type of returned value is correct')<br><br>Minor: the comment why we don't get here a function, when upvalue isn't<br>closed is desirable.<br><br>> +<br>> +os.exit(test:check() and 0 or 1)<br><br>Also, before the patch I got the following assertion in JIT:<br><br>| LUA_PATH="src/?.lua;;" src/luajit -Ohotloop=1 -e '<br>|<br>| local function missing_uclo()<br>| while true do -- luacheck: ignore<br>| local f<br>| if false then break end<br>| while true do<br>| if f then<br>| return f<br>| end<br>| f = function()<br>| return f<br>| end<br>| end<br>| end<br>| end<br>| f = missing_uclo()<br>| print(f())<br>| f = missing_uclo()<br>| print(f())<br>| '<br>| 3.1002202036551<br>| 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)<br>| (IRT_INT-IRT_NUM)))' failed.<br>| Aborted<br><br>I don't sure that we should test this particular failure too, since the<br>origin of the problem is the incorrect emitted bytecode.<br><br>Thoughts?</div></div></div></div></blockquote><div>We should not, because it is most likely caused by the issue</div><div>that was fixed in the LuaJIT/LuaJIT@5c46f477.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> --<br>> 2.34.1<br>><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div><div> </div></div></blockquote></BODY></HTML>