[tarantool-patches] [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[tarantool-patches] [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Cyrill Gorcunov]

The series fixes segfault when unsinking 64-bit pointers.
Note that I've backported a patch and a test for us but
the problem didn't trigger for me even when luajit2 is
unpatched.

That said up to you, still think it might be usefull for
case #4171

-- 
2.20.1

[tarantool-patches] [PATCH 1/2] bugfix: fixed a segfault when unsinking 64-bit pointers.

[Cyrill Gorcunov]

From: Thibault Charbonnier <thibaultcha@me.com>

The unsinking code was not using the correct layout for GC64 IR
constants (value in adjacent slot) for this case.

This patch is a derivative of
https://github.com/raptorjit/raptorjit/pull/246 ported for LuaJIT
itself.

Fixed after an intense debugging session with @lukego.

Co-authored-by: Luke Gorrie <lukego@gmail.com>
---
 src/lj_ir.h   | 12 ++++++------
 src/lj_snap.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/lj_ir.h b/src/lj_ir.h
index 34c2785..3059bf6 100644
--- a/src/lj_ir.h
+++ b/src/lj_ir.h
@@ -560,6 +560,11 @@ typedef union IRIns {
   TValue tv;		/* TValue constant (overlaps entire slot). */
 } IRIns;
 
+#define ir_isk64(ir) ((ir)->o == IR_KNUM || (ir)->o == IR_KINT64 || \
+                      (LJ_GC64 && \
+                       ((ir)->o == IR_KGC || \
+                       (ir)->o == IR_KPTR || (ir)->o == IR_KKPTR)))
+
 #define ir_kgc(ir)	check_exp((ir)->o == IR_KGC, gcref((ir)[LJ_GC64].gcr))
 #define ir_kstr(ir)	(gco2str(ir_kgc((ir))))
 #define ir_ktab(ir)	(gco2tab(ir_kgc((ir))))
@@ -567,12 +572,7 @@ typedef union IRIns {
 #define ir_kcdata(ir)	(gco2cd(ir_kgc((ir))))
 #define ir_knum(ir)	check_exp((ir)->o == IR_KNUM, &(ir)[1].tv)
 #define ir_kint64(ir)	check_exp((ir)->o == IR_KINT64, &(ir)[1].tv)
-#define ir_k64(ir) \
-  check_exp((ir)->o == IR_KNUM || (ir)->o == IR_KINT64 || \
-	    (LJ_GC64 && \
-	     ((ir)->o == IR_KGC || \
-	      (ir)->o == IR_KPTR || (ir)->o == IR_KKPTR)), \
-	    &(ir)[1].tv)
+#define ir_k64(ir)	check_exp(ir_isk64(ir), &(ir)[1].tv)
 #define ir_kptr(ir) \
   check_exp((ir)->o == IR_KPTR || (ir)->o == IR_KKPTR, \
     mref((ir)[LJ_GC64].ptr, void))
diff --git a/src/lj_snap.c b/src/lj_snap.c
index 18ce715..7554caf 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -685,7 +685,7 @@ static void snap_restoredata(GCtrace *T, ExitState *ex,
   int32_t *src;
   uint64_t tmp;
   if (irref_isk(ref)) {
-    if (ir->o == IR_KNUM || ir->o == IR_KINT64) {
+    if (ir_isk64(ir)) {
       src = (int32_t *)&ir[1];
     } else if (sz == 8) {
       tmp = (uint64_t)(uint32_t)ir->i;
-- 
2.20.1

[tarantool-patches] [PATCH 2/2] test/luajit-tap: Add unsink_64_kptr.test.lua

[Cyrill Gorcunov]

Backport of openrusty/luajit2-test-suite
commit 907c536c210ebe6a147861bb4433d28c0ebfc8cd

To test unsink 64 bit pointers

Part-of #4171
---
 test/luajit-tap/unsink_64_kptr.test.lua | 44 +++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100755 test/luajit-tap/unsink_64_kptr.test.lua

diff --git a/test/luajit-tap/unsink_64_kptr.test.lua b/test/luajit-tap/unsink_64_kptr.test.lua
new file mode 100755
index 000000000..89957637b
--- /dev/null
+++ b/test/luajit-tap/unsink_64_kptr.test.lua
@@ -0,0 +1,44 @@
+#!/usr/bin/env tarantool
+
+tap = require('tap')
+
+test = tap.test("232")
+test:plan(1)
+
+--- From: Thibault Charbonnier <thibaultcha@me.com>
+--- tests: ffi: added a test case unsinking a 64-bit pointer from a constant.
+---
+--- This test case reproduces the issue observed at:
+--- https://github.com/openresty/lua-resty-core/issues/232 and was
+--- contributed by @lukego and myself.
+---
+--- Co-authored-by: Luke Gorrie <lukego@gmail.com>
+---
+local ffi = require("ffi")
+
+local array = ffi.new("struct { int x; } [1]")
+
+-- This test forces the VM to unsink a pointer that was constructed
+-- from a constant. The IR will include a 'cnewi' instruction to
+-- allocate an FFI pointer object, the pointer value will be an IR
+-- constant, the allocation will be sunk, and the allocation will
+-- at some point be "unsunk" due to a reference in the snapshot for
+-- a taken exit.
+
+-- Note: JIT will recognize <array> as a "singleton" and allow its
+-- address to be inlined ("constified") instead of looking up the
+-- upvalue at runtime.
+
+local function fn(i)
+    local struct = array[0]     -- Load pointer that the JIT will constify.
+    if i == 1000 then end       -- Force trace exit when i==1000.
+    struct.x = 0                -- Ensure that 'struct' is live after exit.
+end
+
+-- Loop over the function to make it compile and take a trace exit
+-- during the final iteration.
+for i = 1, 1000 do
+    fn(i)
+end
+
+test:ok("PASS")
-- 
2.20.1

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Cyrill Gorcunov]

On Thu, May 23, 2019 at 08:46:32PM +0300, Cyrill Gorcunov wrote:
> The series fixes segfault when unsinking 64-bit pointers.
> Note that I've backported a patch and a test for us but
> the problem didn't trigger for me even when luajit2 is
> unpatched.
> 
> That said up to you, still think it might be usefull for
> case #4171

Forgot to mention, patch 1 for luajit2 submodule and test is
for tarantool itself.

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Alexander Turenko]

On Thu, May 23, 2019 at 08:46:32PM +0300, Cyrill Gorcunov wrote:
> The series fixes segfault when unsinking 64-bit pointers.
> Note that I've backported a patch and a test for us but
> the problem didn't trigger for me even when luajit2 is
> unpatched.

Did you try with -DLUAJIT_ENABLE_GC64=ON cmake option?

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Cyrill Gorcunov]

On Fri, May 24, 2019 at 08:38:33PM +0300, Alexander Turenko wrote:
> On Thu, May 23, 2019 at 08:46:32PM +0300, Cyrill Gorcunov wrote:
> > The series fixes segfault when unsinking 64-bit pointers.
> > Note that I've backported a patch and a test for us but
> > the problem didn't trigger for me even when luajit2 is
> > unpatched.
> 
> Did you try with -DLUAJIT_ENABLE_GC64=ON cmake option?

I didn't. Rerun now with -DLUAJIT_ENABLE_GC64=ON and without
the patch test fails indeed. Thus we need this fix together
with a test. Thanks for the hint!

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Alexander Turenko]

LGTM.

Kirill, please proceed. I think we should push this fix for all
currently supported branches.

Please, close issue https://github.com/tarantool/tarantool/issues/4072
from the commit message in a commit for tarantool repo.

WBR, Alexader Turenko.

On Thu, May 23, 2019 at 08:46:32PM +0300, Cyrill Gorcunov wrote:
> The series fixes segfault when unsinking 64-bit pointers.
> Note that I've backported a patch and a test for us but
> the problem didn't trigger for me even when luajit2 is
> unpatched.
> 
> That said up to you, still think it might be usefull for
> case #4171

This issue is reproduced on Linux, where we don't use GC64, so it is not
related.

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Kirill Yukhin]

Hello,

On 23 May 20:46, Cyrill Gorcunov wrote:
> The series fixes segfault when unsinking 64-bit pointers.
> Note that I've backported a patch and a test for us but
> the problem didn't trigger for me even when luajit2 is
> unpatched.
> 
> That said up to you, still think it might be usefull for
> case #4171

I've checked your patches into 1.10, 2.1 and master.

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Alexander Turenko]

On Tue, May 28, 2019 at 05:02:12AM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 23 May 20:46, Cyrill Gorcunov wrote:
> > The series fixes segfault when unsinking 64-bit pointers.
> > Note that I've backported a patch and a test for us but
> > the problem didn't trigger for me even when luajit2 is
> > unpatched.
> > 
> > That said up to you, still think it might be usefull for
> > case #4171
> 
> I've checked your patches into 1.10, 2.1 and master.

Kirill, please, be careful. In the previous email I asked to close the
issue:

> Please, close issue https://github.com/tarantool/tarantool/issues/4072
> from the commit message in a commit for tarantool repo.

Anyway, I closed it myself.

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Cyrill Gorcunov]

On Tue, May 28, 2019 at 03:01:55PM +0300, Alexander Turenko wrote:
> On Tue, May 28, 2019 at 05:02:12AM +0300, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 23 May 20:46, Cyrill Gorcunov wrote:
> > > The series fixes segfault when unsinking 64-bit pointers.
> > > Note that I've backported a patch and a test for us but
> > > the problem didn't trigger for me even when luajit2 is
> > > unpatched.
> > > 
> > > That said up to you, still think it might be usefull for
> > > case #4171
> > 
> > I've checked your patches into 1.10, 2.1 and master.
> 
> Kirill, please, be careful. In the previous email I asked to close the
> issue:
> 
> > Please, close issue https://github.com/tarantool/tarantool/issues/4072
> > from the commit message in a commit for tarantool repo.
> 
> Anyway, I closed it myself.

Ouch, sorry. Thank you!

[tarantool-patches] Re: [PATCH 0/2] luajit2: bugfix: fixed a segfault when unsinking 64-bit pointers

[Alexander Turenko]

On Tue, May 28, 2019 at 03:11:21PM +0300, Cyrill Gorcunov wrote:
> On Tue, May 28, 2019 at 03:01:55PM +0300, Alexander Turenko wrote:
> > On Tue, May 28, 2019 at 05:02:12AM +0300, Kirill Yukhin wrote:
> > > Hello,
> > > 
> > > On 23 May 20:46, Cyrill Gorcunov wrote:
> > > > The series fixes segfault when unsinking 64-bit pointers.
> > > > Note that I've backported a patch and a test for us but
> > > > the problem didn't trigger for me even when luajit2 is
> > > > unpatched.
> > > > 
> > > > That said up to you, still think it might be usefull for
> > > > case #4171
> > > 
> > > I've checked your patches into 1.10, 2.1 and master.
> > 
> > Kirill, please, be careful. In the previous email I asked to close the
> > issue:
> > 
> > > Please, close issue https://github.com/tarantool/tarantool/issues/4072
> > > from the commit message in a commit for tarantool repo.
> > 
> > Anyway, I closed it myself.
> 
> Ouch, sorry. Thank you!

This was the message for Kirill Yukhin, because he created the commit
with the submodule update.