[Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by XmiliaH.

(cherry-picked from commit bc1bdbf620f58f0978385828bc51272903601e17)

`bufput_append()` may fold `BUFHDR RESET` + `BUFPUT` IRs to `BUFHDR
APPEND` even if the right operand (`BUFSTR`) is the PHI. If it's not the
last IR in the `BUFSTR` chain, this may lead to an incorrect resulting
value in the buffer, which contains a longer string since `APPEND` is
used instead of `RESET`.

This patch adds the corresponding check inside the fold rule.

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-791-fold-bufhdr-append
Tarantool PR: https://github.com/tarantool/tarantool/pull/9369
Relate issues:
* https://github.com/LuaJIT/LuaJIT/issues/791
* https://github.com/tarantool/tarantool/issues/9145

 src/lj_opt_fold.c                             |  3 +-
 .../lj-791-fold-bufhdr-append.test.lua        | 54 +++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua

diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index 944a9ecc..910cbc14 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -584,7 +584,8 @@ LJFOLDF(bufput_append)
   if ((J->flags & JIT_F_OPT_FWD) &&
       !(fleft->op2 & IRBUFHDR_APPEND) &&
       fleft->prev == fright->op2 &&
-      fleft->op1 == IR(fright->op2)->op1) {
+      fleft->op1 == IR(fright->op2)->op1 &&
+      !(irt_isphi(fright->t) && IR(fright->op2)->prev)) {
     IRRef ref = fins->op1;
     IR(ref)->op2 = (fleft->op2 | IRBUFHDR_APPEND);  /* Modify BUFHDR. */
     IR(ref)->op1 = fright->op1;
diff --git a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
new file mode 100644
index 00000000..b2422159
--- /dev/null
+++ b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
@@ -0,0 +1,54 @@
+local tap = require('tap')
+
+-- Test file to demonstrate the incorrect LuaJIT's optimization
+-- `bufput_append()` for BUFPUT IR.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/791.
+
+local test = tap.test('lj-791-fold-bufhdr-append'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local EMPTY_STR = ''
+local prefix = 'Lu'
+local result
+
+jit.opt.start('hotloop=1')
+
+-- The interesting part of IRs is the following (non-GC64 mode):
+-- 0006     str BUFSTR 0005 0003
+-- 0007  >  str SLOAD  #2   T
+-- 0008     p32 BUFHDR [0x400004a0] RESET
+-- 0009     p32 BUFPUT 0008 "Lu"
+-- 0010     p32 BUFPUT 0009 0007
+-- 0011   + str BUFSTR 0010 0008
+-- 0012   + int ADD    0001 +1
+-- 0013  >  int LE     0012 +5
+-- 0014  >  --- LOOP ------------
+-- 0015     p32 BUFHDR [0x400004a0] RESET
+
+-- The instruction to be folded is the following:
+-- 0016     p32 BUFPUT 0015 0011
+--
+-- The 0011 operand is PHI, which is not the last IR in the BUFSTR
+-- chain (`ir->prev = REF_BIAS + 0006`). Folding this IR leads to
+-- this resulting IR:
+-- p32 BUFHDR 0010  APPEND
+-- Which appends to buffer instead of reseting, so the resulting
+-- string contains one more symbol.
+
+-- XXX: Use 5 iterations to run variant part of the loop.
+for _ = 1, 5 do
+  result = prefix .. 'a'
+  -- We need a non-constant string to be appended to prevent more
+  -- aggressive optimizations. Use an empty string for
+  -- convenience. Also, use a constant string in the first operand
+  -- in the concatenation operator for more readable `jit.dump`
+  -- output.
+  prefix = 'Lu' .. EMPTY_STR
+end
+
+test:is(result, 'Lua', 'skipped BUFPUT APPEND optimization for PHIs')
+
+test:done(true)
-- 
2.42.0

Re: [Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
LGTM, except for a few nits below.
On Tue, Nov 14, 2023 at 06:04:55PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by XmiliaH.
>
> (cherry-picked from commit bc1bdbf620f58f0978385828bc51272903601e17)
>
> `bufput_append()` may fold `BUFHDR RESET` + `BUFPUT` IRs to `BUFHDR
> APPEND` even if the right operand (`BUFSTR`) is the PHI. If it's not the
> last IR in the `BUFSTR` chain, this may lead to an incorrect resulting
> value in the buffer, which contains a longer string since `APPEND` is
> used instead of `RESET`.
>
> This patch adds the corresponding check inside the fold rule.
>
> 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-791-fold-bufhdr-append
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9369
> Relate issues:
> * https://github.com/LuaJIT/LuaJIT/issues/791
> * https://github.com/tarantool/tarantool/issues/9145
>
>  src/lj_opt_fold.c                             |  3 +-
>  .../lj-791-fold-bufhdr-append.test.lua        | 54 +++++++++++++++++++
>  2 files changed, 56 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
>
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> index 944a9ecc..910cbc14 100644
> --- a/src/lj_opt_fold.c
> +++ b/src/lj_opt_fold.c
<snipped>

> diff --git a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> new file mode 100644
> index 00000000..b2422159
> --- /dev/null
> +++ b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> @@ -0,0 +1,54 @@
<snipped>

> +-- XXX: Use 5 iterations to run variant part of the loop.
Typo: s/variant/the variant/
> +for _ = 1, 5 do
> +  result = prefix .. 'a'
> +  -- We need a non-constant string to be appended to prevent more
> +  -- aggressive optimizations. Use an empty string for
> +  -- convenience. Also, use a constant string in the first operand
> +  -- in the concatenation operator for more readable `jit.dump`
> +  -- output.
> +  prefix = 'Lu' .. EMPTY_STR
> +end
> +
> +test:is(result, 'Lua', 'skipped BUFPUT APPEND optimization for PHIs')
The test description is a bit misleading. At first glance it reads like
the `BUFPUT APPEND` itself is skipped. I suggest paraphrasing it like
this:
"BUFPUT APPEND optimization is not applied for PHIs"
> +
> +test:done(true)
> --
> 2.42.0
>

Re: [Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 17.11.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> LGTM, except for a few nits below.
> On Tue, Nov 14, 2023 at 06:04:55PM +0300, Sergey Kaplun wrote:

<snipped>

> > +-- XXX: Use 5 iterations to run variant part of the loop.
> Typo: s/variant/the variant/

Fixed.

> > +for _ = 1, 5 do
> > +  result = prefix .. 'a'
> > +  -- We need a non-constant string to be appended to prevent more
> > +  -- aggressive optimizations. Use an empty string for
> > +  -- convenience. Also, use a constant string in the first operand
> > +  -- in the concatenation operator for more readable `jit.dump`
> > +  -- output.
> > +  prefix = 'Lu' .. EMPTY_STR
> > +end
> > +
> > +test:is(result, 'Lua', 'skipped BUFPUT APPEND optimization for PHIs')
> The test description is a bit misleading. At first glance it reads like
> the `BUFPUT APPEND` itself is skipped. I suggest paraphrasing it like
> this:
> "BUFPUT APPEND optimization is not applied for PHIs"

Fixed, thanks!
See the iterative patch below:

===================================================================
diff --git a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
index b2422159..19be91a4 100644
--- a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
+++ b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
@@ -38,7 +38,7 @@ jit.opt.start('hotloop=1')
 -- Which appends to buffer instead of reseting, so the resulting
 -- string contains one more symbol.
 
--- XXX: Use 5 iterations to run variant part of the loop.
+-- XXX: Use 5 iterations to run the variant part of the loop.
 for _ = 1, 5 do
   result = prefix .. 'a'
   -- We need a non-constant string to be appended to prevent more
@@ -49,6 +49,6 @@ for _ = 1, 5 do
   prefix = 'Lu' .. EMPTY_STR
 end
 
-test:is(result, 'Lua', 'skipped BUFPUT APPEND optimization for PHIs')
+test:is(result, 'Lua', 'BUFPUT APPEND optimization is not applied for PHIs')
 
 test:done(true)
===================================================================

Branch is force-pushed.

> > +
> > +test:done(true)
> > --
> > 2.42.0
> >

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Sergey Bronnikov via Tarantool-patches]

Hello, Sergey

thanks for the patch! LGTM with a nit below.

On 11/14/23 18:04, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by XmiliaH.
>
> (cherry-picked from commit bc1bdbf620f58f0978385828bc51272903601e17)
>
> `bufput_append()` may fold `BUFHDR RESET` + `BUFPUT` IRs to `BUFHDR
> APPEND` even if the right operand (`BUFSTR`) is the PHI. If it's not the
> last IR in the `BUFSTR` chain, this may lead to an incorrect resulting
> value in the buffer, which contains a longer string since `APPEND` is
> used instead of `RESET`.
>
> This patch adds the corresponding check inside the fold rule.
>
> 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-791-fold-bufhdr-append
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9369
> Relate issues:
> * https://github.com/LuaJIT/LuaJIT/issues/791
> * https://github.com/tarantool/tarantool/issues/9145
>
>   src/lj_opt_fold.c                             |  3 +-
>   .../lj-791-fold-bufhdr-append.test.lua        | 54 +++++++++++++++++++
>   2 files changed, 56 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
>
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> index 944a9ecc..910cbc14 100644
> --- a/src/lj_opt_fold.c
> +++ b/src/lj_opt_fold.c
> @@ -584,7 +584,8 @@ LJFOLDF(bufput_append)
>     if ((J->flags & JIT_F_OPT_FWD) &&
>         !(fleft->op2 & IRBUFHDR_APPEND) &&
>         fleft->prev == fright->op2 &&
> -      fleft->op1 == IR(fright->op2)->op1) {
> +      fleft->op1 == IR(fright->op2)->op1 &&
> +      !(irt_isphi(fright->t) && IR(fright->op2)->prev)) {
>       IRRef ref = fins->op1;
>       IR(ref)->op2 = (fleft->op2 | IRBUFHDR_APPEND);  /* Modify BUFHDR. */
>       IR(ref)->op1 = fright->op1;
> diff --git a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> new file mode 100644
> index 00000000..b2422159
> --- /dev/null
> +++ b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> @@ -0,0 +1,54 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate the incorrect LuaJIT's optimization
> +-- `bufput_append()` for BUFPUT IR.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/791.
> +
> +local test = tap.test('lj-791-fold-bufhdr-append'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local EMPTY_STR = ''
> +local prefix = 'Lu'
> +local result
> +
> +jit.opt.start('hotloop=1')
> +
> +-- The interesting part of IRs is the following (non-GC64 mode):
> +-- 0006     str BUFSTR 0005 0003
> +-- 0007  >  str SLOAD  #2   T
> +-- 0008     p32 BUFHDR [0x400004a0] RESET
> +-- 0009     p32 BUFPUT 0008 "Lu"
> +-- 0010     p32 BUFPUT 0009 0007
> +-- 0011   + str BUFSTR 0010 0008
> +-- 0012   + int ADD    0001 +1
> +-- 0013  >  int LE     0012 +5
> +-- 0014  >  --- LOOP ------------
> +-- 0015     p32 BUFHDR [0x400004a0] RESET
> +
> +-- The instruction to be folded is the following:
> +-- 0016     p32 BUFPUT 0015 0011
> +--
> +-- The 0011 operand is PHI, which is not the last IR in the BUFSTR
> +-- chain (`ir->prev = REF_BIAS + 0006`). Folding this IR leads to
> +-- this resulting IR:
> +-- p32 BUFHDR 0010  APPEND
> +-- Which appends to buffer instead of reseting, so the resulting

After rebase on tarantool/master and running LuaJIT-codespell:

reseting ==> resetting

> +-- string contains one more symbol.
> +
> +-- XXX: Use 5 iterations to run variant part of the loop.
> +for _ = 1, 5 do
> +  result = prefix .. 'a'
> +  -- We need a non-constant string to be appended to prevent more
> +  -- aggressive optimizations. Use an empty string for
> +  -- convenience. Also, use a constant string in the first operand
> +  -- in the concatenation operator for more readable `jit.dump`
> +  -- output.
> +  prefix = 'Lu' .. EMPTY_STR
> +end
> +
> +test:is(result, 'Lua', 'skipped BUFPUT APPEND optimization for PHIs')
> +
> +test:done(true)

Re: [Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for review!
Fixed your comment, rebased on the current master and force-pushed the
branch.

On 24.11.23, Sergey Bronnikov wrote:
> Hello, Sergey
> 
> thanks for the patch! LGTM with a nit below.
> 
> On 11/14/23 18:04, Sergey Kaplun wrote:

<snipped>

> > +-- The instruction to be folded is the following:
> > +-- 0016     p32 BUFPUT 0015 0011
> > +--
> > +-- The 0011 operand is PHI, which is not the last IR in the BUFSTR
> > +-- chain (`ir->prev = REF_BIAS + 0006`). Folding this IR leads to
> > +-- this resulting IR:
> > +-- p32 BUFHDR 0010  APPEND
> > +-- Which appends to buffer instead of reseting, so the resulting
> 
> After rebase on tarantool/master and running LuaJIT-codespell:
> 
> reseting ==> resetting

Thanks, fixed!

===================================================================
diff --git a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
index 19be91a4..98b83ece 100644
--- a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
+++ b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
@@ -35,7 +35,7 @@ jit.opt.start('hotloop=1')
 -- chain (`ir->prev = REF_BIAS + 0006`). Folding this IR leads to
 -- this resulting IR:
 -- p32 BUFHDR 0010  APPEND
--- Which appends to buffer instead of reseting, so the resulting
+-- Which appends to buffer instead of resetting, so the resulting
 -- string contains one more symbol.
 
 -- XXX: Use 5 iterations to run the variant part of the loop.
===================================================================

> 
> > +-- string contains one more symbol.

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Sergey Bronnikov via Tarantool-patches]

Thanks! LGTM

On 11/27/23 13:38, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for review!
> Fixed your comment, rebased on the current master and force-pushed the
> branch.
>
> On 24.11.23, Sergey Bronnikov wrote:
>> Hello, Sergey
>>
>> thanks for the patch! LGTM with a nit below.
>>
>> On 11/14/23 18:04, Sergey Kaplun wrote:
> <snipped>
>
>>> +-- The instruction to be folded is the following:
>>> +-- 0016     p32 BUFPUT 0015 0011
>>> +--
>>> +-- The 0011 operand is PHI, which is not the last IR in the BUFSTR
>>> +-- chain (`ir->prev = REF_BIAS + 0006`). Folding this IR leads to
>>> +-- this resulting IR:
>>> +-- p32 BUFHDR 0010  APPEND
>>> +-- Which appends to buffer instead of reseting, so the resulting
>> After rebase on tarantool/master and running LuaJIT-codespell:
>>
>> reseting ==> resetting
> Thanks, fixed!
>
> ===================================================================
> diff --git a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> index 19be91a4..98b83ece 100644
> --- a/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> +++ b/test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> @@ -35,7 +35,7 @@ jit.opt.start('hotloop=1')
>   -- chain (`ir->prev = REF_BIAS + 0006`). Folding this IR leads to
>   -- this resulting IR:
>   -- p32 BUFHDR 0010  APPEND
> --- Which appends to buffer instead of reseting, so the resulting
> +-- Which appends to buffer instead of resetting, so the resulting
>   -- string contains one more symbol.
>   
>   -- XXX: Use 5 iterations to run the variant part of the loop.
> ===================================================================
>
>>> +-- string contains one more symbol.
> <snipped>
>

Re: [Tarantool-patches] [PATCH luajit] Fix FOLD rule for BUFHDR append.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 14.11.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Reported by XmiliaH.
> 
> (cherry-picked from commit bc1bdbf620f58f0978385828bc51272903601e17)
> 
> `bufput_append()` may fold `BUFHDR RESET` + `BUFPUT` IRs to `BUFHDR
> APPEND` even if the right operand (`BUFSTR`) is the PHI. If it's not the
> last IR in the `BUFSTR` chain, this may lead to an incorrect resulting
> value in the buffer, which contains a longer string since `APPEND` is
> used instead of `RESET`.
> 
> This patch adds the corresponding check inside the fold rule.
> 
> 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-791-fold-bufhdr-append
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9369
> Relate issues:
> * https://github.com/LuaJIT/LuaJIT/issues/791
> * https://github.com/tarantool/tarantool/issues/9145
> 
>  src/lj_opt_fold.c                             |  3 +-
>  .../lj-791-fold-bufhdr-append.test.lua        | 54 +++++++++++++++++++
>  2 files changed, 56 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-791-fold-bufhdr-append.test.lua
> 

<snipped>

> -- 
> 2.42.0
> 

-- 
Best regards,
IM