Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.
@ 2021-08-20 15:48 Sergey Kaplun via Tarantool-patches
  2021-08-31 11:39 ` Sergey Ostanevich via Tarantool-patches
  0 siblings, 1 reply; 3+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-08-20 15:48 UTC (permalink / raw)
  To: Igor Munkin, Sergey Ostanevich; +Cc: tarantool-patches

From: Mike Pall <mike>

(cherry picked from commit dfa692b746c9de067857d5fc992a41730be3d99a)

`string.char()` call without arguments yields an empty string. When JIT
machinery records the aforementioned call it doesn't handle this case.
Each recording fast function expects 1 result by default.  Hence, when
return from this call is recorded the framelink slot is used as a
result. It is loaded into the corresponding slot as an IR with `IRT_NUM`
type. It leads to assertion failure in `rec_check_slots()`, when a next
bytecode is recorded, because type of TValue on the stack (`LJ_STR`)
isn't the same as IR (and TRef) type.

This patch handles the case without arguments by the loading of IR with
empty string reference into the corresponding slot.

Sergey Kaplun:
* added the description and the test for the problem

Resolves tarantool/tarantool#6371
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6371-string-char-no-arg
Issue: https://github.com/tarantool/tarantool/issues/6371
Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6371-string-char-no-arg
Side note: CI is totally red, but AFAICS it's unrelated with my patch.
Side note: See also Changelog at the Tarantool branch.

 src/lj_ffrecord.c                             |  2 ++
 .../gh-6371-string-char-no-arg.test.lua       | 28 +++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 test/tarantool-tests/gh-6371-string-char-no-arg.test.lua

diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
index 8dfa80ed..be890a93 100644
--- a/src/lj_ffrecord.c
+++ b/src/lj_ffrecord.c
@@ -866,6 +866,8 @@ static void LJ_FASTCALL recff_string_char(jit_State *J, RecordFFData *rd)
     for (i = 0; J->base[i] != 0; i++)
       tr = emitir(IRT(IR_BUFPUT, IRT_PGC), tr, J->base[i]);
     J->base[0] = emitir(IRT(IR_BUFSTR, IRT_STR), tr, hdr);
+  } else if (i == 0) {
+    J->base[0] = lj_ir_kstr(J, &J2G(J)->strempty);
   }
   UNUSED(rd);
 }
diff --git a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
new file mode 100644
index 00000000..6df93f07
--- /dev/null
+++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
@@ -0,0 +1,28 @@
+local tap = require('tap')
+
+-- Test file to demonstrate assertion after `string.char()`
+-- recording.
+-- See also, https://github.com/tarantool/tarantool/issues/6371.
+
+local test = tap.test('gh-6371-string-char-no-arg')
+-- XXX: Number of loop iterations.
+-- 1, 2 -- instruction becomes hot
+-- 3 -- trace is recorded (considering loop recording specifics),
+-- but bytecodes are still executed via VM
+-- 4 -- trace is executed, need to check that emitted mcode is
+--      correct
+local NTEST = 4
+test:plan(NTEST)
+
+-- Storage for the results to avoid trace aborting by `test:ok()`.
+local results = {}
+jit.opt.start('hotloop=1')
+for _ = 1, NTEST do
+  table.insert(results, string.char())
+end
+
+for i = 1, NTEST do
+  test:ok(results[i] == '', 'correct recording of string.char() without args')
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.31.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.
  2021-08-20 15:48 [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments Sergey Kaplun via Tarantool-patches
@ 2021-08-31 11:39 ` Sergey Ostanevich via Tarantool-patches
  2021-09-01  7:10   ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 3+ messages in thread
From: Sergey Ostanevich via Tarantool-patches @ 2021-08-31 11:39 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi! Thanks for the patch!

Some readability comments below.

regards,
Sergos


> On 20 Aug 2021, at 18:48, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> (cherry picked from commit dfa692b746c9de067857d5fc992a41730be3d99a)
> 
> `string.char()` call without arguments yields an empty string. When JIT
> machinery records the aforementioned call it doesn't handle this case.
JIT recording machinery doesn’t handle this case.

> Each recording fast function expects 1 result by default.  Hence, when
                ^
               of a 

> return from this call is recorded the framelink slot is used as a
> result. It is loaded into the corresponding slot as an IR with `IRT_NUM`

I have a question here: is this number denotes the number of results?
Then, perhaps reword the previous sentence that this very number is 
considered as result.

> type. It leads to assertion failure in `rec_check_slots()`, when a next
> bytecode is recorded, because type of TValue on the stack (`LJ_STR`)
> isn't the same as IR (and TRef) type.
> 
> This patch handles the case without arguments by the loading of IR with
> empty string reference into the corresponding slot.
> 
I would add that code reuses assumption of one result by default,
hence no case for ‘i == 1’ in the code.

> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#6371
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6371-string-char-no-arg
> Issue: https://github.com/tarantool/tarantool/issues/6371
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6371-string-char-no-arg
> Side note: CI is totally red, but AFAICS it's unrelated with my patch.
> Side note: See also Changelog at the Tarantool branch.
> 
> src/lj_ffrecord.c                             |  2 ++
> .../gh-6371-string-char-no-arg.test.lua       | 28 +++++++++++++++++++
> 2 files changed, 30 insertions(+)
> create mode 100644 test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> 
> diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> index 8dfa80ed..be890a93 100644
> --- a/src/lj_ffrecord.c
> +++ b/src/lj_ffrecord.c
> @@ -866,6 +866,8 @@ static void LJ_FASTCALL recff_string_char(jit_State *J, RecordFFData *rd)
>     for (i = 0; J->base[i] != 0; i++)
>       tr = emitir(IRT(IR_BUFPUT, IRT_PGC), tr, J->base[i]);
>     J->base[0] = emitir(IRT(IR_BUFSTR, IRT_STR), tr, hdr);
> +  } else if (i == 0) {
> +    J->base[0] = lj_ir_kstr(J, &J2G(J)->strempty);
>   }
>   UNUSED(rd);
> }
> diff --git a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> new file mode 100644
> index 00000000..6df93f07
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> @@ -0,0 +1,28 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate assertion after `string.char()`
> +-- recording.
> +-- See also, https://github.com/tarantool/tarantool/issues/6371.
> +
> +local test = tap.test('gh-6371-string-char-no-arg')
> +-- XXX: Number of loop iterations.
> +-- 1, 2 -- instruction becomes hot
> +-- 3 -- trace is recorded (considering loop recording specifics),
> +-- but bytecodes are still executed via VM
> +-- 4 -- trace is executed, need to check that emitted mcode is
> +--      correct
> +local NTEST = 4
> +test:plan(NTEST)
> +
> +-- Storage for the results to avoid trace aborting by `test:ok()`.
> +local results = {}
> +jit.opt.start('hotloop=1')
> +for _ = 1, NTEST do
> +  table.insert(results, string.char())
> +end
> +
> +for i = 1, NTEST do
> +  test:ok(results[i] == '', 'correct recording of string.char() without args')
> +end
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.31.0
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.
  2021-08-31 11:39 ` Sergey Ostanevich via Tarantool-patches
@ 2021-09-01  7:10   ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 3+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-09-01  7:10 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

Hi, Sergos!

Thanks for the review!

On 31.08.21, Sergey Ostanevich wrote:
> Hi! Thanks for the patch!
> 
> Some readability comments below.
> 
> regards,
> Sergos
> 
> 

The new commit message is the following:

===================================================================
Fix string.char() recording with no arguments.

(cherry picked from commit dfa692b746c9de067857d5fc992a41730be3d99a)

`string.char()` call without arguments yields an empty string. JIT
recording machinery doesn’t handle this case. Each recording of a fast
function expects 1 result by default. Hence, when return from this call
is recorded the framelink slot (the top slot value) is considered as a
result to yield. It is loaded into the corresponding slot as an IR with
`IRT_NUM` type. It leads to assertion failure in `rec_check_slots()`,
when a next bytecode is recorded, because type of TValue on the stack
(`LJ_STR`) isn't the same as IR (and TRef) type.

This patch handles the case without arguments by the loading of IR with
empty string reference into the corresponding slot. It reuses assumption
of one result by default, hence there is no case for `i == 1` in the
code.

Sergey Kaplun:
* added the description and the test for the problem

Resolves tarantool/tarantool#6371
===================================================================

Branch is force-pushed.

> > On 20 Aug 2021, at 18:48, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > (cherry picked from commit dfa692b746c9de067857d5fc992a41730be3d99a)
> > 
> > `string.char()` call without arguments yields an empty string. When JIT
> > machinery records the aforementioned call it doesn't handle this case.
> JIT recording machinery doesn’t handle this case.

Fixed.

> 
> > Each recording fast function expects 1 result by default.  Hence, when
>                 ^
>                of a 

Fixed.

> 
> > return from this call is recorded the framelink slot is used as a
> > result. It is loaded into the corresponding slot as an IR with `IRT_NUM`
> 
> I have a question here: is this number denotes the number of results?
> Then, perhaps reword the previous sentence that this very number is 
> considered as result.

No, it means that the corresponding stack slot type is a number. But,
the result of the call must be the string, not a number.

> 
> > type. It leads to assertion failure in `rec_check_slots()`, when a next
> > bytecode is recorded, because type of TValue on the stack (`LJ_STR`)
> > isn't the same as IR (and TRef) type.
> > 
> > This patch handles the case without arguments by the loading of IR with
> > empty string reference into the corresponding slot.
> > 
> I would add that code reuses assumption of one result by default,
> hence no case for ‘i == 1’ in the code.

Added.

> 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Resolves tarantool/tarantool#6371
> > ---
> > 
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6371-string-char-no-arg
> > Issue: https://github.com/tarantool/tarantool/issues/6371
> > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6371-string-char-no-arg
> > Side note: CI is totally red, but AFAICS it's unrelated with my patch.
> > Side note: See also Changelog at the Tarantool branch.
> > 
> > src/lj_ffrecord.c                             |  2 ++
> > .../gh-6371-string-char-no-arg.test.lua       | 28 +++++++++++++++++++
> > 2 files changed, 30 insertions(+)
> > create mode 100644 test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> > 
> > diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> > index 8dfa80ed..be890a93 100644
> > --- a/src/lj_ffrecord.c
> > +++ b/src/lj_ffrecord.c
> > @@ -866,6 +866,8 @@ static void LJ_FASTCALL recff_string_char(jit_State *J, RecordFFData *rd)
> >     for (i = 0; J->base[i] != 0; i++)
> >       tr = emitir(IRT(IR_BUFPUT, IRT_PGC), tr, J->base[i]);
> >     J->base[0] = emitir(IRT(IR_BUFSTR, IRT_STR), tr, hdr);
> > +  } else if (i == 0) {
> > +    J->base[0] = lj_ir_kstr(J, &J2G(J)->strempty);
> >   }
> >   UNUSED(rd);
> > }
> > diff --git a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> > new file mode 100644
> > index 00000000..6df93f07
> > --- /dev/null
> > +++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> > @@ -0,0 +1,28 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate assertion after `string.char()`
> > +-- recording.
> > +-- See also, https://github.com/tarantool/tarantool/issues/6371.
> > +
> > +local test = tap.test('gh-6371-string-char-no-arg')
> > +-- XXX: Number of loop iterations.
> > +-- 1, 2 -- instruction becomes hot
> > +-- 3 -- trace is recorded (considering loop recording specifics),
> > +-- but bytecodes are still executed via VM
> > +-- 4 -- trace is executed, need to check that emitted mcode is
> > +--      correct
> > +local NTEST = 4
> > +test:plan(NTEST)
> > +
> > +-- Storage for the results to avoid trace aborting by `test:ok()`.
> > +local results = {}
> > +jit.opt.start('hotloop=1')
> > +for _ = 1, NTEST do
> > +  table.insert(results, string.char())
> > +end
> > +
> > +for i = 1, NTEST do
> > +  test:ok(results[i] == '', 'correct recording of string.char() without args')
> > +end
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.31.0
> > 
> 

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-09-01  7:11 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20 15:48 [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments Sergey Kaplun via Tarantool-patches
2021-08-31 11:39 ` Sergey Ostanevich via Tarantool-patches
2021-09-01  7:10   ` Sergey Kaplun via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git