<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>Please consider my comments below.</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_16863034270219012433_BODY">From: Mike Pall <mike><br><br>Reported by Ryan Lucia.<br><br>(cherry-picked from commit 2801500a26084491ae035170cad4700513790890)<br><br>Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so</div></div></div></div></blockquote><div>Typo: s/analizis/analysis/</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>no slots may considered as used. This leads to addititional SLOAD on</div></div></div></div></blockquote><div>Typo: s/may/may be/</div><div>Typo: s/to additional/to an additional/</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>trace with incorrect value used later. This patch disables the use-def</div></div></div></div></blockquote><div>Typo: s/trace with/the trace with an/</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>analisis for BC_VARG as NIY.</div></div></div></div></blockquote><div>Typo: s/analisis/analysis/</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 Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#8516<br>Relates to tarantool/tarantool#8718<br>---<br> src/lj_snap.c | 4 +-<br> .../lj-704-bc-varg-use-def.test.lua | 65 +++++++++++++++++++<br> 2 files changed, 68 insertions(+), 1 deletion(-)<br> create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua<br><br>diff --git a/src/lj_snap.c b/src/lj_snap.c<br>index a8b49fcb..5bbe8498 100644<br>--- a/src/lj_snap.c<br>+++ b/src/lj_snap.c<br>@@ -267,7 +267,7 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,<br> if (!(op == BC_ISTC || op == BC_ISFC)) DEF_SLOT(bc_a(ins));<br> break;<br> case BCMbase:<br>- if (op >= BC_CALLM && op <= BC_VARG) {<br>+ if (op >= BC_CALLM && op <= BC_ITERN) {<br> BCReg top = (op == BC_CALLM || op == BC_CALLMT || bc_c(ins) == 0) ?<br> maxslot : (bc_a(ins) + bc_c(ins)+LJ_FR2);<br> if (LJ_FR2) DEF_SLOT(bc_a(ins)+1);<br>@@ -278,6 +278,8 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,<br> for (s = 0; s < bc_a(ins); s++) DEF_SLOT(s);<br> return 0;<br> }<br>+ } else if (op == BC_VARG) {<br>+ return maxslot; /* NYI: punt. */<br> } else if (op == BC_KNIL) {<br> for (s = bc_a(ins); s <= bc_d(ins); s++) DEF_SLOT(s);<br> } else if (op == BC_TSETM) {<br>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua<br>new file mode 100644<br>index 00000000..c3ba65dd<br>--- /dev/null<br>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua<br>@@ -0,0 +1,65 @@<br>+local tap = require('tap')<br>+-- Test file to demonstrate LuaJIT misbehaviour in use-def<br>+-- snapshot analysis for BC_VARG.<br>+-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/704" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/704</a>.<br>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({<br>+ ['Test requires JIT enabled'] = not jit.status(),<br>+})<br>+<br>+test:plan(1)<br>+<br>+-- XXX: we don't really need to store this builtins, but this is</div></div></div></div></blockquote><div>Typo: s/this/these/</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>+-- reduces `jitdump()` output for reader significantly.<br>+local fmod = math.fmod<br>+local pcall = pcall<br>+<br>+-- Use the 2 values for `fmod()` to produce non-zero value for<br>+-- the call on trace (the last one call).<br>+local ARG_ON_RECORDING = 6<br>+local ON_TRACE_VALUE = ARG_ON_RECORDING + 1</div></div></div></div></blockquote><div>Why are they exactly 6 and 7? Please drop a comment.</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>+-- The `jitdump()` output was like the following before the patch:<br>+-- 0003 > num SLOAD #1 T<br>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|----]<br>+-- 0004 } tab TNEW #3 #0<br>+-- 0005 > num SLOAD #4 T<br>+-- 0006 p32 FLOAD 0004 tab.array<br>+-- 0007 p32 AREF 0006 +1<br>+-- 0008 } num ASTORE 0007 0005<br>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0005]<br>+--<br>+-- The first snapshot misses the 0003 IR in the last slot to be<br>+-- used in the `fmod()` later, so it leads to the additional<br>+-- 0005 SLOAD #4, and storing it in the second snapshot.<br>+--<br>+-- The correct snapshot content after the patch is the following:<br>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|0003]<br>+-- ....<br>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0003]<br>+local function varg(...)<br>+ -- Generate snapshot after `pcall()` with missing slot.<br>+ -- The snapshot is generated before each TNEW after the commit<br>+ -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace<br>+ -- OOM errors from helper functions.")<br>+ local slot = ({...})[1]<br>+ -- Forcify stitch and usage of vararg slot.<br>+ return fmod(ARG_ON_RECORDING, slot)</div></div></div></div></blockquote><div>Are there any reasons behind the `fmod` choice? If so, please drop a comment.</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>+end<br>+<br>+jit.opt.start('hotloop=1')<br>+<br>+local _, result<br>+local function wrap(arg)<br>+ -- `pcall()` is needed to emit snapshot to handle on-trace<br>+ -- errors.</div></div></div></div></blockquote><div>Maybe it is worth mentioning Mike’s original comment[1] here.</div><div>Feel free to ignore.</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>+ _, result = pcall(varg, arg)<br>+end<br>+-- Record trace with the 0 result.<br>+wrap(ARG_ON_RECORDING)<br>+wrap(ARG_ON_RECORDING)<br>+-- Record trace with the non-zero result.<br>+wrap(ON_TRACE_VALUE)<br>+<br>+test:ok(result ~= 0, 'use-def analysis for BC_VARG')<br>+<br>+os.exit(test:check() and 0 or 1)<br>--<br>2.34.1</div></div></div></div></blockquote><div>[1]: <a href="https://github.com/LuaJIT/LuaJIT/issues/704">https://github.com/LuaJIT/LuaJIT/issues/704</a></div><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>