[Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Contributed by Javier Guerra Giraldez.

(cherry picked from commit 02b521981a1ab919ff2cd4d9bcaee80baf77dce2)

When `tonumber()` is recorded (as a part of a trace) for cdata argument
can't be converted to number the `nil` value is recorded as the yielded
result. But without special check on trace for cdata type this nil will
be returned for another type of cdata that can be converted.

This patch adds the corresponding check for recoding of failed cdata
conversions.

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

Resolves tarantool/tarantool#7655
Part of tarantool/tarantool#7230
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-408-tonumber-cdata-record-full-ci
Issues and PRs:
* https://github.com/tarantool/tarantool/issues/7655
* https://github.com/tarantool/tarantool/issues/7230
* https://github.com/LuaJIT/LuaJIT/issues/408
* https://github.com/LuaJIT/LuaJIT/pull/412
Tarantool PR: https://github.com/tarantool/tarantool/pull/7668

 src/lj_crecord.c                              |  2 +
 .../lj-408-tonumber-cdata-record.test.lua     | 44 +++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua

diff --git a/src/lj_crecord.c b/src/lj_crecord.c
index 0d7b71f0..32c767e3 100644
--- a/src/lj_crecord.c
+++ b/src/lj_crecord.c
@@ -1895,6 +1895,8 @@ void LJ_FASTCALL lj_crecord_tonumber(jit_State *J, RecordFFData *rd)
       d = ctype_get(cts, CTID_DOUBLE);
     J->base[0] = crec_ct_tv(J, d, 0, J->base[0], &rd->argv[0]);
   } else {
+    /* Specialize to the ctype that couldn't be converted. */
+    argv2cdata(J, J->base[0], &rd->argv[0]);
     J->base[0] = TREF_NIL;
   }
 }
diff --git a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
new file mode 100644
index 00000000..1c175de1
--- /dev/null
+++ b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
@@ -0,0 +1,44 @@
+local ffi = require('ffi')
+local tap = require('tap')
+
+-- Test file to demonstrate the incorrect JIT recording for
+-- `tonumber()` function with cdata argument for failed
+-- conversions.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/408,
+-- https://github.com/LuaJIT/LuaJIT/pull/412,
+-- https://github.com/LuaJIT/LuaJIT/pull/412,
+-- https://github.com/tarantool/tarantool/issues/7655.
+local test = tap.test('lj-408-tonumber-cdata-record')
+
+local NULL = ffi.cast('void *', 0)
+
+test:plan(4)
+
+local function check(x)
+  -- Don't use a tail call to avoid "leaving loop in root trace"
+  -- error, so the trace will be compiled.
+  local res = tonumber(x)
+  return res
+end
+
+jit.opt.start('hotloop=1')
+-- Record `check()` with `tonumber(NULL)` -- not converted.
+check(NULL)
+check(NULL)
+
+test:ok(not check(NULL), 'recorded with NULL and not converted for NULL')
+test:ok(check(0LL), 'recorded with NULL and converted for 0LL')
+
+-- Reset JIT.
+jit.off()
+jit.flush()
+jit.on()
+
+-- Record `check()` with `tonumber(0LL)` -- converted.
+check(0LL)
+check(0LL)
+
+test:ok(check(0LL), 'recorded with 0LL and converted for 0LL')
+test:ok(not check(NULL), 'recorded with 0LL and not converted for NULL')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM, except for a single nit below:
>
>When `tonumber()` is recorded (as a part of a trace) for cdata argument
>can't be converted to number the `nil` value is recorded as the yielded
>result. But without special check on trace for cdata type this nil will
>be returned for another type of cdata that can be converted.
The first sentence lacks commas and is completely unreadable.
I suggest the following fix:
| When `tonumber()` is recorded (as a part of a trace) for a cdata argument that can't be converted to number, the `nil` value is | recorded as the yielded result.
>
>This patch adds the corresponding check for recoding of failed cdata
>conversions.
Typo: s/recoding/recording
 
<snipped>
--
Best regards,
Maxim Kokryashkin
 

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

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[sergos via Tarantool-patches]

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

Hi!

Thanks for the patch!

Unfortunately, the test doesn’t fail for me, using tarantool test:

/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-695-ffi-vararg-call.test.lua ...................... ok
/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua .......... ok
/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua ................ ok
/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/gh-6189-cur_L.test.lua ............................... ok
/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-418-assert-any-type.test.lua ...................... ok

The sources at the build time are:

void LJ_FASTCALL lj_crecord_tonumber(jit_State *J, RecordFFData *rd)
{
  CTState *cts = ctype_ctsG(J2G(J));
  CType *d, *ct = lj_ctype_rawref(cts, cdataV(&rd->argv[0])->ctypeid);
  if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
  if (ctype_isnum(ct->info) || ctype_iscomplex(ct->info)) {
    if (ctype_isinteger_or_bool(ct->info) && ct->size <= 4 &&
        !(ct->size == 4 && (ct->info & CTF_UNSIGNED)))
      d = ctype_get(cts, CTID_INT32);
    else
      d = ctype_get(cts, CTID_DOUBLE);
    J->base[0] = crec_ct_tv(J, d, 0, J->base[0], &rd->argv[0]);
  } else {
    J->base[0] = TREF_NIL;
  }
}

Means, test passess even without the patch.

Sergos

> On 19 Sep 2022, at 10:50, Maxim Kokryashkin <m.kokryashkin@tarantool.org> wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a single nit below:
> 
> When `tonumber()` is recorded (as a part of a trace) for cdata argument
> can't be converted to number the `nil` value is recorded as the yielded
> result. But without special check on trace for cdata type this nil will
> be returned for another type of cdata that can be converted.
> The first sentence lacks commas and is completely unreadable.
> I suggest the following fix:
> | When `tonumber()` is recorded (as a part of a trace) for a cdata argument that can't be converted to number, the `nil` value is | recorded as the yielded result.
> 
> This patch adds the corresponding check for recoding of failed cdata
> conversions.
> Typo: s/recoding/recording
>  
> <snipped>
> --
> Best regards,
> Maxim Kokryashkin
>  


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

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Are you sure you are doing everything right?
I’ve tested the test case both on M1 and x86 platforms and it works perfectly fine.
--
Best regards,
Maxim Kokryashkin
 
  
>Вторник, 20 сентября 2022, 11:53 +03:00 от sergos <sergos@tarantool.org>:
> 
>Hi!
> 
>Thanks for the patch!
> 
>Unfortunately, the test doesn’t fail for me, using tarantool test:
> 
>/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-695-ffi-vararg-call.test.lua ...................... ok
>/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua .......... ok
>/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua ................ ok
>/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/gh-6189-cur_L.test.lua ............................... ok
>/Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-418-assert-any-type.test.lua ...................... ok
> 
>The sources at the build time are:
> 
>void LJ_FASTCALL lj_crecord_tonumber(jit_State *J, RecordFFData *rd)
>{
>  CTState *cts = ctype_ctsG(J2G(J));
>  CType *d, *ct = lj_ctype_rawref(cts, cdataV(&rd->argv[ 0 ])->ctypeid);
>   if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
>   if (ctype_isnum(ct->info) || ctype_iscomplex(ct->info)) {
>     if (ctype_isinteger_or_bool(ct->info) && ct->size <=  4 &&
>        !(ct->size ==  4 && (ct->info & CTF_UNSIGNED)))
>      d = ctype_get(cts, CTID_INT32);
>     else
>      d = ctype_get(cts, CTID_DOUBLE);
>    J->base[ 0 ] = crec_ct_tv(J, d,  0 , J->base[ 0 ], &rd->argv[ 0 ]);
>  }  else {
>    J->base[ 0 ] = TREF_NIL;
>  }
>}
> 
>Means, test passess even without the patch.
> 
>Sergos
> 
>>On 19 Sep 2022, at 10:50, Maxim Kokryashkin < m.kokryashkin@tarantool.org > wrote:  
>>Hi, Sergey!
>>Thanks for the patch!
>>LGTM, except for a single nit below:
>>>
>>>When `tonumber()` is recorded (as a part of a trace) for cdata argument
>>>can't be converted to number the `nil` value is recorded as the yielded
>>>result. But without special check on trace for cdata type this nil will
>>>be returned for another type of cdata that can be converted.
>>The first sentence lacks commas and is completely unreadable.
>>I suggest the following fix:
>>| When `tonumber()` is recorded (as a part of a trace) for a cdata argument that can't be converted to number, the `nil` value is | recorded as the yielded result.
>>>
>>>This patch adds the corresponding check for recoding of failed cdata
>>>conversions.
>>Typo: s/recoding/recording
>> 
>><snipped>
>>--
>>Best regards,
>>Maxim Kokryashkin
>> 
 

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

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[sergos via Tarantool-patches]

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

First of all:

	s.ostanevich@s-ostanevich2:~/workspaces/t.sergos % uname -a                                                 
	Darwin s-ostanevich2 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10 PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64

Then I just built Tarantool with ninja:

	Tarantool 2.11.0-entrypoint-494-gdc147ec91
	Target: Darwin-x86_64-debug

Then I brought the test as a patch from the mail and run ninja test.

The output says the test passes, while sources contains no patch. Am I missing something?

Sergos

> On 20 Sep 2022, at 12:58, Maxim Kokryashkin <m.kokryashkin@tarantool.org> wrote:
> 
> Hi!
> Are you sure you are doing everything right?
> I’ve tested the test case both on M1 and x86 platforms and it works perfectly fine.
> --
> Best regards,
> Maxim Kokryashkin
>  
>  
> Вторник, 20 сентября 2022, 11:53 +03:00 от sergos <sergos@tarantool.org>:
>  
> Hi!
>  
> Thanks for the patch!
>  
> Unfortunately, the test doesn’t fail for me, using tarantool test:
>  
> /Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-695-ffi-vararg-call.test.lua ...................... ok
> /Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua .......... ok
> /Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua ................ ok
> /Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/gh-6189-cur_L.test.lua ............................... ok
> /Users/s.ostanevich/workspaces/t.sergos/third_party/luajit/test/tarantool-tests/lj-418-assert-any-type.test.lua ...................... ok
>  
> The sources at the build time are:
>  
> void LJ_FASTCALL lj_crecord_tonumber(jit_State *J, RecordFFData *rd)
> {
>   CTState *cts = ctype_ctsG(J2G(J));
>   CType *d, *ct = lj_ctype_rawref(cts, cdataV(&rd->argv[0])->ctypeid);
>   if (ctype_isenum(ct->info)) ct = ctype_child(cts, ct);
>   if (ctype_isnum(ct->info) || ctype_iscomplex(ct->info)) {
>     if (ctype_isinteger_or_bool(ct->info) && ct->size <= 4 &&
>         !(ct->size == 4 && (ct->info & CTF_UNSIGNED)))
>       d = ctype_get(cts, CTID_INT32);
>     else
>       d = ctype_get(cts, CTID_DOUBLE);
>     J->base[0] = crec_ct_tv(J, d, 0, J->base[0], &rd->argv[0]);
>   } else {
>     J->base[0] = TREF_NIL;
>   }
> }
>  
> Means, test passess even without the patch.
>  
> Sergos
>  
>> On 19 Sep 2022, at 10:50, Maxim Kokryashkin <m.kokryashkin@tarantool.org <x-msg://e.mail.ru/compose/?mailto=mailto%3am.kokryashkin@tarantool.org>> wrote:
>>  
>> Hi, Sergey!
>> Thanks for the patch!
>> LGTM, except for a single nit below:
>> 
>> When `tonumber()` is recorded (as a part of a trace) for cdata argument
>> can't be converted to number the `nil` value is recorded as the yielded
>> result. But without special check on trace for cdata type this nil will
>> be returned for another type of cdata that can be converted.
>> The first sentence lacks commas and is completely unreadable.
>> I suggest the following fix:
>> | When `tonumber()` is recorded (as a part of a trace) for a cdata argument that can't be converted to number, the `nil` value is | recorded as the yielded result.
>> 
>> This patch adds the corresponding check for recoding of failed cdata
>> conversions.
>> Typo: s/recoding/recording
>>  
>> <snipped>
>> --
>> Best regards,
>> Maxim Kokryashkin
>>  
> 
>  


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

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 20.09.22, sergos wrote:
> First of all:
> 
> 	s.ostanevich@s-ostanevich2:~/workspaces/t.sergos % uname -a                                                 
> 	Darwin s-ostanevich2 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10 PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
> 
> Then I just built Tarantool with ninja:

I tried run the test suite with ninja, but it's failed:
| $ ninja -C build tarantool-tests
| ...
| /home/burii/builds_workspace/tarantool/master/third_party/luajit/test/tarantool-tests/lj-408.test.lua ...................................... 
| not ok - recorded with NULL and converted for 0LL
| Dubious, test returned 1 (wstat 256, 0x100)
| Failed 1/4 subtests 
|...
| ninja: build stopped: subcommand failed.

> 
> 	Tarantool 2.11.0-entrypoint-494-gdc147ec91
> 	Target: Darwin-x86_64-debug
> 
> Then I brought the test as a patch from the mail and run ninja test.

May you check the test output running without ninja like the follwing,
please:

| $ ../src/tarantool app-tap/lj-408-tonumber-cdata-record.test.lua 
| TAP version 13
| 1..4
| ok - recorded with NULL and not converted for NULL
| not ok - recorded with NULL and converted for 0LL
|   ---
|   filename: app-tap/lj-408-tonumber-cdata-record.test.lua
|   line: 0
|   trace:
|   - line: 0
|     source: '@app-tap/lj-408-tonumber-cdata-record.test.lua'
|     filename: app-tap/lj-408-tonumber-cdata-record.test.lua
|     what: main
|     namewhat: 
|     src: app-tap/lj-408-tonumber-cdata-record.test.lua
|   ...
| ok - recorded with 0LL and converted for 0LL
| ok - recorded with 0LL and not converted for NULL
| # failed subtest: 1

Also, please, check `jit.dump()` output. Maybe there is not trace for
some reason.
You can run the following command:
| $ ../src/tarantool -e 'require"jit.dump".start("ib")' app-tap/lj-408-tonumber-cdata-record.test.lua

The start of output should be the following without the patch.

| 1..4
| ---- TRACE 1 start lj-408-tonumber-cdata-record.test.lua:17
| 0001  GGET     1   0      ; "tonumber"
| 0002  MOV      2   0
| 0003  CALL     1   2   2
| 0000  . FUNCC               ; tonumber
| 0004  RET1     1   2
| ---- TRACE 1 IR
| 0001    fun SLOAD  #0    R
| 0002    tab FLOAD  0001  func.env
| 0003    int FLOAD  0002  tab.hmask
| 0004 >  int EQ     0003  +63 
| 0005    p32 FLOAD  0002  tab.node
| 0006 >  p32 HREFK  0005  "tonumber" @8
| 0007 >  fun HLOAD  0006
| 0008 >  cdt SLOAD  #1    T
| 0009 >  fun EQ     0007  tonumber
| ---- TRACE 1 stop -> return
| ...

> 
> The output says the test passes, while sources contains no patch. Am I missing something?
> 
> Sergos
> 

<snipped>

> >  
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[sergos via Tarantool-patches]

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

Hi!

I see four ‘ok’s as a result of run. 
See the full output for the run with dump() below:


s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % ../../../../build-debug/src/tarantool -e 'require"jit.dump".start("ib")' lj-408-tonumber-cdata-record.test.lua

TAP version 13
1..4
---- TRACE 1 start lj-408-tonumber-cdata-record.test.lua:17
0001  GGET     1   0      ; "tonumber"
0002  MOV      3   0
0003  CALL     1   2   2
0000  . FUNCC               ; tonumber
0004  RET1     1   2
---- TRACE 1 IR
0001    fun SLOAD  #0    R
0002    tab FLOAD  0001  func.env
0003    int FLOAD  0002  tab.hmask
0004 >  int EQ     0003  +63 
0005    p64 FLOAD  0002  tab.node
0006 >  p64 HREFK  0005  "tonumber" @8
0007 >  fun HLOAD  0006
0008 >  cdt SLOAD  #2    T
0009 >  fun EQ     0007  tonumber
---- TRACE 1 stop -> return

ok - recorded with NULL and not converted for NULL
---- TRACE 2 start tap.lua:44
0001  TGETS    4   0   0  ; "total"
0002  ADDVN    4   4   0  ; 1
0003  TSETS    4   0   0  ; "total"
0004  GGET     4   1      ; "io"
0005  TGETS    4   4   2  ; "write"
0006  GGET     6   3      ; "string"
0007  TGETS    6   6   4  ; "rep"
0008  KSTR     8   5      ; " "
0009  TGETS    9   0   6  ; "level"
0010  MULNV    9   9   1  ; 4
0011  CALL     6   0   3
0000  . FUNCC               ; string.rep
0012  CALLM    4   1   0
0000  . FUNCC               ; io.write
0013  ISF          1
0014  JMP      4 => 0025
0015  GGET     4   1      ; "io"
0016  TGETS    4   4   2  ; "write"
0017  GGET     6   3      ; "string"
0018  TGETS    6   6   7  ; "format"
0019  KSTR     8   8      ; "ok - %s\n"
---- TRACE 2 abort tap.lua:48 -- error thrown or hook called during recording

ok - recorded with NULL and converted for 0LL
---- TRACE flush

---- TRACE 1 start lj-408-tonumber-cdata-record.test.lua:17
0001  GGET     1   0      ; "tonumber"
0002  MOV      3   0
0003  CALL     1   2   2
0000  . FUNCC               ; tonumber
0004  RET1     1   2
---- TRACE 1 IR
0001    fun SLOAD  #0    R
0002    tab FLOAD  0001  func.env
0003    int FLOAD  0002  tab.hmask
0004 >  int EQ     0003  +63 
0005    p64 FLOAD  0002  tab.node
0006 >  p64 HREFK  0005  "tonumber" @8
0007 >  fun HLOAD  0006
0008 >  cdt SLOAD  #2    T
0009 >  fun EQ     0007  tonumber
0010    u16 FLOAD  0008  cdata.ctypeid
0011 >  int EQ     0010  +11 
0012    i64 FLOAD  0008  cdata.int64
0013    num CONV   0012  num.i64
---- TRACE 1 stop -> return

ok - recorded with 0LL and converted for 0LL
---- TRACE 2 start tap.lua:44
0001  TGETS    4   0   0  ; "total"
0002  ADDVN    4   4   0  ; 1
0003  TSETS    4   0   0  ; "total"
0004  GGET     4   1      ; "io"
0005  TGETS    4   4   2  ; "write"
0006  GGET     6   3      ; "string"
0007  TGETS    6   6   4  ; "rep"
0008  KSTR     8   5      ; " "
0009  TGETS    9   0   6  ; "level"
0010  MULNV    9   9   1  ; 4
0011  CALL     6   0   3
0000  . FUNCC               ; string.rep
0012  CALLM    4   1   0
0000  . FUNCC               ; io.write
0013  ISF          1
0014  JMP      4 => 0025
0015  GGET     4   1      ; "io"
0016  TGETS    4   4   2  ; "write"
0017  GGET     6   3      ; "string"
0018  TGETS    6   6   7  ; "format"
0019  KSTR     8   8      ; "ok - %s\n"
---- TRACE 2 abort tap.lua:48 -- error thrown or hook called during recording

ok - recorded with 0LL and not converted for NULL
---- TRACE 2 start tarantool.lua:66
0016  UGET     2   1      ; fiber
0017  TGETS    2   2   4  ; "sleep"
0018  MOV      4   1
0019  CALL     2   1   2
0000  . FUNCC               ; C:10b23f5e0
---- TRACE 2 IR
0001    fun SLOAD  #0    R
0002 >  p64 UREFC  0001  #1  
0003 >  tab ULOAD  0002
0004    int FLOAD  0003  tab.hmask
0005 >  int EQ     0004  +31 
0006    p64 FLOAD  0003  tab.node
0007 >  p64 HREFK  0006  "sleep" @4
0008 >  fun HLOAD  0007
0009 >  num SLOAD  #3    T
0010 >  fun EQ     0008  C:10b23f5e0
---- TRACE 2 stop -> stitch

s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % git status
HEAD detached at ae79d993
Untracked files:
  (use "git add <file>..." to include in what will be committed)
	../../patch
	../../src/lj_opt_narrow.c.orig
	../../src/lj_opt_narrow.c.rej
	gh-6976-narrowing-of-unary-minus.test.lua
	lj-408-tonumber-cdata-record.test.lua

nothing added to commit but untracked files present (use "git add" to track)
s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % git diff
s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % 
s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % git -C ../../../.. describe 
2.11.0-entrypoint-494-gdc147ec91



> On 21 Sep 2022, at 15:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 20.09.22, sergos wrote:
>> First of all:
>> 
>> 	s.ostanevich@s-ostanevich2:~/workspaces/t.sergos % uname -a                                                 
>> 	Darwin s-ostanevich2 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10 PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64
>> 
>> Then I just built Tarantool with ninja:
> 
> I tried run the test suite with ninja, but it's failed:
> | $ ninja -C build tarantool-tests
> | ...
> | /home/burii/builds_workspace/tarantool/master/third_party/luajit/test/tarantool-tests/lj-408.test.lua ...................................... 
> | not ok - recorded with NULL and converted for 0LL
> | Dubious, test returned 1 (wstat 256, 0x100)
> | Failed 1/4 subtests 
> |...
> | ninja: build stopped: subcommand failed.
> 
>> 
>> 	Tarantool 2.11.0-entrypoint-494-gdc147ec91
>> 	Target: Darwin-x86_64-debug
>> 
>> Then I brought the test as a patch from the mail and run ninja test.
> 
> May you check the test output running without ninja like the follwing,
> please:
> 
> | $ ../src/tarantool app-tap/lj-408-tonumber-cdata-record.test.lua 
> | TAP version 13
> | 1..4
> | ok - recorded with NULL and not converted for NULL
> | not ok - recorded with NULL and converted for 0LL
> |   ---
> |   filename: app-tap/lj-408-tonumber-cdata-record.test.lua
> |   line: 0
> |   trace:
> |   - line: 0
> |     source: '@app-tap/lj-408-tonumber-cdata-record.test.lua'
> |     filename: app-tap/lj-408-tonumber-cdata-record.test.lua
> |     what: main
> |     namewhat: 
> |     src: app-tap/lj-408-tonumber-cdata-record.test.lua
> |   ...
> | ok - recorded with 0LL and converted for 0LL
> | ok - recorded with 0LL and not converted for NULL
> | # failed subtest: 1
> 
> Also, please, check `jit.dump()` output. Maybe there is not trace for
> some reason.
> You can run the following command:
> | $ ../src/tarantool -e 'require"jit.dump".start("ib")' app-tap/lj-408-tonumber-cdata-record.test.lua
> 
> The start of output should be the following without the patch.
> 
> | 1..4
> | ---- TRACE 1 start lj-408-tonumber-cdata-record.test.lua:17
> | 0001  GGET     1   0      ; "tonumber"
> | 0002  MOV      2   0
> | 0003  CALL     1   2   2
> | 0000  . FUNCC               ; tonumber
> | 0004  RET1     1   2
> | ---- TRACE 1 IR
> | 0001    fun SLOAD  #0    R
> | 0002    tab FLOAD  0001  func.env
> | 0003    int FLOAD  0002  tab.hmask
> | 0004 >  int EQ     0003  +63 
> | 0005    p32 FLOAD  0002  tab.node
> | 0006 >  p32 HREFK  0005  "tonumber" @8
> | 0007 >  fun HLOAD  0006
> | 0008 >  cdt SLOAD  #1    T
> | 0009 >  fun EQ     0007  tonumber
> | ---- TRACE 1 stop -> return
> | ...
> 
>> 
>> The output says the test passes, while sources contains no patch. Am I missing something?
>> 
>> Sergos
>> 
> 
> <snipped>
> 
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun


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

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

On 21.09.22, sergos wrote:
> Hi!
> 
> I see four ‘ok’s as a result of run. 
> See the full output for the run with dump() below:
> 
> 
> s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % ../../../../build-debug/src/tarantool -e 'require"jit.dump".start("ib")' lj-408-tonumber-cdata-record.test.lua
> 
> TAP version 13
> 1..4
> ---- TRACE 1 start lj-408-tonumber-cdata-record.test.lua:17
> 0001  GGET     1   0      ; "tonumber"
> 0002  MOV      3   0
> 0003  CALL     1   2   2
> 0000  . FUNCC               ; tonumber
> 0004  RET1     1   2
> ---- TRACE 1 IR
> 0001    fun SLOAD  #0    R
> 0002    tab FLOAD  0001  func.env
> 0003    int FLOAD  0002  tab.hmask
> 0004 >  int EQ     0003  +63 
> 0005    p64 FLOAD  0002  tab.node
> 0006 >  p64 HREFK  0005  "tonumber" @8
> 0007 >  fun HLOAD  0006
> 0008 >  cdt SLOAD  #2    T
> 0009 >  fun EQ     0007  tonumber
> ---- TRACE 1 stop -> return

Gotcha!
It's a funny thing:
This is happening due to GC64 mode on x86_x64.

There is an invalid type check for SLOAD IR with the following emitted
mcode:

| 55557f6bffc9  mov rdi, [rdx+0x4]
| 55557f6bffcd  sar rdi, 0x2f
| 55557f6bffd1  cmp edi, -0x0b
| 55557f6bffd4  jnz 0x55557f6b0010        ->0

This 0x4 addiction is crucial:
We got the invalid irtype value to compare (due wrong addressing) -- so
the assertion guard is always failed and we always exit from the trace.
This is why we fallback into the interpreter and the test is passed.

In LuaJIT upstream this issue fixed via the following commit:
https://github.com/LuaJIT/LuaJIT/commit/05fbdf56

See also https://github.com/LuaJIT/LuaJIT/pull/350

<snipped>

> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!

Thanks for the review!

On 19.09.22, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a single nit below:
> >
> >When `tonumber()` is recorded (as a part of a trace) for cdata argument
> >can't be converted to number the `nil` value is recorded as the yielded
> >result. But without special check on trace for cdata type this nil will
> >be returned for another type of cdata that can be converted.
> The first sentence lacks commas and is completely unreadable.
> I suggest the following fix:
> | When `tonumber()` is recorded (as a part of a trace) for a cdata argument that can't be converted to number, the `nil` value is
> | recorded as the yielded result.

Yes, this is clearer now :). Thanks!

> >
> >This patch adds the corresponding check for recoding of failed cdata
> >conversions.
> Typo: s/recoding/recording

Fixed, thanks!

The new commit message is the following:

| FFI: Add tonumber() specialization for failed conversions.
|
| Contributed by Javier Guerra Giraldez.
|
| (cherry picked from commit 02b521981a1ab919ff2cd4d9bcaee80baf77dce2)
|
| When `tonumber()` is recorded (as a part of a trace) for a cdata
| argument that can't be converted to a number, the `nil` value is recorded
| as the yielded result. But without special check on trace for cdata type
| this `nil` will be returned for another type of cdata that can be
| converted.
|
| This patch adds the corresponding check for recording of failed cdata
| conversions.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Resolves tarantool/tarantool#7655
| Part of tarantool/tarantool#7230

Branch is force-pushed.

> <snipped>
> --
> Best regards,
> Maxim Kokryashkin
>  

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[sergos via Tarantool-patches]

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

Since we fallback to interpreter it looks safe to keep the test for GC64.
Still you’d better mention it in commit message.

LGTM with above.

Sergos


> On 22 Sep 2022, at 14:28, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> On 21.09.22, sergos wrote:
>> Hi!
>> 
>> I see four ‘ok’s as a result of run. 
>> See the full output for the run with dump() below:
>> 
>> 
>> s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % ../../../../build-debug/src/tarantool -e 'require"jit.dump".start("ib")' lj-408-tonumber-cdata-record.test.lua
>> 
>> TAP version 13
>> 1..4
>> ---- TRACE 1 start lj-408-tonumber-cdata-record.test.lua:17
>> 0001  GGET     1   0      ; "tonumber"
>> 0002  MOV      3   0
>> 0003  CALL     1   2   2
>> 0000  . FUNCC               ; tonumber
>> 0004  RET1     1   2
>> ---- TRACE 1 IR
>> 0001    fun SLOAD  #0    R
>> 0002    tab FLOAD  0001  func.env
>> 0003    int FLOAD  0002  tab.hmask
>> 0004 >  int EQ     0003  +63 
>> 0005    p64 FLOAD  0002  tab.node
>> 0006 >  p64 HREFK  0005  "tonumber" @8
>> 0007 >  fun HLOAD  0006
>> 0008 >  cdt SLOAD  #2    T
>> 0009 >  fun EQ     0007  tonumber
>> ---- TRACE 1 stop -> return
> 
> Gotcha!
> It's a funny thing:
> This is happening due to GC64 mode on x86_x64.
> 
> There is an invalid type check for SLOAD IR with the following emitted
> mcode:
> 
> | 55557f6bffc9  mov rdi, [rdx+0x4]
> | 55557f6bffcd  sar rdi, 0x2f
> | 55557f6bffd1  cmp edi, -0x0b
> | 55557f6bffd4  jnz 0x55557f6b0010        ->0
> 
> This 0x4 addiction is crucial:
> We got the invalid irtype value to compare (due wrong addressing) -- so
> the assertion guard is always failed and we always exit from the trace.
> This is why we fallback into the interpreter and the test is passed.
> 
> In LuaJIT upstream this issue fixed via the following commit:
> https://github.com/LuaJIT/LuaJIT/commit/05fbdf56 <https://github.com/LuaJIT/LuaJIT/commit/05fbdf56>
> 
> See also https://github.com/LuaJIT/LuaJIT/pull/350 <https://github.com/LuaJIT/LuaJIT/pull/350>
> 
> <snipped>
> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun


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

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!


On 26.09.22, sergos wrote:
> Since we fallback to interpreter it looks safe to keep the test for GC64.
> Still you’d better mention it in commit message.

Strictly saying, this problem still exists for GC64 mode if we get the
cdata argument to record via loading from a table, for example.
I didn't modify the recorded function `check()` to keep the test case clear.
I just added the following comment behind.

===================================================================
diff --git a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
index 1c175de1..bf9e8e46 100644
--- a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
+++ b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
@@ -14,6 +14,15 @@ local NULL = ffi.cast('void *', 0)
 
 test:plan(4)
 
+-- This test won't fail for GC64 on x86_64. This happens due to
+-- wrong instruction emitting for SLOAD IR -- we always exit by
+-- the assertion guard on the argument type check. See also
+-- https://github.com/LuaJIT/LuaJIT/pull/350.
+-- The test fails without fix in the current commit, if the
+-- following commit is backported:
+-- https://github.com/LuaJIT/LuaJIT/commit/05fbdf56
+-- Feel free to remove this comment after backporting of the
+-- aforementioned commit.
 local function check(x)
   -- Don't use a tail call to avoid "leaving loop in root trace"
   -- error, so the trace will be compiled.
===================================================================

The branch is force-pushed.

> 
> LGTM with above.
> 
> Sergos
> 
> 
> > On 22 Sep 2022, at 14:28, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > Hi, Sergos!
> > 
> > On 21.09.22, sergos wrote:
> >> Hi!
> >> 
> >> I see four ‘ok’s as a result of run. 
> >> See the full output for the run with dump() below:
> >> 
> >> 
> >> s.ostanevich@s-ostanevich2:~/workspaces/t.sergos/third_party/luajit/test/tarantool-tests % ../../../../build-debug/src/tarantool -e 'require"jit.dump".start("ib")' lj-408-tonumber-cdata-record.test.lua
> >> 
> >> TAP version 13
> >> 1..4
> >> ---- TRACE 1 start lj-408-tonumber-cdata-record.test.lua:17
> >> 0001  GGET     1   0      ; "tonumber"
> >> 0002  MOV      3   0
> >> 0003  CALL     1   2   2
> >> 0000  . FUNCC               ; tonumber
> >> 0004  RET1     1   2
> >> ---- TRACE 1 IR
> >> 0001    fun SLOAD  #0    R
> >> 0002    tab FLOAD  0001  func.env
> >> 0003    int FLOAD  0002  tab.hmask
> >> 0004 >  int EQ     0003  +63 
> >> 0005    p64 FLOAD  0002  tab.node
> >> 0006 >  p64 HREFK  0005  "tonumber" @8
> >> 0007 >  fun HLOAD  0006
> >> 0008 >  cdt SLOAD  #2    T
> >> 0009 >  fun EQ     0007  tonumber
> >> ---- TRACE 1 stop -> return
> > 
> > Gotcha!
> > It's a funny thing:
> > This is happening due to GC64 mode on x86_x64.
> > 
> > There is an invalid type check for SLOAD IR with the following emitted
> > mcode:
> > 
> > | 55557f6bffc9  mov rdi, [rdx+0x4]
> > | 55557f6bffcd  sar rdi, 0x2f
> > | 55557f6bffd1  cmp edi, -0x0b
> > | 55557f6bffd4  jnz 0x55557f6b0010        ->0
> > 
> > This 0x4 addiction is crucial:
> > We got the invalid irtype value to compare (due wrong addressing) -- so
> > the assertion guard is always failed and we always exit from the trace.
> > This is why we fallback into the interpreter and the test is passed.
> > 
> > In LuaJIT upstream this issue fixed via the following commit:
> > https://github.com/LuaJIT/LuaJIT/commit/05fbdf56 <https://github.com/LuaJIT/LuaJIT/commit/05fbdf56>
> > 
> > See also https://github.com/LuaJIT/LuaJIT/pull/350 <https://github.com/LuaJIT/LuaJIT/pull/350>
> > 
> > <snipped>
> > 
> >> 
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Add tonumber() specialization for failed conversions.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

-- 
Best regards,
IM