Re: [Tarantool-patches] [PATCH luajit 2/2] Fix maxslots when recording BC_VARG, part 2.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>Fixed your comments, branch is force-pushed.
>>
>>On 14.07.23, Maxim Kokryashkin wrote:
>>>
>>> Hi!
>>> Thanks for the patch!
>>> LGTM, except for a few nits below.
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Analyzed by Sergey Kaplun.
>>> >>
>>> >>(cherry-picked from commit a01cba9d2d74efc57376822aa43db2d5043af5a4)
>>> >>
>>> >>This patch is the follow-up for the previous one. It removes the
>>> >Typo: s/the/a
>>> >Typo: s/for the/to the/
>>
>>Fixed, thanks!
>>
>>> >>condition for `maxslot` changing in the case when varargs are defined
>>> >>on the trace (i.e. `framedepth` > 0).
>>> >Typo: s/i.e./i.e.,/
>>
>>I prefer to avoid double punctuation symbols as mentioned here[1].
>>Also, this style is allowed by Oxford dictionary (as referenced in
>>[1]).
>>
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description and the test for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8825
>>> >>---
>>> >> src/lj_record.c | 8 ++------
>>> >> .../lj-1024-varg-maxslot.test.lua | 19 ++++++++++++++++++-
>>> >> 2 files changed, 20 insertions(+), 7 deletions(-)
>>> >>
>>> >>diff --git a/src/lj_record.c b/src/lj_record.c
>>> >>index 112524d3..02d9db9e 100644
>>> >>--- a/src/lj_record.c
>>> >>+++ b/src/lj_record.c
>>> >>@@ -1775,12 +1775,8 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>>> >>   if (J->framedepth > 0) { /* Simple case: varargs defined on-trace. */
>>> >>     ptrdiff_t i;
>>> >>     if (nvararg < 0) nvararg = 0;
>>> >>- if (nresults == -1) {
>>> >>- nresults = nvararg;
>>> >>- J->maxslot = dst + (BCReg)nvararg;
>>> >>- } else if (dst + nresults > J->maxslot) {
>>> >>- J->maxslot = dst + (BCReg)nresults;
>>> >>- }
>>> >>+ if (nresults == -1) nresults = nvararg;
>>> >>+ J->maxslot = dst + (BCReg)nresults;
>>> >>     for (i = 0; i < nresults; i++)
>>> >>       J->base[dst+i] = i < nvararg ? getslot(J, i - nvararg - 1 - LJ_FR2) : TREF_NIL;
>>> >>   } else { /* Unknown number of varargs passed to trace. */
>>> >>diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>> >>index 14270595..f8d74e8a 100644
>>> >>--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>> >>+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>> >>@@ -3,7 +3,7 @@ local test = tap.test('lj-noticket-varg-usedef'):skipcond({
>>> >>   ['Test requires JIT enabled'] = not jit.status(),
>>> >> })
>>> >> 
>>> >>-test:plan(1)
>>> >>+test:plan(2)
>>> >> 
>>> >> jit.opt.start('hotloop=1')
>>> >> 
>>> >>@@ -20,4 +20,21 @@ end
>>> >> 
>>> >> test:ok(true, 'BC_VARG recording 0th frame depth')
>>> >> 
>>> >>+-- Now the same case, but with additional frame, so VARG slots
>>> >Typo: s/with/with an/
>>
>>Fixed, thanks!
>>
>>> >>+-- are defined on the trace.
>>> >>+local function bump_varg_frame(...)
>>> >Nit: Not sure about the name here, maybe it is better to call it
>>> >just `varg_frame` or something like that. Feel free to ignore, though.
>>
>>Changed to the `varg_frame()`.
>>
>>See the iterative patch below.
>>
>>===================================================================
>>diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>index f8d74e8a..cdefbe83 100644
>>--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>@@ -20,9 +20,9 @@ end
>> 
>> test:ok(true, 'BC_VARG recording 0th frame depth')
>> 
>>--- Now the same case, but with additional frame, so VARG slots
>>+-- Now the same case, but with an additional frame, so VARG slots
>> -- are defined on the trace.
>>-local function bump_varg_frame(...)
>>+local function varg_frame(...)
>>   -- BC_VARG 1 1 0. `...` is nil (argument for the script).
>>   -- luacheck: ignore
>>   -- XXX: some condition to use several slots on the Lua stack.
>>@@ -32,7 +32,7 @@ end
>> counter = 0
>> while counter < 3 do
>>   counter = counter + 1
>>- bump_varg_frame()
>>+ varg_frame()
>> end
>> 
>> test:ok(true, 'BC_VARG recording with defined on trace VARG slots')
>>===================================================================
>>
>>> >>+ -- BC_VARG 1 1 0. `...` is nil (argument for the script).
>>> >>+ -- luacheck: ignore
>>> >>+ -- XXX: some condition to use several slots on the Lua stack.
>>> >>+ anchor = 1 >= 1, ...
>>> >>+end
>>> >>+
>>> >>+counter = 0
>>> >>+while counter < 3 do
>>> >>+ counter = counter + 1
>>> >>+ bump_varg_frame()
>>> >>+end
>>> >>+
>>> >>+test:ok(true, 'BC_VARG recording with defined on trace VARG slots')
>>> >>+
>>> >> os.exit(test:check() and 0 or 1)
>>> >>--
>>> >>2.34.1
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>[1]:  https://english.stackexchange.com/questions/352253/why-does-oxforddictionaries-com-not-use-a-comma-after-the-abbreviations-i-e-a
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

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