Re: [Tarantool-patches] [PATCH luajit] Fix ABC FOLD rule with constants.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hello, Sergey!

thanks for the patch! LGTM, see minor comments below.

Sergey

On 11/13/23 18:05, 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
> 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
IR output would be useful in a test, what do you think?
> 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
> @@ -1877,14 +1877,15 @@ LJFOLDF(abc_fwd)
>   LJFOLD(ABC any KINT)
>   LJFOLDF(abc_k)
>   {
> +  PHIBARRIER(fleft);
>     if (LJ_LIKELY(J->flags & JIT_F_OPT_ABC)) {
>       IRRef ref = J->chain[IR_ABC];
>       IRRef asize = fins->op1;
>       while (ref > asize) {
>         IRIns *ir = IR(ref);
>         if (ir->op1 == asize && irref_isk(ir->op2)) {
> -	int32_t k = IR(ir->op2)->i;
> -	if (fright->i > k)
> +	uint32_t k = (uint32_t)IR(ir->op2)->i;
> +	if ((uint32_t)fright->i > k)
>   	  ir->op2 = fins->op2;
>   	return DROPFOLD;
>         }
> 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 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's incorrect fold optimization
> +-- for Array Bound Check for constants.
> +-- ABC(asize, k1), ABC(asize k2) ==> ABC(asize, max(k1, k2)).
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/794.
> +
> +local test = tap.test('lj-794-abc-fold-constants'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +local MAGIC_UNUSED = 42

AFAIK we put all test-related stuff after "test:plan".

Feel free to ignore.

> +test:plan(2)
> +
> +local function abc_check_sign()
> +  local tab = {MAGIC_UNUSED}
> +  local return_value = 0
> +  local abc_limit = 1
> +  -- No need to run the loop on the first call. We will take
> +  -- the side exit anyway.
> +  for i = 1, 3 do
> +    -- Add an additional ABC check to be merged with.
> +    if i > 1 then
> +      -- luacheck: ignore
> +      return_value = tab[1]
> +      return_value = tab[abc_limit]
> +      -- XXX: Just use some negative number.
> +      abc_limit = -1000000

With -1 works too, I would replace -10^6 with -1 for simplification.



<snipped>