[Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Sergey Kaplun via 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

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Sergey Ostanevich via 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
> 

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Sergey Kaplun via 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

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[sergos via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5849 bytes --]

Thanks, LGTM.

Sergos

> On 1 Sep 2021, at 10:10, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> 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 <mailto: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


[-- Attachment #2: Type: text/html, Size: 40354 bytes --]

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! Please consider my comments below.

On 20.08.21, Sergey Kaplun 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.
> Each recording fast function expects 1 result by default.  Hence, when

Typo: double space prior to "Hence".

> return from this call is recorded the framelink slot is used as a

Strictly saying, this might be not the framelink slot. If you look onto
the trace dump, you'll see there is an SLOAD emitted for 12th stack
slot, but the framelink of the string.char (to be inlined on the trace)
is 11th AFAICS. So the loaded slot is just a garbage left after tap
initialization, or something else (though it definitely looks to be a
framelink earlier). See the artefacts of my investigation below.

Side note: I fixed some issues in luajit-gdb.py while investigating.

| $ git dc
| diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
| index be890a93..cac98240 100644
| --- a/src/lj_ffrecord.c
| +++ b/src/lj_ffrecord.c
| @@ -866,8 +866,10 @@ 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);
| +#if 0
|    } else if (i == 0) {
|      J->base[0] = lj_ir_kstr(J, &J2G(J)->strempty);
| +#endif
|    }
|    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
| index 6df93f07..f8b620e1 100644
| --- a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
| +++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
| @@ -11,17 +11,19 @@ local test = tap.test('gh-6371-string-char-no-arg')
|  -- but bytecodes are still executed via VM
|  -- 4 -- trace is executed, need to check that emitted mcode is
|  --      correct
| -local NTEST = 4
| +local NTEST = 3
|  test:plan(NTEST)
|  
|  -- Storage for the results to avoid trace aborting by `test:ok()`.
| -local results = {}
| +local results = require'table.new'(3, 0)
|  jit.opt.start('hotloop=1')
|  for _ = 1, NTEST do
|    table.insert(results, string.char())
|  end
| +jit.opt.start('hotloop=56')
|  
|  for i = 1, NTEST do
| +  print(('Results:"%s"'):format(results[i]))
|    test:ok(results[i] == '', 'correct recording of string.char() without args')
|  end
| $ LUA_PATH="${PWD}/../test/tarantool-tests/?.lua;;" gdb --args ./luajit -jdump=+tbisrmXaT,dump.out ../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
| GNU gdb (Gentoo 11.1 vanilla) 11.1
| Copyright (C) 2021 Free Software Foundation, Inc.
| License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
| This is free software: you are free to change and redistribute it.
| There is NO WARRANTY, to the extent permitted by law.
| Type "show copying" and "show warranty" for details.
| This GDB was configured as "x86_64-pc-linux-gnu".
| Type "show configuration" for configuration details.
| For bug reporting instructions, please see:
| <https://bugs.gentoo.org/>.
| Find the GDB manual and other documentation resources online at:
|     <http://www.gnu.org/software/gdb/documentation/>.
| 
| For help, type "help".
| Type "apropos word" to search for commands related to "word"...
| Reading symbols from ./luajit...
| lj-arch command initialized
| lj-tv command initialized
| lj-str command initialized
| lj-tab command initialized
| lj-stack command initialized
| lj-state command initialized
| lj-gc command initialized
| luajit-gdb.py is successfully loaded
| (gdb) b recff_string_char
| Breakpoint 1 at 0xa9497: file /home/imun/projects/tarantool-luajit-maintain/src/lj_ffrecord.c, line 856.
| (gdb) b recff_table_insert
| Breakpoint 2 at 0x5555555feff7: file /home/imun/projects/tarantool-luajit-maintain/src/lj_ffrecord.c, line 1055.
| (gdb) b lj_BC_JLOOP
| Breakpoint 3 at 0x5555555d0b77: file buildvm_x86.dasc, line 811.
| (gdb) r
| Starting program: /home/imun/projects/tarantool-luajit-maintain/src/luajit -jdump=+tbisrmXaT,dump.out ../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
| TAP version 13
| 1..3
| 
| Breakpoint 1, recff_string_char (J=0x40027060, rd=0x40027098) at /home/imun/projects/tarantool-luajit-maintain/src/lj_ffrecord.c:856
| 856     {
| (gdb) lj-stack globalL
| ----------- Red zone:  5 slots -----------
| 0x40026d98            [    ] VALUE: nil
| 0x40026d90            [    ] VALUE: nil
| 0x40026d88            [    ] VALUE: nil
| 0x40026d80            [    ] VALUE: nil
| 0x40026d78            [    ] VALUE: nil
| ----------- Stack:   130 slots -----------
| 0x40026d70            [   M] VALUE: nil
| 0x400269f0:0x40026d68 [    ] 112 slots: Free stack slots
| 0x400269e8            [ BT ] VALUE: number 2.8116508684117752e-313
| 0x400269e0            [    ] FRAME: [LP] delta=11, fast function #77
| 0x400269d8            [    ] VALUE: table @ 0x40015150 (asize: 4, hmask: 0x0)
| 0x400269d0            [    ] VALUE: fast function #91
| 0x400269c8            [    ] VALUE: number 2
| 0x400269c0            [    ] VALUE: number 1
| 0x400269b8            [    ] VALUE: number 3
| 0x400269b0            [    ] VALUE: number 2
| 0x400269a8            [    ] VALUE: table @ 0x40015150 (asize: 4, hmask: 0x0)
| 0x400269a0            [    ] VALUE: number 3
| 0x40026998            [    ] VALUE: table @ 0x40015018 (asize: 0, hmask: 0x7)
| 0x40026990            [    ] VALUE: table @ 0x40014f78 (asize: 0, hmask: 0x1)
| 0x40026988            [    ] FRAME: [V] delta=1, Lua function @ 0x4000f9e0, 0 upvalues, "@../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua":0
| 0x40026980            [    ] FRAME: [CP] delta=3, Lua function @ 0x4000f9e0, 0 upvalues, "@../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua":0
| 0x40026978            [    ] VALUE: C function @ 0x55555555c942
| 0x40026970            [    ] VALUE: light userdata @ 0x0
| 0x40026968            [    ] FRAME: [CP] delta=1, C function @ 0x55555555defc
| 0x40026960            [S   ] FRAME: dummy L
| (gdb) c
| Continuing.
| 
| Breakpoint 2, recff_table_insert (J=0x8007fee, rd=0x7fffffffd140) at /home/imun/projects/tarantool-luajit-maintain/src/lj_ffrecord.c:1055
| 1055    {
| (gdb) lj-stack globalL
| ----------- Red zone:  5 slots -----------
| 0x40026d98            [    ] VALUE: nil
| 0x40026d90            [    ] VALUE: nil
| 0x40026d88            [    ] VALUE: nil
| 0x40026d80            [    ] VALUE: nil
| 0x40026d78            [    ] VALUE: nil
| ----------- Stack:   130 slots -----------
| 0x40026d70            [   M] VALUE: nil
| 0x400269f0:0x40026d68 [    ] 112 slots: Free stack slots
| 0x400269e8            [  T ] VALUE: number 6.2068441339562109e-313
| 0x400269e0            [    ] VALUE: string "" @ 0x400004b0
| 0x400269d8            [ B  ] VALUE: table @ 0x40015150 (asize: 4, hmask: 0x0)
| 0x400269d0            [    ] FRAME: [L] delta=9, fast function #91
| 0x400269c8            [    ] VALUE: number 2
| 0x400269c0            [    ] VALUE: number 1
| 0x400269b8            [    ] VALUE: number 3
| 0x400269b0            [    ] VALUE: number 2
| 0x400269a8            [    ] VALUE: table @ 0x40015150 (asize: 4, hmask: 0x0)
| 0x400269a0            [    ] VALUE: number 3
| 0x40026998            [    ] VALUE: table @ 0x40015018 (asize: 0, hmask: 0x7)
| 0x40026990            [    ] VALUE: table @ 0x40014f78 (asize: 0, hmask: 0x1)
| 0x40026988            [    ] FRAME: [V] delta=1, Lua function @ 0x4000f9e0, 0 upvalues, "@../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua":0
| 0x40026980            [    ] FRAME: [CP] delta=3, Lua function @ 0x4000f9e0, 0 upvalues, "@../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua":0
| 0x40026978            [    ] VALUE: C function @ 0x55555555c942
| 0x40026970            [    ] VALUE: light userdata @ 0x0
| 0x40026968            [    ] FRAME: [CP] delta=1, C function @ 0x55555555defc
| 0x40026960            [S   ] FRAME: dummy L
| (gdb) c
| Continuing.
| 
| Breakpoint 3, 0x00005555555d0b77 in lj_BC_JLOOP () at buildvm_x86.dasc:811
| 811     buildvm_x86.dasc: No such file or directory.
| (gdb) lj-stack globalL
| ----------- Red zone:  5 slots -----------
| 0x40026d98            [    ] VALUE: nil
| 0x40026d90            [    ] VALUE: nil
| 0x40026d88            [    ] VALUE: nil
| 0x40026d80            [    ] VALUE: nil
| 0x40026d78            [    ] VALUE: nil
| ----------- Stack:   130 slots -----------
| 0x40026d70            [   M] VALUE: nil
| 0x400269f8:0x40026d68 [    ] 111 slots: Free stack slots
| 0x400269f0            [  T ] VALUE: number 2.3182810450216066e-312
| 0x400269e8            [    ] VALUE: number 6.2068441339562109e-313
| 0x400269e0            [    ] VALUE: string "" @ 0x400004b0
| 0x400269d8            [    ] VALUE: table @ 0x40015150 (asize: 4, hmask: 0x0)
| 0x400269d0            [    ] VALUE: number 2.0815281868063522
| 0x400269c8            [    ] VALUE: number 3
| 0x400269c0            [    ] VALUE: number 1
| 0x400269b8            [    ] VALUE: number 3
| 0x400269b0            [    ] VALUE: number 3
| 0x400269a8            [    ] VALUE: table @ 0x40015150 (asize: 4, hmask: 0x0)
| 0x400269a0            [    ] VALUE: number 3
| 0x40026998            [    ] VALUE: table @ 0x40015018 (asize: 0, hmask: 0x7)
| 0x40026990            [ B  ] VALUE: table @ 0x40014f78 (asize: 0, hmask: 0x1)
| 0x40026988            [    ] FRAME: [V] delta=1, Lua function @ 0x4000f9e0, 0 upvalues, "@../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua":0
| 0x40026980            [    ] FRAME: [CP] delta=3, Lua function @ 0x4000f9e0, 0 upvalues, "@../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua":0
| 0x40026978            [    ] VALUE: C function @ 0x55555555c942
| 0x40026970            [    ] VALUE: light userdata @ 0x0
| 0x40026968            [    ] FRAME: [CP] delta=1, C function @ 0x55555555defc
| 0x40026960            [S   ] FRAME: dummy L
| (gdb) c
| Continuing.
| Results:""
| ok - correct recording of string.char() without args
| Results:""
| ok - correct recording of string.char() without args
| Results:"6.2068441339562e-313"
| not ok - correct recording of string.char() without args
|     filename:   eval
|     line:       -1
|     frame #1
|       line:     0
|       source:   @../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
|       filename: ../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
|       what:     main
|       namewhat: 
|       src:      ../test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
|     frame #2
|       line:     -1
|       source:   =[C]
|       filename: eval
|       what:     C
|       namewhat: 
|       src:      [C]
| # failed subtest: 1
| [Inferior 1 (process 14271) exited with code 01]
| $ cat dump.out
| ---- TRACE 1 start gh-6371-string-char-no-arg.test.lua:20
| 0027  GGET     8  10      ; "table"
| 0028  TGETS    8   8  11  ; "insert"
| 0029  MOV      9   3
| 0030  GGET    10  12      ; "string"
| 0031  TGETS   10  10  13  ; "char"
| 0032  CALL    10   0   1
| 0000  . FUNCC               ; string.char
| 0033  CALLM    8   1   1
| 0000  . FUNCC               ; table.insert
| 0034  FORL     4 => 0027
| ---- TRACE 1 IR
| ....              SNAP   #0   [ ---- ]
| 0001 rbx   >  int SLOAD  #6    CRI
| 0002       >  int LE     0001  +2147483646
| 0003 rbp      int SLOAD  #5    CI
| 0004 rax      fun SLOAD  #0    R
| 0005 rax      tab FLOAD  0004  func.env
| 0006          int FLOAD  0005  tab.hmask
| 0007       >  int EQ     0006  +63 
| 0008 rax      p32 FLOAD  0005  tab.node
| 0009       >  p32 HREFK  0008  "table" @47
| 0010 rcx   >  tab HLOAD  0009
| 0011          int FLOAD  0010  tab.hmask
| 0012       >  int EQ     0011  +15 
| 0013 r14      p32 FLOAD  0010  tab.node
| 0014       >  p32 HREFK  0013  "insert" @15
| 0015       >  fun HLOAD  0014
| 0016 [10]  >  tab SLOAD  #4    T
| 0017       >  p32 HREFK  0008  "string" @59
| 0018 rax   >  tab HLOAD  0017
| 0019          int FLOAD  0018  tab.hmask
| 0020       >  int EQ     0019  +15 
| 0021 r15      p32 FLOAD  0018  tab.node
| 0022       >  p32 HREFK  0021  "char" @12
| 0023       >  fun HLOAD  0022
| 0024       >  fun EQ     0023  string.char
| 0025 [8]   >  num SLOAD  #12   T
| 0026       >  fun EQ     0015  table.insert
| 0027 rax      int CALLL  lj_tab_len  (0016)
| 0028 rax      int ADD    0027  +1  
| 0029 r13      int FLOAD  0016  tab.asize
| 0030       >  int ABC    0029  0028
| 0031 r12      p32 FLOAD  0016  tab.array
| 0032          p32 AREF   0031  0028
| 0033          num ASTORE 0032  0025
| 0034 rbp    + int ADD    0003  +1  
| ....              SNAP   #1   [ ---- ---- ---- ---- ---- ]
| 0035       >  int LE     0034  0001
| ....              SNAP   #2   [ ---- ---- ---- ---- ---- 0034 0001 ---- 0034 ]
| 0036 ------------ LOOP ------------
| 0037 rax      int CALLL  lj_tab_len  (0016)
| 0038 rax      int ADD    0037  +1  
| 0039       >  int ABC    0029  0038
| 0040          p32 AREF   0031  0038
| 0041          num ASTORE 0040  0025
| 0042 rbp    + int ADD    0034  +1  
| ....              SNAP   #3   [ ---- ---- ---- ---- ---- ]
| 0043       >  int LE     0042  0001
| 0044 rbp      int PHI    0034  0042
| ---- TRACE 1 mcode 441
| 55557f6bfe44  add rsp, -0x10
| 55557f6bfe48  mov dword [0x40000518], 0x1
| 55557f6bfe53  movsd xmm7, [rdx+0x28]
| 55557f6bfe58  cvttsd2si ebx, xmm7
| 55557f6bfe5c  xorps xmm6, xmm6
| 55557f6bfe5f  cvtsi2sd xmm6, ebx
| 55557f6bfe63  ucomisd xmm7, xmm6
| 55557f6bfe67  jnz 0x55557f6b0010        ->0
| 55557f6bfe6d  jpe 0x55557f6b0010        ->0
| 55557f6bfe73  cmp ebx, 0x7ffffffe
| 55557f6bfe79  jg 0x55557f6b0010 ->0
| 55557f6bfe7f  cvttsd2si ebp, [rdx+0x20]
| 55557f6bfe84  mov eax, [rdx-0x8]
| 55557f6bfe87  mov eax, [rax+0x8]
| 55557f6bfe8a  cmp dword [rax+0x1c], +0x3f
| 55557f6bfe8e  jnz 0x55557f6b0010        ->0
| 55557f6bfe94  mov eax, [rax+0x14]
| 55557f6bfe97  mov rdi, 0xfffffffb40002f78
| 55557f6bfea1  cmp rdi, [rax+0x470]
| 55557f6bfea8  jnz 0x55557f6b0010        ->0
| 55557f6bfeae  cmp dword [rax+0x46c], -0x0c
| 55557f6bfeb5  jnz 0x55557f6b0010        ->0
| 55557f6bfebb  mov ecx, [rax+0x468]
| 55557f6bfec1  cmp dword [rcx+0x1c], +0x0f
| 55557f6bfec5  jnz 0x55557f6b0010        ->0
| 55557f6bfecb  mov r14d, [rcx+0x14]
| 55557f6bfecf  mov rdi, 0xfffffffb400048c8
| 55557f6bfed9  cmp rdi, [r14+0x170]
| 55557f6bfee0  jnz 0x55557f6b0010        ->0
| 55557f6bfee6  cmp dword [r14+0x16c], -0x09
| 55557f6bfeee  jnz 0x55557f6b0010        ->0
| 55557f6bfef4  cmp dword [rdx+0x1c], -0x0c
| 55557f6bfef8  jnz 0x55557f6b0010        ->0
| 55557f6bfefe  mov edi, [rdx+0x18]
| 55557f6bff01  mov [rsp+0x10], edi
| 55557f6bff05  mov rsi, 0xfffffffb40002eb8
| 55557f6bff0f  cmp rsi, [rax+0x590]
| 55557f6bff16  jnz 0x55557f6b0010        ->0
| 55557f6bff1c  cmp dword [rax+0x58c], -0x0c
| 55557f6bff23  jnz 0x55557f6b0010        ->0
| 55557f6bff29  mov eax, [rax+0x588]
| 55557f6bff2f  cmp dword [rax+0x1c], +0x0f
| 55557f6bff33  jnz 0x55557f6b0010        ->0
| 55557f6bff39  mov r15d, [rax+0x14]
| 55557f6bff3d  mov rsi, 0xfffffffb40005d80
| 55557f6bff47  cmp rsi, [r15+0x128]
| 55557f6bff4e  jnz 0x55557f6b0010        ->0
| 55557f6bff54  cmp dword [r15+0x124], -0x09
| 55557f6bff5c  jnz 0x55557f6b0010        ->0
| 55557f6bff62  cmp dword [r15+0x120], 0x40005d58
| 55557f6bff6d  jnz 0x55557f6b0010        ->0
| 55557f6bff73  cmp dword [rdx+0x5c], 0xfffeffff
| 55557f6bff7a  jnb 0x55557f6b0010        ->0
| 55557f6bff80  movsd xmm0, [rdx+0x58]
| 55557f6bff85  movsd [rsp+0x8], xmm0
| 55557f6bff8b  cmp dword [r14+0x168], 0x400048a0
| 55557f6bff96  jnz 0x55557f6b0010        ->0
| 55557f6bff9c  call 0x55555557a37f       ->lj_tab_len
| 55557f6bffa1  mov edi, [rsp+0x10]
| 55557f6bffa5  movsd xmm0, [rsp+0x8]
| 55557f6bffab  add eax, +0x01
| 55557f6bffae  mov r13d, [rdi+0x18]
| 55557f6bffb2  cmp eax, r13d
| 55557f6bffb5  jnb 0x55557f6b0010        ->0
| 55557f6bffbb  mov r12d, [rdi+0x8]
| 55557f6bffbf  movsd [r12+rax*8], xmm0
| 55557f6bffc5  add ebp, +0x01
| 55557f6bffc8  cmp ebp, ebx
| 55557f6bffca  jg 0x55557f6b0014 ->1
| ->LOOP:
| 55557f6bffd0  mov edi, [rsp+0x10]
| 55557f6bffd4  call 0x55555557a37f       ->lj_tab_len
| 55557f6bffd9  movsd xmm0, [rsp+0x8]
| 55557f6bffdf  add eax, +0x01
| 55557f6bffe2  cmp eax, r13d
| 55557f6bffe5  jnb 0x55557f6b0018        ->2
| 55557f6bffeb  movsd [r12+rax*8], xmm0
| 55557f6bfff1  add ebp, +0x01
| 55557f6bfff4  cmp ebp, ebx
| 55557f6bfff6  jle 0x55557f6bffd0        ->LOOP
| 55557f6bfff8  jmp 0x55557f6b001c        ->3
| ---- TRACE 1 stop -> loop
| 
| ---- TRACE 1 exit 1
|  0000000000000003 0000000040004470 0000000000000010 0000000000000003
|  00007fffffffd600 0000000000000004 fffffffb40005d80 0000000040015150
|  ffffffffffffffe8 0000000000000400 0000000000000062 00000000fffffffe
|  0000000040015170 0000000000000004 0000000040004498 0000000040005af0
|  +6.2068441339562e-313                +3 +4.635570563489e-310 +4.6355705635109e-310
|  +4.6355705632257e-310 +4.6355705635238e-310                +3                +3
|  +4.635570563481e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0

As I've mentioned above, IR 0025 is SLOAD #12, and the 12th slot while
running the compiled trace is 0x400269e8 which was both base and top
slot for <string.char> call, but wasn't not its framelink. It wasn't so
clear, since luajit-gdb.py dumps nothing if both base and top points to
a single slot.

I propose to omit this framelink part and just mention that some garbage
can be loaded from stack as a result of <string.char> recording in case
when no arguments are given.

> 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.

I see no assertion running the test in Debug, unfortunately. Could you
please provide the way to hit it?

> 
> 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: Try to rebase on the bleeding master, please.

> 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

Minor: Strictly saying, this is not quite true. The right description is
the following:
* 1 -- instruction becomes hot.
* 2 -- recording of the loop body.
* 3 -- required for trace finalization, but this iteration runs the
  generated mcode. NB: the issue doesn't occur, since execution leaves
  the trace on table resizing guard.
* 4 -- reproduces the issue.

Hence, if you allocate the table with necessary size (3 in our case),
then only 3 iterations are needed. Please adjust the comment above and
the code below.

> +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,
IM

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Igor Munkin via Tarantool-patches]

Sergey,

On 28.01.22, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider my comments below.
> 
> On 20.08.21, Sergey Kaplun 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.
> > Each recording fast function expects 1 result by default.  Hence, when
> 
> Typo: double space prior to "Hence".

Damn, I was looking at the old patch, but you've already fixed it in the
newer version. Sorry, feel free to ignore this nit.

> 

<snipped>

> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Sergey Kaplun via Tarantool-patches]

Igor,
Thanks for the review!

On 28.01.22, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider my comments below.
> 
> On 20.08.21, Sergey Kaplun 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.
> > Each recording fast function expects 1 result by default.  Hence, when
> 
> Typo: double space prior to "Hence".

Fixed.

> 
> > return from this call is recorded the framelink slot is used as a
> 
> Strictly saying, this might be not the framelink slot. If you look onto
> the trace dump, you'll see there is an SLOAD emitted for 12th stack
> slot, but the framelink of the string.char (to be inlined on the trace)
> is 11th AFAICS. So the loaded slot is just a garbage left after tap
> initialization, or something else (though it definitely looks to be a
> framelink earlier). See the artefacts of my investigation below.
> 

<snipped>

> 
> As I've mentioned above, IR 0025 is SLOAD #12, and the 12th slot while
> running the compiled trace is 0x400269e8 which was both base and top
> slot for <string.char> call, but wasn't not its framelink. It wasn't so
> clear, since luajit-gdb.py dumps nothing if both base and top points to
> a single slot.
> 
> I propose to omit this framelink part and just mention that some garbage
> can be loaded from stack as a result of <string.char> recording in case
> when no arguments are given.

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 some garbarge 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
===================================================================

> 
> > 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.
> 
> I see no assertion running the test in Debug, unfortunately. Could you
> please provide the way to hit it?

I build as usual:
| $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON && make -j

and run this chunk (like test but simplier).
| $ src/luajit -e '
| local results = {}
| jit.opt.start("hotloop=1")
| for _ = 1, 3 do
|   print(string.char())
| end
| '

With the following result:

| luajit: /home/burii/builds_workspace/luajit/gh-6371-string-char-no-arg/src/lj_record.c:137: rec_check_slots: Assertion `itype2irt(tv) == ((IRType)(((tr)>>24) &
|  IRT_TYPE))' failed.
| Aborted (core dumped)

But there are many ways to fail (for example on fold optimisation):

| luajit: /home/burii/builds_workspace/luajit/gh-6371-string-char-no-arg/src/lj_opt_fold.c:2318: fold_fwd_sload: Assertion `J->slot[(&J->fold.ins)->op1] != 0' failed.

> 
> > 
> > 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: Try to rebase on the bleeding master, please.

The updated tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6371-string-char-no-arg-full-ci

> 
> > 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
> 
> Minor: Strictly saying, this is not quite true. The right description is
> the following:
> * 1 -- instruction becomes hot.
> * 2 -- recording of the loop body.
> * 3 -- required for trace finalization, but this iteration runs the
>   generated mcode. NB: the issue doesn't occur, since execution leaves
>   the trace on table resizing guard.
> * 4 -- reproduces the issue.
> 
> Hence, if you allocate the table with necessary size (3 in our case),
> then only 3 iterations are needed. Please adjust the comment above and
> the code below.

Fixed, thanks!

See the iterative patch below.
Branch is force-pushed.
===================================================================
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
index 6df93f07..61d02101 100644
--- a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
+++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
@@ -6,11 +6,12 @@ local tap = require('tap')
 
 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
+-- * 1 -- instruction becomes hot.
+-- * 2 -- recording of the loop body.
+-- * 3 -- required for trace finalization, but this iteration runs the
+--   generated mcode. NB: the issue doesn't occur, since execution leaves
+--   the trace on table resizing guard.
+-- * 4 -- reproduces the issue.
 local NTEST = 4
 test:plan(NTEST)
 
===================================================================

> 
> > +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,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Sergey Kaplun via Tarantool-patches]

Igor,

On 28.01.22, Sergey Kaplun via Tarantool-patches wrote:
> Igor,
> Thanks for the review!
> 
> On 28.01.22, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! Please consider my comments below.
> > 
> > On 20.08.21, Sergey Kaplun wrote:

<snipped>

> > 
> > Minor: Strictly saying, this is not quite true. The right description is
> > the following:
> > * 1 -- instruction becomes hot.
> > * 2 -- recording of the loop body.
> > * 3 -- required for trace finalization, but this iteration runs the
> >   generated mcode. NB: the issue doesn't occur, since execution leaves
> >   the trace on table resizing guard.
> > * 4 -- reproduces the issue.
> > 
> > Hence, if you allocate the table with necessary size (3 in our case),
> > then only 3 iterations are needed. Please adjust the comment above and
> > the code below.
> 
> Fixed, thanks!
> 
> See the iterative patch below.
> Branch is force-pushed.
> ===================================================================
> 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
> index 6df93f07..61d02101 100644
> --- a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> +++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
> @@ -6,11 +6,12 @@ local tap = require('tap')
>  
>  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
> +-- * 1 -- instruction becomes hot.
> +-- * 2 -- recording of the loop body.
> +-- * 3 -- required for trace finalization, but this iteration runs the
> +--   generated mcode. NB: the issue doesn't occur, since execution leaves
> +--   the trace on table resizing guard.
> +-- * 4 -- reproduces the issue.
>  local NTEST = 4
>  test:plan(NTEST)
>  
> ===================================================================

Modified test for 3 iterations without side exit from trace.

===================================================================
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
index 61d02101..93f0fbfb 100644
--- a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
+++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
@@ -8,15 +8,14 @@ local test = tap.test('gh-6371-string-char-no-arg')
 -- XXX: Number of loop iterations.
 -- * 1 -- instruction becomes hot.
 -- * 2 -- recording of the loop body.
--- * 3 -- required for trace finalization, but this iteration runs the
---   generated mcode. NB: the issue doesn't occur, since execution leaves
---   the trace on table resizing guard.
--- * 4 -- reproduces the issue.
-local NTEST = 4
+-- * 3 -- reproduces the issue.
+local NTEST = 3
 test:plan(NTEST)
 
 -- Storage for the results to avoid trace aborting by `test:ok()`.
-local results = {}
+-- XXX: Use `table.new()` here to avoid side exit from trace by
+-- table resizing.
+local results = require('table.new')(3, 0)
 jit.opt.start('hotloop=1')
 for _ = 1, NTEST do
   table.insert(results, string.char())
===================================================================

> 
> > 

<snipped>

> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Igor Munkin via Tarantool-patches]

Sergey,

On 28.01.22, Sergey Kaplun wrote:
> Igor,
> Thanks for the review!
> 

<snipped>

> > 
> > I see no assertion running the test in Debug, unfortunately. Could you
> > please provide the way to hit it?
> 
> I build as usual:
> | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON && make -j

PEBKAC, I totally forgot to enable LUA_USE_APICHECK and LUA_USE_ASSERT.
I guess it's time to enable both by default for Debug. Thoughts?

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Sergey Kaplun via Tarantool-patches]

Igor,

On 28.01.22, Igor Munkin wrote:
> Sergey,
> 
> On 28.01.22, Sergey Kaplun wrote:
> > Igor,
> > Thanks for the review!
> > 
> 
> <snipped>
> 
> > > 
> > > I see no assertion running the test in Debug, unfortunately. Could you
> > > please provide the way to hit it?
> > 
> > I build as usual:
> > | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON && make -j
> 
> PEBKAC, I totally forgot to enable LUA_USE_APICHECK and LUA_USE_ASSERT.
> I guess it's time to enable both by default for Debug. Thoughts?

I'm OK with it. Not against at least. :)

> 
> <snipped>
> 
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in 1.10, 2.8 and master.

On 20.08.21, Sergey Kaplun 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.
> 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
> 

<snipped>

> -- 
> 2.31.0
> 

-- 
Best regards,
IM