Tarantool development patches archive
 help / color / mirror / Atom feed
From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_VARG, part 3.
Date: Wed, 16 Aug 2023 16:51:21 +0300	[thread overview]
Message-ID: <svqoezjou4b3i4mn6mciryalg56bjhjlqz5gw7iv5m5bfec5le@bo3qjmjk2vwh> (raw)
In-Reply-To: <20230815123215.510-1-skaplun@tarantool.org>

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Tue, Aug 15, 2023 at 03:32:15PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)
> 
> In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
Typo: s/set/sets/
> `maxslot` value was unconditionally set to the destination slot, so some
> top slots may be omitted in the snapshot entry. Since these slots are
> omitted, they are not restored correctly, when restoring from snapshot
Typo: s/snapshot/a snapshot/
> for this side exit.
> 
> This patch adds the check for the aforementioned case, to avoid maxslot
> shrinking.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording
> PR: https://github.com/tarantool/tarantool/pull/8986
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1046
> * https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_record.c                               | 12 +++-
>  .../lj-1046-fix-bc-varg-recording.test.lua    | 58 +++++++++++++++++++
>  2 files changed, 67 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> 
> diff --git a/src/lj_record.c b/src/lj_record.c
> index 34d1210a..6bcdb04c 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -1807,8 +1807,12 @@ 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)nresults;
> +    if (nresults != 1) {
> +      if (nresults == -1) nresults = nvararg;
> +      J->maxslot = dst + (BCReg)nresults;
> +    } else if (dst >= J->maxslot) {
> +      J->maxslot = dst + 1;
> +    }
>      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. */
> @@ -1840,7 +1844,9 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>        }
>        for (i = nvararg; i < nresults; i++)
>  	J->base[dst+i] = TREF_NIL;
> -      J->maxslot = dst + (BCReg)nresults;
> +      if (nresults != 1 || dst >= J->maxslot) {
> +	J->maxslot = dst + (BCReg)nresults;
> +      }
>      } else if (select_detect(J)) {  /* y = select(x, ...) */
>        TRef tridx = J->base[dst-1];
>        TRef tr = TREF_NIL;
> diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> new file mode 100644
> index 00000000..34c5c572
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> @@ -0,0 +1,58 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1')
> +
> +-- luacheck: ignore
> +local anchor
> +local N_ITER = 5
> +local SIDE_ITER = N_ITER - 1
> +for i = 1, N_ITER do
> +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> +  -- stack slot, `maxslot` value was unconditionally set to the
> +  -- destination slot, so the following snapshot is used:
> +  -- SNAP   #4   [ ---- ---- ---- nil  ]
> +  -- instead of:
> +  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
Snapshot examples here ceratinly give the idea of what goes wrong,
but the `0009` and `0001` are meaningless by themselves. I think it would be
nice to include IRs here too.
> +  -- Since these slots are omitted, they are not restored
> +  -- correctly, when restoring from snapshot for this side exit.
Please fix the same typos as in the commit message here.
> +  anchor = ...
> +  if i > SIDE_ITER then
> +    -- XXX: Don't use `test:ok()` here to avoid double-running of
I think better phrasing would be:
| `test:ok()` is not used here ...
> +    -- tests in case of `i` incorrect restoring from the snapshot.
Typo: s/restoring/restoration/
> +    assert(i > SIDE_ITER)
> +  end
> +end
> +
> +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
> +
> +-- Now the same case, but with an additional frame, so VARG slots
> +-- are defined on the trace.
> +local function varg_frame(anchor, i, side_iter, ...)
> +  anchor = ...
> +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> +  -- stack slot, `maxslot` value was unconditionally set to the
> +  -- destination slot, so the following snapshot is used:
> +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
> +  -- instead of:
> +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
> +  -- Since these slots are omitted, they are not restored
> +  -- correctly, when restoring from snapshot for this side exit.
I guess we don't need to repeat the entire comment again.
> +  if i > side_iter then
> +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> +    -- tests in case of `i` incorrect restoring from the snapshot.
Same typos as in the comment above.
> +    assert(i > side_iter)
> +  end
> +end
> +
> +for i = 1, N_ITER do
> +  varg_frame(nil, i, SIDE_ITER)
> +end
> +
> +test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 result')
> +
> +test:done(true)
> -- 
> 2.41.0
> 

  reply	other threads:[~2023-08-16 13:51 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-15 12:32 Sergey Kaplun via Tarantool-patches
2023-08-16 13:51 ` Maxim Kokryashkin via Tarantool-patches [this message]
2023-08-16 16:11   ` Sergey Kaplun via Tarantool-patches
2023-08-17 13:16     ` Maxim Kokryashkin via Tarantool-patches
2023-08-21 13:38 ` Sergey Bronnikov via Tarantool-patches
2023-08-22 15:44   ` Sergey Kaplun via Tarantool-patches
2023-08-28 13:15 ` Sergey Bronnikov via Tarantool-patches
2023-08-31 15:18 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=svqoezjou4b3i4mn6mciryalg56bjhjlqz5gw7iv5m5bfec5le@bo3qjmjk2vwh \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_VARG, part 3.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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