[Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.

[sergos]

Hi!

Thanks, LGTM.

Sergos


> On 15 Jul 2022, at 17:44, Sergey Kaplun <skaplun at tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 14.07.22, sergos wrote:
>> Hi!
>> 
>> Thanks for the patch, some updates to the test.
>> 
>> Regards,
>> Sergos
>> 
>>> On 13 Jul 2022, at 12:53, Sergey Kaplun <skaplun at tarantool.org> wrote:
>>> 
>>> From: Mike Pall <mike>
>>> 
>>> Reported by George Vaintrub. Fixed by Sergey Kaplun.
>>> 
>>> (cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1)
>>> 
>>> This bug occurs when recording `BC_VARG` with the following conditions:
>>> 1) varargs undefined on trace.
>> Later in the test you just mention its size should be bigger than results’ one?
> 
> I meant that the __content__ of these slots is undefined, because the trace
> is started inside current function and we don't know the caller's context.
> So we can't just copy these slots.
> 
>> 
>>> 2) known fixed number of results.
>>> 
>>> For this case the vararg slots loads via `IR_VLOAD` by offset from
>> are loaded by using an
>> 
>>> vararg base. In GC64 mode this offset was miscounting due to missing
>> the the is miscounted
>> 
>>> `LJ_FR2` correction in the base TRef calculation. As the result the
>>> wrong (+1) vararg slot is used.
>>> 
>>> This patch adds the missing the aforementioned `LJ_FR2` correction.
>> xxx
> 
> I've updated commit message to the following:
> Branch is force-pushed.
> 
> ===================================================================
> LJ_GC64: Fix IR_VARG offset for fixed number of results.
> 
> Reported by George Vaintrub. Fixed by Sergey Kaplun.
> 
> (cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1)
> 
> This bug occurs when recording `BC_VARG` with the following conditions:
> 1) A trace is started inside current vararg function, so the content of
> varargs slots is undefined for the trace.
> 2) Known fixed number of results.
> 
> For this case the vararg slots are loaded by `IR_VLOAD` using an offset
> from the vararg base. In the GC64 mode this offset is miscounted due to
> missing `LJ_FR2` correction in the base TRef calculation. As the result
> the wrong (+1) vararg slot is used.
> 
> This patch adds the missing aforementioned `LJ_FR2` correction.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#7172
> Part of tarantool/tarantool#7230
> ===================================================================
> 
>>> 
>>> Sergey Kaplun:
>>> * added the description and the test for the problem
>>> 
>>> Resolves tarantool/tarantool#7172
>>> Part of tarantool/tarantool#7230
>>> ---
>>> 
>>> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-864-varg-rec-base-offset-full-ci
>>> Issues:
>>> * https://github.com/tarantool/tarantool/issues/7172
>>> * https://github.com/LuaJIT/LuaJIT/issues/864
>>> 
>>> src/lj_record.c | 2 +-
>>> .../lj-864-varg-rec-base-offset.test.lua | 25 +++++++++++++++++++
>>> 2 files changed, 26 insertions(+), 1 deletion(-)
>>> create mode 100644 test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
>>> 
>>> diff --git a/src/lj_record.c b/src/lj_record.c
>>> index a11f3712..9e2e1d9e 100644
>>> --- a/src/lj_record.c
>>> +++ b/src/lj_record.c
>>> @@ -1794,7 +1794,7 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>>> 	 emitir(IRTGI(IR_EQ), fr,
>>> 		 lj_ir_kint(J, (int32_t)frame_ftsz(J->L->base-1)));
>>> 	vbase = emitir(IRT(IR_SUB, IRT_IGC), REF_BASE, fr);
>>> -	vbase = emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, frofs-8));
>>> +	vbase = emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, frofs-8*(1+LJ_FR2)));
>> Wherearemyspaces?
>> (nevermind, just a moan)
> 
> Yes, but this style is preferred by Mike (see code below) :).
> 
>> 
>>> 	for (i = 0; i < nload; i++) {
>>> 	 IRType t = itype2irt(&J->L->base[i-1-LJ_FR2-nvararg]);
>>> 	 TRef aref = emitir(IRT(IR_AREF, IRT_PGC),
>>> diff --git a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
>>> new file mode 100644
>>> index 00000000..ca30f92f
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
>>> @@ -0,0 +1,25 @@
>>> +local tap = require('tap')
>>> +
>>> +-- Test file to demonstrate LuaJIT misbehaviour during recording
>>> +-- BC_VARG with nvarargs >= nresults in GC64 mode.
>> In the message you say it should be unknown. What’s the dirty truth is?
>> 
>>> +-- See also https://github.com/LuaJIT/LuaJIT/issues/864,
>>> +-- https://github.com/tarantool/tarantool/issues/7172.
>>> +local test = tap.test('lj-864-varg-rec-base-offset')
>>> +test:plan(1)
>>> +
>>> +jit.opt.start('hotloop=1')
>>> +
>>> +local MAGIC = 42
>> Should be enough to test against the first argument, no MAGIC :)
>> 
>>> +local function test_rec_varg(...)
>>> + local slot1
>>> + for _ = 1, 3 do
>>> + slot1 = ...
>>> + end
>> ++ args = {...}
>> +- return slot1 == args[1]
>>> +end
> 
> I've rewritten test wo MAGIC :).
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
> index ca30f92f..d74c3c2b 100644
> --- a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
> +++ b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
> @@ -9,17 +9,17 @@ test:plan(1)
> 
> jit.opt.start('hotloop=1')
> 
> -local MAGIC = 42
> local function test_rec_varg(...)
> - local slot1
> + local trace_value, interp_value
> for _ = 1, 3 do
> - slot1 = ...
> + trace_value = ...
> end
> - return slot1 == MAGIC
> + interp_value = ...
> + return trace_value == interp_value
> end
> 
> -- Test case for nvarargs >= nresults. Equality is not suitable
> -- due to failing assertion guard for type of loaded vararg slot.
> -test:ok(test_rec_varg(MAGIC, 0), 'correct BC_VARG recording')
> +test:ok(test_rec_varg(42, 0), 'correct BC_VARG recording')
> 
> os.exit(test:check() and 0 or 1)
> ===================================================================
> 
>>> +
>>> +-- Test case for nvarargs >= nresults. Equality is not suitable
>>> +-- due to failing assertion guard for type of loaded vararg slot.
>>> +test:ok(test_rec_varg(MAGIC, 0), 'correct BC_VARG recording')
>>> +
>>> +os.exit(test:check() and 0 or 1)
>>> -- 
>>> 2.34.1
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20220715/3f82c845/attachment.htm>