<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM, except for a single nit 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_16873378111114576116_BODY">On 21.06.23, Sergey Kaplun via Tarantool-patches wrote:<br>> Hi, Maxim!<br>> Thanks for the review!<br>><br>> On 14.06.23, Maxim Kokryashkin wrote:<br>> ><br>> > Hi, Sergey!<br>> > Thanks for the patch!<br>> > Please consider my comments below.<br>> > <br>> > > <br>> > >>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<br>> > >Typo: s/analizis/analysis/<br>><br>> Fixed! Thanks!<br>><br>> > >>no slots may considered as used. This leads to addititional SLOAD on<br>> > >Typo: s/may/may be/<br>> > >Typo: s/to additional/to an additional/<br>><br>> Fixed.<br>><br>> > >>trace with incorrect value used later. This patch disables the use-def<br>> > >Typo: s/trace with/the trace with an/<br>><br>> Fixed.<br>><br>> > >>analisis for BC_VARG as NIY.<br>> > >Typo: s/analisis/analysis/<br>><br>> Fixed, thanks!<br>><br>> > >><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>><br>> <snipped><br>><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<br>> > >Typo: s/this/these/<br>><br>> Fixed, thanks!</div></div></div></div></blockquote><div>Typo: s/this is/this/</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>> > >>+-- 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<br>> > >Why are they exactly 6 and 7? Please drop a comment.<br>><br>> No special meaning, added a comment.<br>><br>> > >>+<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)<br>> > >Are there any reasons behind the `fmod` choice? If so, please drop a comment.<br>><br>> No, added the comment.<br>><br>> > >>+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.<br>> > >Maybe it is worth mentioning Mike’s original comment[1] here.<br>> > >Feel free to ignore.<br>><br>> I just added the reference to the issue in the header, the comment above<br>> is about the same as Mike's but more verbose.<br>><br>> ===================================================================<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>> index c3ba65dd..3608ea4e 100644<br>> --- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua<br>> +++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua<br>> @@ -8,13 +8,13 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({<br>><br>> test:plan(1)<br>><br>> --- XXX: we don't really need to store this builtins, but this is<br>> +-- XXX: we don't really need to store these builtins, but this is<br>> -- 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>> +-- the call on trace (the last one call). No special meaning.<br>> local ARG_ON_RECORDING = 6<br>> local ON_TRACE_VALUE = ARG_ON_RECORDING + 1<br>><br>> @@ -42,23 +42,23 @@ local function varg(...)<br>> -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace<br>> -- OOM errors from helper functions.")<br>> local slot = ({...})[1]<br>> - -- Forcify stitch and usage of vararg slot.<br>> + -- Forcify stitch and usage of vararg slot. Any NIY is OK here.<br>> return fmod(ARG_ON_RECORDING, slot)<br>> end<br>><br>> jit.opt.start('hotloop=1')<br>><br>> local _, result<br>> -local function wrap(arg)<br>> +local function wrap(func, arg)<br>> -- `pcall()` is needed to emit snapshot to handle on-trace<br>> -- errors.<br>> - _, result = pcall(varg, arg)<br>> + _, result = pcall(func, arg)<br>> end<br>> -- Record trace with the 0 result.<br>> -wrap(ARG_ON_RECORDING)<br>> -wrap(ARG_ON_RECORDING)<br>> +wrap(varg, ARG_ON_RECORDING)<br>> +wrap(varg, ARG_ON_RECORDING)<br>> -- Record trace with the non-zero result.<br>> -wrap(ON_TRACE_VALUE)<br>> +wrap(varg, ON_TRACE_VALUE)<br><br>Brr, acturally, we need to separate two `wrap()` functions - to prevent<br>compilation for the `wrap()` itself as non pcall-ed fixed-arg function.<br>Added the comment.<br><br>><br>> test:ok(result ~= 0, 'use-def analysis for BC_VARG')<br>> ===================================================================<br>><br>> I also modify `wrap()` function to get the function to call considering<br>> your comments in the next patch.<br>><br>> > >>+ _, 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<br>> > >[1]: <a href="https://github.com/LuaJIT/LuaJIT/issues/704" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/704</a><br>> > >--<br>> > >Best regards,<br>> > >Maxim Kokryashkin<br>><br>> --<br>> Best regards,<br>> Sergey Kaplun<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>