Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] x86/x64: Add more red zone checks to assembler backend.
Date: Fri, 17 Jan 2025 11:31:27 +0300	[thread overview]
Message-ID: <Z4oVX_40bpmgGujv@root> (raw)
In-Reply-To: <e177a0ed-27b0-46b1-bbe3-bbcec1d28f8c@tarantool.org>

Hi, Sergey!
Thanks for the review!
Fixed your comments below.

Branch is force-pushed.

On 16.01.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! See comments below.
> 
> 
> On 16.01.2025 16:35, Sergey Kaplun wrote:
> 
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-1116-redzones-checks.test.lua b/test/tarantool-tests/lj-1116-redzones-checks.test.lua
> > new file mode 100644
> > index 00000000..70062ec9
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1116-redzones-checks.test.lua
> > @@ -0,0 +1,118 @@
> > +local tap = require('tap')
> > +-- Test file to demonstrate mcode area overflow during recording a
> > +-- trace with the high FPR pressure.
> > +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1116.
> > +--
> > +-- XXX: Test fails only with GC64 enabled before the commit.
> I would rephrase: XXX: Test fails with reverted fix and enabled GC64.

Rephrased as you suggested. See the iterative patch below.

> > +local test = tap.test('lj-1116-redzones-checks'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(1)
> > +

<snipped>

> > +--
> > +-- [1]:https://wiki.osdev.org/X86-64_Instruction_Encoding#REX_prefix
> > +
> > +_G.a = 0
> > +_G.b = 0
> > +_G.c = 0
> > +_G.d = 0
> > +_G.e = 0
> > +_G.f = 0
> > +_G.g = 0
> > +_G.h = 0
> > +-- Skip `i`.
> 
> I didn't get it.

Updated the commit:

===================================================================
diff --git a/test/tarantool-tests/lj-1116-redzones-checks.test.lua b/test/tarantool-tests/lj-1116-redzones-checks.test.lua
index 70062ec9..4f4f5870 100644
--- a/test/tarantool-tests/lj-1116-redzones-checks.test.lua
+++ b/test/tarantool-tests/lj-1116-redzones-checks.test.lua
@@ -3,7 +3,7 @@ local tap = require('tap')
 -- trace with the high FPR pressure.
 -- See also, https://github.com/LuaJIT/LuaJIT/issues/1116.
 --
--- XXX: Test fails only with GC64 enabled before the commit.
+-- XXX: Test fails with reverted fix and enabled GC64 mode.
 local test = tap.test('lj-1116-redzones-checks'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
@@ -54,7 +54,7 @@ _G.e = 0
 _G.f = 0
 _G.g = 0
 _G.h = 0
--- Skip `i`.
+-- Skip `i` -- it is used for the loop index.
 _G.j = 0
 _G.k = 0
 _G.l = 0
===================================================================

> 
> 
> <snipped>

-- 
Best regards,
Sergey Kaplun

  reply	other threads:[~2025-01-17  8:32 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-16 13:35 Sergey Kaplun via Tarantool-patches
2025-01-16 14:55 ` Sergey Bronnikov via Tarantool-patches
2025-01-17  8:31   ` Sergey Kaplun via Tarantool-patches [this message]
2025-01-17 10:07     ` Sergey Bronnikov 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=Z4oVX_40bpmgGujv@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] x86/x64: Add more red zone checks to assembler backend.' \
    /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