Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CData numbers are compared incorrectly #4089

Closed
Gerold103 opened this issue Mar 29, 2019 · 9 comments
Closed

CData numbers are compared incorrectly #4089

Gerold103 opened this issue Mar 29, 2019 · 9 comments
Assignees
Labels
bug Something isn't working lua wontfix This will not be worked on

Comments

@Gerold103
Copy link
Collaborator

Gerold103 commented Mar 29, 2019

box.cfg{listen = 3313}
netbox = require('net.box')
box.schema.user.grant('guest', 'read,write,execute', 'universe')
c = netbox.connect(box.cfg.listen)
res = {}
for i = 1, 10 do table.insert(res, c:execute('SELECT random()').rows[1]:totable()[1]) end
table.sort(res)
res
tarantool> res
---
- - 1064442337705904389
  - 5430535088656468933
  - 8084626668617668138
  - -9114667304427008884
  - -8060774415037890208
  - -6794628713547132026
  - -3278541528434270828
  - -2746644752953370174
  - -2467935731651350913
  - -1266204910185345866
...

All the values are cdata.

@Gerold103 Gerold103 added bug Something isn't working lua sql labels Mar 29, 2019
@Gerold103
Copy link
Collaborator Author

A test appeared to be much simpler:

8084626668617668138LLU < -9114667304427008884LL
---
- true
...

Just compare unsigned number and a signed negative number.

@Totktonada
Copy link
Member

Totktonada commented Mar 29, 2019

It seems it can be fixed like so (less-then only):

diff --git a/src/lj_carith.c b/src/lj_carith.c
index 218abd2..d32f680 100644
--- a/src/lj_carith.c
+++ b/src/lj_carith.c
@@ -159,22 +159,31 @@ static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
 {
   if (ctype_isnum(ca->ct[0]->info) && ca->ct[0]->size <= 8 &&
       ctype_isnum(ca->ct[1]->info) && ca->ct[1]->size <= 8) {
-    CTypeID id = (((ca->ct[0]->info & CTF_UNSIGNED) && ca->ct[0]->size == 8) ||
-                 ((ca->ct[1]->info & CTF_UNSIGNED) && ca->ct[1]->size == 8)) ?
+    CTypeID id0 = ((ca->ct[0]->info & CTF_UNSIGNED) && ca->ct[0]->size == 8) ?
                 CTID_UINT64 : CTID_INT64;
-    CType *ct = ctype_get(cts, id);
+    CTypeID id1 = ((ca->ct[1]->info & CTF_UNSIGNED) && ca->ct[1]->size == 8) ?
+                CTID_UINT64 : CTID_INT64;
+    CTypeID id = id0 == CTID_UINT64 || id1 == CTID_UINT64 ?
+                CTID_UINT64 : CTID_INT64;
+    CType *ct0 = ctype_get(cts, id0);
+    CType *ct1 = ctype_get(cts, id1);
     GCcdata *cd;
     uint64_t u0, u1, *up;
-    lj_cconv_ct_ct(cts, ct, ca->ct[0], (uint8_t *)&u0, ca->p[0], 0);
+    lj_cconv_ct_ct(cts, ct0, ca->ct[0], (uint8_t *)&u0, ca->p[0], 0);
     if (mm != MM_unm)
-      lj_cconv_ct_ct(cts, ct, ca->ct[1], (uint8_t *)&u1, ca->p[1], 0);
+      lj_cconv_ct_ct(cts, ct1, ca->ct[1], (uint8_t *)&u1, ca->p[1], 0);
     switch (mm) {
     case MM_eq:
       setboolV(L->top-1, (u0 == u1));
       return 1;
     case MM_lt:
-      setboolV(L->top-1,
-              id == CTID_INT64 ? ((int64_t)u0 < (int64_t)u1) : (u0 < u1));
+      if (id0 != id1 && id0 == CTID_INT64 && (int64_t)u0 < 0)
+        setboolV(L->top-1, 1);
+      else if (id0 != id1 && id1 == CTID_INT64 && (int64_t)u1 < 0)
+        setboolV(L->top-1, 0);
+      else
+        setboolV(L->top-1,
+                id == CTID_INT64 ? ((int64_t)u0 < (int64_t)u1) : (u0 < u1));
       return 1;
     case MM_le:
       setboolV(L->top-1,

Not sure about behaviour of jitted code. Not sure whether it is right thing to do.

@Gerold103 Gerold103 removed the sql label Apr 16, 2019
@kyukhin kyukhin added this to the wishlist milestone Jul 18, 2019
@kyukhin kyukhin modified the milestones: wishlist, 2.1.3 Sep 17, 2019
@igormunkin
Copy link
Collaborator

Several LuaJIT implementations seem to be affected with it. I'll take a look at @Totktonada patch and check whether compiler also need to be patched.

@igormunkin
Copy link
Collaborator

igormunkin commented Oct 2, 2019

TL;DR:

Considering the described semantics about 64-bit arithmetics here, the comparison result is fine (see the cite below).

64 bit integer comparison: two cdata numbers, or a cdata number and a Lua number can be compared with each other. If one of them is an uint64_t, the other side is converted to an uint64_t and an unsigned comparison is performed. Otherwise both sides are converted to an int64_t and a signed comparison is performed.


I took another look at the problem and after some time knee deep into FFI internals I can provide the following rationale for such behaviour.

According to the FFI page on LJ wiki: "The FFI library allows <snipped> using C data structures from pure Lua code". Thereby, all operations with such types should be interpreted considering its C specifics. The issue relates only to 64-bit integers which are represented with cdata objects instead of native Lua numbers. All other integer types successfully maps into Lua numeric type (see here for more).

According to 6.3.1.8 and 6.5.8 C99 standard paragraphs (the C standard FFI implementation complies to) whether "the operand that has unsigned integer type has rank greater or equal to the rank of the type of the other operand, then the operand with signed integer type is converted to the type of the operand with unsigned integer type".

Many thanks to @Totktonada for a minimal oneliner with PoC that I modified a bit for the comment:

$ gcc -Wextra -x c <(echo 'int main() { return !(8084626668617668138ULL
< -9114667304427008884LL); }') -o cmp && ./cmp; echo $?
/proc/self/fd/11: In function 'main':
/proc/self/fd/11:1:46: warning: comparison of integer expressions of
different signedness: 'long long unsigned int' and 'long long int'
[-Wsign-compare]
0
$ gcc -Wextra -x c <(echo 'int main() { return !(8084626668617668138LL
< -9114667304427008884LL); }') -o cmp && ./cmp; echo $?
1
$

You can see the first result is the same one provided by LuaJIT. Considering the warning produced by gcc we will obtain the proper comparison result after changing the suffix of the first literal:

tarantool> 8084626668617668138LL < -9114667304427008884LL
---
- false
...

Long story short:

The issue is not related to FFI and I suggest replacing luajit label with the corresponding one. As far as I'm able to judge the root problem is about representing some MP data with native Lua types. In this way I propose to proceed the following work in this direction.

@igormunkin igormunkin removed the luajit label Oct 2, 2019
@kyukhin
Copy link
Contributor

kyukhin commented Oct 4, 2019

This looks like a correct behavior. @igormunkin please prepare update request to documentation with details.

@Totktonada
Copy link
Member

See also #1555.

@igormunkin
Copy link
Collaborator

@kyukhin, created the corresponding issue in tarantool/doc queue.

@kyukhin
Copy link
Contributor

kyukhin commented Oct 24, 2019

Closing as not a bug.

@kyukhin kyukhin closed this as completed Oct 24, 2019
@kyukhin kyukhin removed this from the 2.1.3 milestone Oct 24, 2019
@kyukhin kyukhin added the wontfix This will not be worked on label Oct 24, 2019
@no1seman
Copy link

@olegrok It's not an error, it's documented behaviour: http://www.lua.org/pil/2.3.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lua wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants