<HTML><BODY><div>Hi, Sergey!</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;">Понедельник, 20 ноября 2023, 14:03 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_17004782110096999510_BODY">Hi, Maxim!<br>Thanks for the review!<br>Please consider my answers below.<br><br>On 17.11.23, Maxim Kokryashkin wrote:<br>> Hi, Sergey!<br>> Thanks for the patch!<br>> Please consider my comments below.<br>><br>> On Mon, Nov 13, 2023 at 06:05:01PM +0300, Sergey Kaplun wrote:<br>> > From: Mike Pall <mike><br>> ><br>> > Reported by XmiliaH.<br>> ><br>> > (cherry-picked from commit c8bcf1e5fb8eb72c7e35604fdfd27bba512761bb)<br>> ><br>> > `fold_abc_k()` doesn't patch the first ABC check when the right constant<br>> > operand is negative. This leads to out-of-bounds access from the array<br>> > on a trace. This patch casts to uint32_t the operands to compare. If the<br>> It would be right to paraphrase this sentence like this:<br>> "This patch casts the operands to uint32_t for comparison."<br><br>Replaced, thanks.<br><br>> > right IR contains a negative integer, the second IR will always be<br>> > patched. Also, because the ABC check on the trace is unordered, this<br>> > guard will always fail.<br>> ><br>> > Also, this fold rule creates new instructions that reference operands<br>> > across PHIs. This opens the room for other optimizations (like DCE), so<br>> > some guards become eliminated, and we use out-of-bounds access from the<br>> > array part of the table on trace. This patch adds the missing<br>> > `PHIBARRIER()` check.<br>> ><br>> > Sergey Kaplun:<br>> > * added the description and the test for the problem<br>> ><br>> > Part of tarantool/tarantool#9145<br>> > ---<br>> > Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-794-abc-fold-constants" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-794-abc-fold-constants</a><br>> > Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/9364" target="_blank">https://github.com/tarantool/tarantool/pull/9364</a><br>> > Related issues:<br>> > * <a href="https://github.com/LuaJIT/LuaJIT/issues/794" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/794</a><br>> > * <a href="https://github.com/tarantool/tarantool/issues/9145" target="_blank">https://github.com/tarantool/tarantool/issues/9145</a><br>> ><br>> > src/lj_opt_fold.c | 5 +-<br>> > .../lj-794-abc-fold-constants.test.lua | 85 +++++++++++++++++++<br>> > 2 files changed, 88 insertions(+), 2 deletions(-)<br>> > create mode 100644 test/tarantool-tests/lj-794-abc-fold-constants.test.lua<br>> ><br>> > diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c<br>> > index 944a9ecc..6175f7c1 100644<br>> > --- a/src/lj_opt_fold.c<br>> > +++ b/src/lj_opt_fold.c<br>> <snipped><br>><br>> > diff --git a/test/tarantool-tests/lj-794-abc-fold-constants.test.lua b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua<br>> > new file mode 100644<br>> > index 00000000..f8609933<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua<br>> > @@ -0,0 +1,85 @@<br>> <snipped><br>><br>> > +-- Now test the second issue, when ABC optimization applies for<br>> > +-- operands across PHIs.<br>> > +<br>> > +-- XXX: Reset hotcounters to avoid collisions.<br>> > +jit.opt.start('hotloop=1')<br>> > +<br>> > +local tab_array = {}<br>> > +local small_tab = {MAGIC_UNUSED}<br>> > +local full_tab = {}<br>> > +<br>> > +-- First, create tables with different asizes, to be used in PHI.<br>> > +-- Create a large enough array part for the noticeable<br>> > +-- out-of-bounds access.<br>> > +for i = 1, 8 do<br>> > + full_tab[i] = MAGIC_UNUSED<br>> > +end<br>> > +<br>> > +-- We need 5 iterations to execute both the variant and the<br>> > +-- invariant parts of the trace below.<br>> > +for i = 1, 5 do<br>> > + -- On the 3rd iteration, the recording is started.<br>> > + if i > 3 then<br>> > + tab_array[i] = small_tab<br>> > + else<br>> > + tab_array[i] = full_tab<br>> > + end<br>> > +end<br>> > +<br>> > +local result<br>> > +local alias_tab = tab_array[1]<br>> > +-- Compile a trace.<br>> > +-- Run 5 iterations to execute both the variant and the invariant<br>> > +-- parts.<br>> > +for i = 1, 5 do<br>> > + local local_tab = alias_tab<br>> > + alias_tab = tab_array[i]<br>> > + -- Additional ABC check to fold.<br>> > + -- luacheck: ignore<br>> > + result = alias_tab[1]<br>> > + result = local_tab[8]<br>> > +end<br>><br>> The black magic that happens here with tables is hard to understand.<br>> Please drop a comment with a detailed explanations for why do we need<br>> this complex `tab_array` construction and what effects does this have on<br>> IRs.<br><br>Added the following comment for clarification:<br>Also, renaming `local_tab` -> `previous_tab` to avoid confusion and<br>emphasize that the table from the previous iteration, which ABC check IR<br>is used.<br><br>===================================================================<br>diff --git a/test/tarantool-tests/lj-794-abc-fold-constants.test.lua b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua<br>index f8609933..c69d395b 100644<br>--- a/test/tarantool-tests/lj-794-abc-fold-constants.test.lua<br>+++ b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua<br>@@ -55,6 +55,12 @@ for i = 1, 8 do<br> full_tab[i] = MAGIC_UNUSED<br> end<br> <br>+-- Now, store these tables in the array. The PHI should be used in<br>+-- the trace to distinguish asizes from the variant and the<br>+-- invariant parts of the loop for the future ABC check.<br>+-- Nevertheless, before the patch, the ABC IR and the<br>+-- corresponding PHI are folded via optimization. This leads to<br>+-- incorrect behaviour.<br> -- We need 5 iterations to execute both the variant and the<br> -- invariant parts of the trace below.<br> for i = 1, 5 do<br>@@ -72,12 +78,12 @@ local alias_tab = tab_array[1]<br> -- Run 5 iterations to execute both the variant and the invariant<br> -- parts.<br> for i = 1, 5 do<br>- local local_tab = alias_tab<br>+ local previous_tab = alias_tab<br> alias_tab = tab_array[i]<br> -- Additional ABC check to fold.<br> -- luacheck: ignore<br> result = alias_tab[1]<br>- result = local_tab[8]<br>+ result = previous_tab[8]<br> end<br> <br> test:is(result, nil, 'correct ABC constant rule across PHI')<br>===================================================================<br><br>> > +<br>> > +test:is(result, nil, 'correct ABC constant rule across PHI')<br>> > +<br>> > +test:done(true)<br>> > --<br>> > 2.42.0<br>> ><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>