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

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

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