Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.
@ 2022-07-13  9:53 Sergey Kaplun via Tarantool-patches
  2022-07-14 11:54 ` sergos via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-07-13  9:53 UTC (permalink / raw)
  To: Sergey Ostanevich, Igor Munkin; +Cc: tarantool-patches

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.
2) known fixed number of results.

For this case the vararg slots loads via `IR_VLOAD` by offset from
vararg base. In GC64 mode this offset was miscounting 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 the aforementioned `LJ_FR2` correction.

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)));
 	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.
+-- 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
+local function test_rec_varg(...)
+  local slot1
+  for _ = 1, 3 do
+    slot1 = ...
+  end
+  return slot1 == MAGIC
+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')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1


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

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.
  2022-07-13  9:53 [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results Sergey Kaplun via Tarantool-patches
@ 2022-07-14 11:54 ` sergos via Tarantool-patches
  2022-07-15 14:44   ` Sergey Kaplun via Tarantool-patches
  2022-07-19  0:14 ` Igor Munkin via Tarantool-patches
  2022-08-10 14:32 ` Igor Munkin via Tarantool-patches
  2 siblings, 1 reply; 6+ messages in thread
From: sergos via Tarantool-patches @ 2022-07-14 11:54 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi!

Thanks for the patch, some updates to the test.

Regards,
Sergos

> On 13 Jul 2022, at 12:53, Sergey Kaplun <skaplun@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?

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

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


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

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.
  2022-07-14 11:54 ` sergos via Tarantool-patches
@ 2022-07-15 14:44   ` Sergey Kaplun via Tarantool-patches
  2022-07-15 15:03     ` sergos via Tarantool-patches
  0 siblings, 1 reply; 6+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-07-15 14:44 UTC (permalink / raw)
  To: sergos; +Cc: tarantool-patches

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

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

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.
  2022-07-15 14:44   ` Sergey Kaplun via Tarantool-patches
@ 2022-07-15 15:03     ` sergos via Tarantool-patches
  0 siblings, 0 replies; 6+ messages in thread
From: sergos via Tarantool-patches @ 2022-07-15 15:03 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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

Hi!

Thanks, LGTM.

Sergos


> On 15 Jul 2022, at 17:44, Sergey Kaplun <skaplun@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@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


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

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

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.
  2022-07-13  9:53 [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results Sergey Kaplun via Tarantool-patches
  2022-07-14 11:54 ` sergos via Tarantool-patches
@ 2022-07-19  0:14 ` Igor Munkin via Tarantool-patches
  2022-08-10 14:32 ` Igor Munkin via Tarantool-patches
  2 siblings, 0 replies; 6+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-07-19  0:14 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

Thanks for your patch! LGTM, after the fixes you've made[1] against
Sergos' review comments.

On 13.07.22, Sergey Kaplun 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.
> 2) known fixed number of results.
> 
> For this case the vararg slots loads via `IR_VLOAD` by offset from
> vararg base. In GC64 mode this offset was miscounting 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 the aforementioned `LJ_FR2` correction.
> 
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

[1]: https://github.com/tarantool/luajit/commit/5dbc5ab

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.
  2022-07-13  9:53 [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results Sergey Kaplun via Tarantool-patches
  2022-07-14 11:54 ` sergos via Tarantool-patches
  2022-07-19  0:14 ` Igor Munkin via Tarantool-patches
@ 2022-08-10 14:32 ` Igor Munkin via Tarantool-patches
  2 siblings, 0 replies; 6+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-08-10 14:32 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

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

On 13.07.22, Sergey Kaplun 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.
> 2) known fixed number of results.
> 
> For this case the vararg slots loads via `IR_VLOAD` by offset from
> vararg base. In GC64 mode this offset was miscounting 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 the aforementioned `LJ_FR2` correction.
> 
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM

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

end of thread, other threads:[~2022-08-10 14:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-13  9:53 [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results Sergey Kaplun via Tarantool-patches
2022-07-14 11:54 ` sergos via Tarantool-patches
2022-07-15 14:44   ` Sergey Kaplun via Tarantool-patches
2022-07-15 15:03     ` sergos via Tarantool-patches
2022-07-19  0:14 ` Igor Munkin via Tarantool-patches
2022-08-10 14:32 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox