

<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>A few CI jobs are red, please address them.</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</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_16886366040095669465_BODY"><div class="cl_683199"><p>Hi, Max!</p><p> </p><p>Thanks for review! Added more comments to the test and commit message.</p><p>New changes force-pushed to the branch. Please take a look.</p><p> </p><p>S.</p><div class="moz-cite-prefix_mr_css_attr">On 6/7/23 14:35, Maxim Kokryashkin wrote:</div><blockquote type="cite"><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_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div id="style_16860561301802465383_BODY_mr_css_attr">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>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></div></blockquote></blockquote><p>Fixed, thanks!</p><p> </p><blockquote type="cite"><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><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><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></div></blockquote></blockquote><p>Added more details.</p><p> </p><snipped><blockquote type="cite"><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><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>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></div></blockquote></blockquote><p> </p><p>assert in rec_check_slots could be for many reasons, so I added a testcase for compiler too.</p><p> </p><blockquote type="cite"><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><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><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></blockquote></div></div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>