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

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

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