<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM</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_16894339911477703348_BODY">Hi, Maxim!<br>Thanks for the review!<br>Fixed your comments, branch is force-pushed.<br><br>On 14.07.23, Maxim Kokryashkin wrote:<br>><br>> Hi!<br>> Thanks for the patch!<br>> LGTM, except for a few nits below.<br>> > <br>> >>From: Mike Pall <mike><br>> >><br>> >>Analyzed by Sergey Kaplun.<br>> >><br>> >>(cherry-picked from commit a01cba9d2d74efc57376822aa43db2d5043af5a4)<br>> >><br>> >>This patch is the follow-up for the previous one. It removes the<br>> >Typo: s/the/a<br>> >Typo: s/for the/to the/<br><br>Fixed, thanks!<br><br>> >>condition for `maxslot` changing in the case when varargs are defined<br>> >>on the trace (i.e. `framedepth` > 0).<br>> >Typo: s/i.e./i.e.,/<br><br>I prefer to avoid double punctuation symbols as mentioned here[1].<br>Also, this style is allowed by Oxford dictionary (as referenced in<br>[1]).<br><br>> >><br>> >>Sergey Kaplun:<br>> >>* added the description and the test for the problem<br>> >><br>> >>Part of tarantool/tarantool#8825<br>> >>---<br>> >> src/lj_record.c | 8 ++------<br>> >> .../lj-1024-varg-maxslot.test.lua | 19 ++++++++++++++++++-<br>> >> 2 files changed, 20 insertions(+), 7 deletions(-)<br>> >><br>> >>diff --git a/src/lj_record.c b/src/lj_record.c<br>> >>index 112524d3..02d9db9e 100644<br>> >>--- a/src/lj_record.c<br>> >>+++ b/src/lj_record.c<br>> >>@@ -1775,12 +1775,8 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)<br>> >> if (J->framedepth > 0) { /* Simple case: varargs defined on-trace. */<br>> >> ptrdiff_t i;<br>> >> if (nvararg < 0) nvararg = 0;<br>> >>- if (nresults == -1) {<br>> >>- nresults = nvararg;<br>> >>- J->maxslot = dst + (BCReg)nvararg;<br>> >>- } else if (dst + nresults > J->maxslot) {<br>> >>- J->maxslot = dst + (BCReg)nresults;<br>> >>- }<br>> >>+ if (nresults == -1) nresults = nvararg;<br>> >>+ J->maxslot = dst + (BCReg)nresults;<br>> >> for (i = 0; i < nresults; i++)<br>> >> J->base[dst+i] = i < nvararg ? getslot(J, i - nvararg - 1 - LJ_FR2) : TREF_NIL;<br>> >> } else { /* Unknown number of varargs passed to trace. */<br>> >>diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua<br>> >>index 14270595..f8d74e8a 100644<br>> >>--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua<br>> >>+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua<br>> >>@@ -3,7 +3,7 @@ local test = tap.test('lj-noticket-varg-usedef'):skipcond({<br>> >> ['Test requires JIT enabled'] = not jit.status(),<br>> >> })<br>> >> <br>> >>-test:plan(1)<br>> >>+test:plan(2)<br>> >> <br>> >> jit.opt.start('hotloop=1')<br>> >> <br>> >>@@ -20,4 +20,21 @@ end<br>> >> <br>> >> test:ok(true, 'BC_VARG recording 0th frame depth')<br>> >> <br>> >>+-- Now the same case, but with additional frame, so VARG slots<br>> >Typo: s/with/with an/<br><br>Fixed, thanks!<br><br>> >>+-- are defined on the trace.<br>> >>+local function bump_varg_frame(...)<br>> >Nit: Not sure about the name here, maybe it is better to call it<br>> >just `varg_frame` or something like that. Feel free to ignore, though.<br><br>Changed to the `varg_frame()`.<br><br>See the iterative patch below.<br><br>===================================================================<br>diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua<br>index f8d74e8a..cdefbe83 100644<br>--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua<br>+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua<br>@@ -20,9 +20,9 @@ end<br> <br> test:ok(true, 'BC_VARG recording 0th frame depth')<br> <br>--- Now the same case, but with additional frame, so VARG slots<br>+-- Now the same case, but with an additional frame, so VARG slots<br> -- are defined on the trace.<br>-local function bump_varg_frame(...)<br>+local function varg_frame(...)<br> -- BC_VARG 1 1 0. `...` is nil (argument for the script).<br> -- luacheck: ignore<br> -- XXX: some condition to use several slots on the Lua stack.<br>@@ -32,7 +32,7 @@ end<br> counter = 0<br> while counter < 3 do<br> counter = counter + 1<br>- bump_varg_frame()<br>+ varg_frame()<br> end<br> <br> test:ok(true, 'BC_VARG recording with defined on trace VARG slots')<br>===================================================================<br><br>> >>+ -- BC_VARG 1 1 0. `...` is nil (argument for the script).<br>> >>+ -- luacheck: ignore<br>> >>+ -- XXX: some condition to use several slots on the Lua stack.<br>> >>+ anchor = 1 >= 1, ...<br>> >>+end<br>> >>+<br>> >>+counter = 0<br>> >>+while counter < 3 do<br>> >>+ counter = counter + 1<br>> >>+ bump_varg_frame()<br>> >>+end<br>> >>+<br>> >>+test:ok(true, 'BC_VARG recording with defined on trace VARG slots')<br>> >>+<br>> >> os.exit(test:check() and 0 or 1)<br>> >>--<br>> >>2.34.1<br>> >--<br>> >Best regards,<br>> >Maxim Kokryashkin<br>> > <br><br>[1]: <a href="https://english.stackexchange.com/questions/352253/why-does-oxforddictionaries-com-not-use-a-comma-after-the-abbreviations-i-e-a" target="_blank">https://english.stackexchange.com/questions/352253/why-does-oxforddictionaries-com-not-use-a-comma-after-the-abbreviations-i-e-a</a><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>