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

[sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- 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 --]