Thanks, LGTM. Sergos > On 1 Sep 2021, at 10:10, Sergey Kaplun 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 > wrote: >>> >>> From: Mike Pall >>> >>> (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