[Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Kaplun.

(cherry picked from commit 7a608e4425ce0777f5c980dad9f4fdc1bcce0b8c)

The aforementioned function doesn't handle gentle recording of the cdata
addition to `nil` or some string, presuming that the interpreter will throw
an error. This may lead to an assertion due to an uninitialized ctype
state or an attempt to use in the fold engine the non-cdata summand (casted
to `IR_KPTR`) as the (invalid) GC pointer.

This patch handles such cases by:
* Initializing the ctype state where it is needed.
* Raising an error when the argument has a suspicious type. Since the
  interpreter will throw the error anyway, these traces will abort
  anyway.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#10199
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1224-fix-jit-cdata-arith
Related issues:
* https://github.com/tarantool/tarantool/issues/10199
* https://github.com/LuaJIT/LuaJIT/issues/1224

 src/lj_crecord.c                              | 10 ++--
 .../lj-1224-fix-cdata-arith-ptr.test.lua      | 48 +++++++++++++++++++
 .../lj-1224-fix-cdata-arith-side.test.lua     | 20 ++++++++
 .../lj-1224-fix-cdata-arith-side/script.lua   | 16 +++++++
 .../lj-1224-fix-cdata-arith.test.lua          | 20 ++++++++
 .../lj-1224-fix-cdata-arith/script.lua        | 18 +++++++
 6 files changed, 128 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
 create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua
 create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua
 create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua
 create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua

diff --git a/src/lj_crecord.c b/src/lj_crecord.c
index 255bfa45..81ae6dfa 100644
--- a/src/lj_crecord.c
+++ b/src/lj_crecord.c
@@ -1473,7 +1473,8 @@ static TRef crec_arith_meta(jit_State *J, TRef *sp, CType **s, CTState *cts,
 
 void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
 {
-  CTState *cts = ctype_ctsG(J2G(J));
+  CTState *cts = ctype_cts(J->L);
+  MMS mm = (MMS)rd->data;
   TRef sp[2];
   CType *s[2];
   MSize i;
@@ -1523,6 +1524,8 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
 	}
       }
     } else if (tref_isnil(tr)) {
+      if (!(mm == MM_len || mm == MM_eq || mm == MM_lt || mm == MM_le))
+	lj_trace_err(J, LJ_TRERR_BADTYPE);
       tr = lj_ir_kptr(J, NULL);
       ct = ctype_get(cts, CTID_P_VOID);
     } else if (tref_isinteger(tr)) {
@@ -1541,12 +1544,12 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
 	  ct = ctype_child(cts, cct);
 	  tr = lj_ir_kint(J, (int32_t)ofs);
 	} else {  /* Interpreter will throw or return false. */
-	  ct = ctype_get(cts, CTID_P_VOID);
+	  lj_trace_err(J, LJ_TRERR_BADTYPE);
 	}
       } else if (ctype_isptr(ct->info)) {
 	tr = emitir(IRT(IR_ADD, IRT_PTR), tr, lj_ir_kintp(J, sizeof(GCstr)));
       } else {
-	ct = ctype_get(cts, CTID_P_VOID);
+	lj_trace_err(J, LJ_TRERR_BADTYPE);
       }
     } else if (!tref_isnum(tr)) {
       tr = 0;
@@ -1558,7 +1561,6 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
   }
   {
     TRef tr;
-    MMS mm = (MMS)rd->data;
     if ((mm == MM_len || mm == MM_concat ||
 	 (!(tr = crec_arith_int64(J, sp, s, mm)) &&
 	  !(tr = crec_arith_ptr(J, sp, s, mm)))) &&
diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
new file mode 100644
index 00000000..9de9586d
--- /dev/null
+++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
@@ -0,0 +1,48 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's incorrect recording of cdata
+-- arithmetic looks like pointer arithmetic, which raises the
+-- error.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1224.
+
+local test = tap.test('lj-1224-fix-cdata-arith-ptr'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(2)
+
+local function arith_nil_protected()
+  local i = 1
+  while i < 3 do
+    -- Before the patch, `nil` was "cast" to `IR_KPTR(NULL)`
+    -- during the recording of the cdata arith metamethod, and the
+    -- fold engine tried to add this value to the `-1LL`. This
+    -- obviously leads to the value being out-of-range for the GC
+    -- pointer.
+    local _ = -1LL + nil
+    i = i + 1
+  end
+end
+
+-- The similar test but for the string value instead of nil.
+local function arith_str_protected()
+  local i = 1
+  while i < 3 do
+    local _ = 1LL + ''
+    i = i + 1
+  end
+end
+
+local function check_error(subtest, test_f)
+  subtest:plan(2)
+  -- Reset counters.
+  jit.opt.start('hotloop=1')
+  local result, errmsg = pcall(test_f)
+  subtest:ok(not result, 'correct recording error with bad cdata arithmetic')
+  subtest:like(errmsg, 'attempt to perform arithmetic', 'correct error message')
+end
+
+test:test('cdata arithmetic with nil',    check_error, arith_nil_protected)
+test:test('cdata arithmetic with string', check_error, arith_str_protected)
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua
new file mode 100644
index 00000000..3f3e6011
--- /dev/null
+++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua
@@ -0,0 +1,20 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's incorrect recording for the
+-- side trace with cdata arithmetic, which raises the error.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1224.
+
+local test = tap.test('lj-1224-fix-cdata-arith-side'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+-- The loading of the 'tap' module initializes `cts->L` during
+-- parsing. Run standalone script for testing.
+local script = require('utils').exec.makecmd(arg)
+
+test:plan(1)
+
+local output = script()
+test:is(output, 'OK', 'correct recording with uninitialized cts->L')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua
new file mode 100644
index 00000000..e96877ff
--- /dev/null
+++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua
@@ -0,0 +1,16 @@
+local function test_record_protected()
+  -- Start the root trace here.
+  for _ = 1, 3 do end
+  -- An exit from the root trace triggered the side trace
+  -- recording.
+  local _ = 1LL + ''
+end
+
+jit.opt.start('hotloop=1', 'hotexit=1')
+
+local status, errmsg = pcall(test_record_protected)
+
+assert(not status, 'recoding correctly handling the error')
+assert(errmsg:match('attempt to perform arithmetic'), 'correct error message')
+
+print('OK')
diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua
new file mode 100644
index 00000000..ba89b472
--- /dev/null
+++ b/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua
@@ -0,0 +1,20 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's incorrect recording of cdata
+-- arithmetic, which raises the error.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1224.
+
+local test = tap.test('lj-1224-fix-cdata-arith'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+-- The loading of the 'tap' module initializes `cts->L` during
+-- parsing. Run standalone script for testing.
+local script = require('utils').exec.makecmd(arg)
+
+test:plan(1)
+
+local output = script()
+test:is(output, 'OK', 'correct recording with uninitialized cts->L')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua
new file mode 100644
index 00000000..df79e430
--- /dev/null
+++ b/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua
@@ -0,0 +1,18 @@
+local function test_record_protected()
+  local i = 1
+  while i < 3 do
+    -- Use `while` to start compilation of the trace at the first
+    -- iteration, before `cts->L` is uninitialized.
+    local _ = 1LL + nil
+    i = i + 1
+  end
+end
+
+jit.opt.start('hotloop=1')
+
+local status, errmsg = pcall(test_record_protected)
+
+assert(not status, 'recoding correctly handling the error')
+assert(errmsg:match('attempt to perform arithmetic'), 'correct error message')
+
+print('OK')
-- 
2.45.2

Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for the patch! see my comments below.

On 21.08.2024 19:52, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit 7a608e4425ce0777f5c980dad9f4fdc1bcce0b8c)
>
> The aforementioned function doesn't handle gentle recording of the cdata
> addition to `nil` or some string, presuming that the interpreter will throw
> an error. This may lead to an assertion due to an uninitialized ctype
> state or an attempt to use in the fold engine the non-cdata summand (casted
> to `IR_KPTR`) as the (invalid) GC pointer.
>
> This patch handles such cases by:
> * Initializing the ctype state where it is needed.
> * Raising an error when the argument has a suspicious type. Since the
>    interpreter will throw the error anyway, these traces will abort
>    anyway.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10199
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1224-fix-jit-cdata-arith
> Related issues:
> *https://github.com/tarantool/tarantool/issues/10199
> *https://github.com/LuaJIT/LuaJIT/issues/1224
>
>   src/lj_crecord.c                              | 10 ++--
>   .../lj-1224-fix-cdata-arith-ptr.test.lua      | 48 +++++++++++++++++++

This test does not fail without fix (but repro from the issue does):

[0] ~/sources/MRG/tarantool/third_party/luajit $ ./build/gc64/src/luajit 
-Ohotloop=1 -e "
repeat
   r = 1LL + nil
until true
"
LuaJIT ASSERT 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_ctype.c:185: 
lj_ctype_intern: uninitialized cts->L
Aborted
[0] ~/sources/MRG/tarantool/third_party/luajit $ ./build/gc64/src/luajit 
test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
TAP version 13
1..2
     # cdata arithmetic with nil
     1..2
     ok - correct recording error with bad cdata arithmetic
     ok - correct error message
     # cdata arithmetic with nil: end
ok - cdata arithmetic with nil
     # cdata arithmetic with string
     1..2
     ok - correct recording error with bad cdata arithmetic
     ok - correct error message
     # cdata arithmetic with string: end
ok - cdata arithmetic with string
[0] ~/sources/MRG/tarantool/third_party/luajit $


>   .../lj-1224-fix-cdata-arith-side.test.lua     | 20 ++++++++
>   .../lj-1224-fix-cdata-arith-side/script.lua   | 16 +++++++
>   .../lj-1224-fix-cdata-arith.test.lua          | 20 ++++++++
>   .../lj-1224-fix-cdata-arith/script.lua        | 18 +++++++
>   6 files changed, 128 insertions(+), 4 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
>   create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua
>   create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua
>   create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua
>   create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua
>
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index 255bfa45..81ae6dfa 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -1473,7 +1473,8 @@ static TRef crec_arith_meta(jit_State *J, TRef *sp, CType **s, CTState *cts,
>   
>   void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>   {
> -  CTState *cts = ctype_ctsG(J2G(J));
> +  CTState *cts = ctype_cts(J->L);
> +  MMS mm = (MMS)rd->data;
>     TRef sp[2];
>     CType *s[2];
>     MSize i;
> @@ -1523,6 +1524,8 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>   	}
>         }
>       } else if (tref_isnil(tr)) {
> +      if (!(mm == MM_len || mm == MM_eq || mm == MM_lt || mm == MM_le))
> +	lj_trace_err(J, LJ_TRERR_BADTYPE);
>         tr = lj_ir_kptr(J, NULL);
>         ct = ctype_get(cts, CTID_P_VOID);
>       } else if (tref_isinteger(tr)) {
> @@ -1541,12 +1544,12 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>   	  ct = ctype_child(cts, cct);
>   	  tr = lj_ir_kint(J, (int32_t)ofs);
>   	} else {  /* Interpreter will throw or return false. */
> -	  ct = ctype_get(cts, CTID_P_VOID);
> +	  lj_trace_err(J, LJ_TRERR_BADTYPE);
>   	}
>         } else if (ctype_isptr(ct->info)) {
>   	tr = emitir(IRT(IR_ADD, IRT_PTR), tr, lj_ir_kintp(J, sizeof(GCstr)));
>         } else {
> -	ct = ctype_get(cts, CTID_P_VOID);
> +	lj_trace_err(J, LJ_TRERR_BADTYPE);
>         }
>       } else if (!tref_isnum(tr)) {
>         tr = 0;
> @@ -1558,7 +1561,6 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>     }
>     {
>       TRef tr;
> -    MMS mm = (MMS)rd->data;
>       if ((mm == MM_len || mm == MM_concat ||
>   	 (!(tr = crec_arith_int64(J, sp, s, mm)) &&
>   	  !(tr = crec_arith_ptr(J, sp, s, mm)))) &&
> diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> new file mode 100644
> index 00000000..9de9586d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> @@ -0,0 +1,48 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's incorrect recording of cdata
> +-- arithmetic looks like pointer arithmetic, which raises the
> +-- error.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1224.
> +
> +local test = tap.test('lj-1224-fix-cdata-arith-ptr'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +local function arith_nil_protected()
> +  local i = 1
> +  while i < 3 do
> +    -- Before the patch, `nil` was "cast" to `IR_KPTR(NULL)`
> +    -- during the recording of the cdata arith metamethod, and the
> +    -- fold engine tried to add this value to the `-1LL`. This
> +    -- obviously leads to the value being out-of-range for the GC
> +    -- pointer.
> +    local _ = -1LL + nil
> +    i = i + 1
> +  end
> +end
> +
> +-- The similar test but for the string value instead of nil.
> +local function arith_str_protected()
> +  local i = 1
> +  while i < 3 do
> +    local _ = 1LL + ''
> +    i = i + 1
> +  end
> +end
> +
> +local function check_error(subtest, test_f)
> +  subtest:plan(2)
> +  -- Reset counters.
> +  jit.opt.start('hotloop=1')
> +  local result, errmsg = pcall(test_f)
> +  subtest:ok(not result, 'correct recording error with bad cdata arithmetic')
> +  subtest:like(errmsg, 'attempt to perform arithmetic', 'correct error message')
> +end
> +
> +test:test('cdata arithmetic with nil',    check_error, arith_nil_protected)
> +test:test('cdata arithmetic with string', check_error, arith_str_protected)
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua
> new file mode 100644
> index 00000000..3f3e6011
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua
> @@ -0,0 +1,20 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's incorrect recording for the
> +-- side trace with cdata arithmetic, which raises the error.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1224.
> +
> +local test = tap.test('lj-1224-fix-cdata-arith-side'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +-- The loading of the 'tap' module initializes `cts->L` during
> +-- parsing. Run standalone script for testing.
> +local script = require('utils').exec.makecmd(arg)
> +
> +test:plan(1)
> +
> +local output = script()
> +test:is(output, 'OK', 'correct recording with uninitialized cts->L')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua
> new file mode 100644
> index 00000000..e96877ff
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua
> @@ -0,0 +1,16 @@
> +local function test_record_protected()
> +  -- Start the root trace here.
> +  for _ = 1, 3 do end
> +  -- An exit from the root trace triggered the side trace
> +  -- recording.
> +  local _ = 1LL + ''
> +end
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +local status, errmsg = pcall(test_record_protected)
> +
> +assert(not status, 'recoding correctly handling the error')
> +assert(errmsg:match('attempt to perform arithmetic'), 'correct error message')
> +
> +print('OK')
> diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua
> new file mode 100644
> index 00000000..ba89b472
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua
> @@ -0,0 +1,20 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT's incorrect recording of cdata
> +-- arithmetic, which raises the error.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1224.
> +
> +local test = tap.test('lj-1224-fix-cdata-arith'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +-- The loading of the 'tap' module initializes `cts->L` during
> +-- parsing. Run standalone script for testing.
> +local script = require('utils').exec.makecmd(arg)
> +
> +test:plan(1)
> +
> +local output = script()
> +test:is(output, 'OK', 'correct recording with uninitialized cts->L')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua
> new file mode 100644
> index 00000000..df79e430
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua
> @@ -0,0 +1,18 @@
> +local function test_record_protected()
> +  local i = 1
> +  while i < 3 do
> +    -- Use `while` to start compilation of the trace at the first
> +    -- iteration, before `cts->L` is uninitialized.
> +    local _ = 1LL + nil
> +    i = i + 1
> +  end
> +end
> +
> +jit.opt.start('hotloop=1')
> +
> +local status, errmsg = pcall(test_record_protected)
> +
> +assert(not status, 'recoding correctly handling the error')
> +assert(errmsg:match('attempt to perform arithmetic'), 'correct error message')
> +
> +print('OK')

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

Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

On 09.09.2024 18:37, Sergey Bronnikov via Tarantool-patches wrote:
>
> Hi, Sergey,
>
> thanks for the patch! see my comments below.
>
> On 21.08.2024 19:52, Sergey Kaplun wrote:
>> From: Mike Pall <mike>
>>
>> Thanks to Sergey Kaplun.
>>
>> (cherry picked from commit 7a608e4425ce0777f5c980dad9f4fdc1bcce0b8c)
>>
>> The aforementioned function doesn't handle gentle recording of the cdata
>> addition to `nil` or some string, presuming that the interpreter will throw
>> an error. This may lead to an assertion due to an uninitialized ctype
>> state or an attempt to use in the fold engine the non-cdata summand (casted
>> to `IR_KPTR`) as the (invalid) GC pointer.
>>
>> This patch handles such cases by:
>> * Initializing the ctype state where it is needed.
>> * Raising an error when the argument has a suspicious type. Since the
>>    interpreter will throw the error anyway, these traces will abort
>>    anyway.
>>
>> Sergey Kaplun:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#10199
>> ---
>>
>> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1224-fix-jit-cdata-arith
>> Related issues:
>> *https://github.com/tarantool/tarantool/issues/10199
>> *https://github.com/LuaJIT/LuaJIT/issues/1224
>>
>>   src/lj_crecord.c                              | 10 ++--
>>   .../lj-1224-fix-cdata-arith-ptr.test.lua      | 48 +++++++++++++++++++
>
> This test does not fail without fix (but repro from the issue does):
>
> [0] ~/sources/MRG/tarantool/third_party/luajit $ 
> ./build/gc64/src/luajit -Ohotloop=1 -e "
> repeat
>   r = 1LL + nil
> until true
> "
> LuaJIT ASSERT 
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_ctype.c:185: 
> lj_ctype_intern: uninitialized cts->L
> Aborted
> [0] ~/sources/MRG/tarantool/third_party/luajit $ 
> ./build/gc64/src/luajit 
> test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> TAP version 13
> 1..2
>     # cdata arithmetic with nil
> 1..2
>     ok - correct recording error with bad cdata arithmetic
>     ok - correct error message
>     # cdata arithmetic with nil: end
> ok - cdata arithmetic with nil
>     # cdata arithmetic with string
>     1..2
>     ok - correct recording error with bad cdata arithmetic
>     ok - correct error message
>     # cdata arithmetic with string: end
> ok - cdata arithmetic with string
> [0] ~/sources/MRG/tarantool/third_party/luajit $
>

With GC64 only (LUAJIT_ENABLE_GC64).


<snipped>

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

Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 10.09.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> On 09.09.2024 18:37, Sergey Bronnikov via Tarantool-patches wrote:
> >
> > Hi, Sergey,
> >
> > thanks for the patch! see my comments below.
> >
> > On 21.08.2024 19:52, Sergey Kaplun wrote:
> >> From: Mike Pall <mike>
> >>

<snipped>

> >> ---
> >>
> >> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1224-fix-jit-cdata-arith
> >> Related issues:
> >> *https://github.com/tarantool/tarantool/issues/10199
> >> *https://github.com/LuaJIT/LuaJIT/issues/1224
> >>
> >>   src/lj_crecord.c                              | 10 ++--
> >>   .../lj-1224-fix-cdata-arith-ptr.test.lua      | 48 +++++++++++++++++++
> >
> > This test does not fail without fix (but repro from the issue does):
> >
> > [0] ~/sources/MRG/tarantool/third_party/luajit $ 
> > ./build/gc64/src/luajit -Ohotloop=1 -e "
> > repeat
> >   r = 1LL + nil
> > until true
> > "
> > LuaJIT ASSERT 
> > /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_ctype.c:185: 
> > lj_ctype_intern: uninitialized cts->L
> > Aborted
> > [0] ~/sources/MRG/tarantool/third_party/luajit $ 
> > ./build/gc64/src/luajit 
> > test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> > TAP version 13
> > 1..2
> >     # cdata arithmetic with nil
> > 1..2
> >     ok - correct recording error with bad cdata arithmetic
> >     ok - correct error message
> >     # cdata arithmetic with nil: end
> > ok - cdata arithmetic with nil
> >     # cdata arithmetic with string
> >     1..2
> >     ok - correct recording error with bad cdata arithmetic
> >     ok - correct error message
> >     # cdata arithmetic with string: end
> > ok - cdata arithmetic with string
> > [0] ~/sources/MRG/tarantool/third_party/luajit $
> >
> 
> With GC64 only (LUAJIT_ENABLE_GC64).

It don't fail with GC64?

Should I add the comment?

> <snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Sergey Kaplun via Tarantool-patches]

On 10.09.24, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the review!
> 
> On 10.09.24, Sergey Bronnikov wrote:
> > Hi, Sergey,
> > 
> > On 09.09.2024 18:37, Sergey Bronnikov via Tarantool-patches wrote:
> > >
> > > Hi, Sergey,
> > >
> > > thanks for the patch! see my comments below.
> > >
> > > On 21.08.2024 19:52, Sergey Kaplun wrote:
> > >> From: Mike Pall <mike>
> > >>
> 
> <snipped>
> 
> > >> ---
> > >>
> > >> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1224-fix-jit-cdata-arith
> > >> Related issues:
> > >> *https://github.com/tarantool/tarantool/issues/10199
> > >> *https://github.com/LuaJIT/LuaJIT/issues/1224
> > >>
> > >>   src/lj_crecord.c                              | 10 ++--
> > >>   .../lj-1224-fix-cdata-arith-ptr.test.lua      | 48 +++++++++++++++++++
> > >
> > > This test does not fail without fix (but repro from the issue does):
> > >
> > > [0] ~/sources/MRG/tarantool/third_party/luajit $ 
> > > ./build/gc64/src/luajit -Ohotloop=1 -e "
> > > repeat
> > >   r = 1LL + nil
> > > until true
> > > "
> > > LuaJIT ASSERT 
> > > /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_ctype.c:185: 
> > > lj_ctype_intern: uninitialized cts->L
> > > Aborted
> > > [0] ~/sources/MRG/tarantool/third_party/luajit $ 
> > > ./build/gc64/src/luajit 
> > > test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> > > TAP version 13
> > > 1..2
> > >     # cdata arithmetic with nil
> > > 1..2
> > >     ok - correct recording error with bad cdata arithmetic
> > >     ok - correct error message
> > >     # cdata arithmetic with nil: end
> > > ok - cdata arithmetic with nil
> > >     # cdata arithmetic with string
> > >     1..2
> > >     ok - correct recording error with bad cdata arithmetic
> > >     ok - correct error message
> > >     # cdata arithmetic with string: end
> > > ok - cdata arithmetic with string
> > > [0] ~/sources/MRG/tarantool/third_party/luajit $
> > >
> > 
> > With GC64 only (LUAJIT_ENABLE_GC64).
> 
> It don't fail with GC64?
> 
> Should I add the comment?

I've added the following comment to avoid the confusion.
Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
index 9de9586d..dd7e23ec 100644
--- a/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
+++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
@@ -11,6 +11,8 @@ local test = tap.test('lj-1224-fix-cdata-arith-ptr'):skipcond({
 
 test:plan(2)
 
+-- XXX: This test leads to the assertion failure before the patch
+-- only for non-GC64 build.
 local function arith_nil_protected()
   local i = 1
   while i < 3 do
===================================================================

> 
> > <snipped>
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Sergey Bronnikov via Tarantool-patches]

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

Thanks! LGTM

On 12.09.2024 13:16, Sergey Kaplun wrote:
> On 10.09.24, Sergey Kaplun via Tarantool-patches wrote:
>> Hi, Sergey!
>> Thanks for the review!
>>
>> On 10.09.24, Sergey Bronnikov wrote:
>>> Hi, Sergey,
>>>
>>> On 09.09.2024 18:37, Sergey Bronnikov via Tarantool-patches wrote:
>>>> Hi, Sergey,
>>>>
>>>> thanks for the patch! see my comments below.
>>>>
>>>> On 21.08.2024 19:52, Sergey Kaplun wrote:
>>>>> From: Mike Pall <mike>
>>>>>
>> <snipped>
>>
>>>>> ---
>>>>>
>>>>> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1224-fix-jit-cdata-arith
>>>>> Related issues:
>>>>> *https://github.com/tarantool/tarantool/issues/10199
>>>>> *https://github.com/LuaJIT/LuaJIT/issues/1224
>>>>>
>>>>>    src/lj_crecord.c                              | 10 ++--
>>>>>    .../lj-1224-fix-cdata-arith-ptr.test.lua      | 48 +++++++++++++++++++
>>>> This test does not fail without fix (but repro from the issue does):
>>>>
>>>> [0] ~/sources/MRG/tarantool/third_party/luajit $
>>>> ./build/gc64/src/luajit -Ohotloop=1 -e "
>>>> repeat
>>>>    r = 1LL + nil
>>>> until true
>>>> "
>>>> LuaJIT ASSERT
>>>> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_ctype.c:185:
>>>> lj_ctype_intern: uninitialized cts->L
>>>> Aborted
>>>> [0] ~/sources/MRG/tarantool/third_party/luajit $
>>>> ./build/gc64/src/luajit
>>>> test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
>>>> TAP version 13
>>>> 1..2
>>>>      # cdata arithmetic with nil
>>>> 1..2
>>>>      ok - correct recording error with bad cdata arithmetic
>>>>      ok - correct error message
>>>>      # cdata arithmetic with nil: end
>>>> ok - cdata arithmetic with nil
>>>>      # cdata arithmetic with string
>>>>      1..2
>>>>      ok - correct recording error with bad cdata arithmetic
>>>>      ok - correct error message
>>>>      # cdata arithmetic with string: end
>>>> ok - cdata arithmetic with string
>>>> [0] ~/sources/MRG/tarantool/third_party/luajit $
>>>>
>>> With GC64 only (LUAJIT_ENABLE_GC64).
>> It don't fail with GC64?
>>
>> Should I add the comment?
> I've added the following comment to avoid the confusion.
> Branch is force-pushed.
>
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> index 9de9586d..dd7e23ec 100644
> --- a/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua
> @@ -11,6 +11,8 @@ local test = tap.test('lj-1224-fix-cdata-arith-ptr'):skipcond({
>   
>   test:plan(2)
>   
> +-- XXX: This test leads to the assertion failure before the patch
> +-- only for non-GC64 build.
>   local function arith_nil_protected()
>     local i = 1
>     while i < 3 do
> ===================================================================
>
>>> <snipped>
>> -- 
>> Best regards,
>> Sergey Kaplun

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

Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch and the fixes!
LGTM

Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.

[Sergey Kaplun via Tarantool-patches]

I've applied the patch into all long-term branches in tarantool/luajit
and bumped a new version in master [1], release/3.2 [2] and
release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10712
[2]: https://github.com/tarantool/tarantool/pull/10713
[3]: https://github.com/tarantool/tarantool/pull/10714

-- 
Best regards,
Sergey Kaplun