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