[Tarantool-patches] [PATCH luajit 0/2] Fix tonumber("-0") inconsistencies

[Tarantool-patches] [PATCH luajit 0/2] Fix tonumber("-0") inconsistencies

[Sergey Kaplun via Tarantool-patches]

This patchset fixes incorrect behaviour for `tonumber("-0")` different
from Lua 5.1.

The first patch fixes it for non-dualnum mode (default on x86 and x64).
The second fixes it for dual-number mode too.
So, there is the corresponding skipcond in the test for the first patch
that is removed in the next patch.

Related issues/PR:
* https://github.com/LuaJIT/LuaJIT/issues/528
* https://github.com/LuaJIT/LuaJIT/pull/787
* https://github.com/tarantool/tarantool/issues/6548
Branch: https://github.com/tarantool/tarantool/tree/skaplun/lj-528-tonumber-0-full-ci
Tarantool branch: https://github.com/tarantool/luajit/tree/skaplun/lj-528-tonumber-0-full-ci

Mike Pall (2):
  Fix tonumber("-0").
  Fix tonumber("-0") in dual-number mode.

 src/lj_strscan.c                                | 13 ++++++++-----
 test/tarantool-tests/lj-528-tonumber-0.test.lua | 13 +++++++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)
 create mode 100644 test/tarantool-tests/lj-528-tonumber-0.test.lua

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/2] Fix tonumber("-0").

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by bluecheetah001.

(cherry picked from 45a7e5073ce0a59465fef0b80bb08bd4e76b7979)

Common case for scanning a string containing a number is a string with a
decimal number. For this case, a decimal number first casts to int32_t
and later to double (lua_Number) if necessary. Information about sign of
0 is lost during the cast to int32_t. So resulting TValue contains `0.`
instead of `-0.`.

This patch removes cast to int32_t for the case when resulting TValue
contains double value.

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

Part of tarantool/tarantool#6548
---
 src/lj_strscan.c                              |  5 ++---
 .../lj-528-tonumber-0.test.lua                | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-528-tonumber-0.test.lua

diff --git a/src/lj_strscan.c b/src/lj_strscan.c
index 08f41f19..4e4ef6d3 100644
--- a/src/lj_strscan.c
+++ b/src/lj_strscan.c
@@ -495,12 +495,11 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
     /* Fast path for decimal 32 bit integers. */
     if (fmt == STRSCAN_INT && base == 10 &&
 	(dig < 10 || (dig == 10 && *sp <= '2' && x < 0x80000000u+neg))) {
-      int32_t y = neg ? -(int32_t)x : (int32_t)x;
       if ((opt & STRSCAN_OPT_TONUM)) {
-	o->n = (double)y;
+	o->n = neg ? -(double)x : (double)x;
 	return STRSCAN_NUM;
       } else {
-	o->i = y;
+	o->i = neg ? -(int32_t)x : (int32_t)x;
 	return STRSCAN_INT;
       }
     }
diff --git a/test/tarantool-tests/lj-528-tonumber-0.test.lua b/test/tarantool-tests/lj-528-tonumber-0.test.lua
new file mode 100644
index 00000000..03ba2aff
--- /dev/null
+++ b/test/tarantool-tests/lj-528-tonumber-0.test.lua
@@ -0,0 +1,19 @@
+local tap = require('tap')
+
+-- Test disabled for DUALNUM mode default for some arches.
+-- See also https://github.com/LuaJIT/LuaJIT/pull/787.
+require("utils").skipcond(
+  jit.arch ~= "x86" and jit.arch ~= "x64",
+  jit.arch.." in DUALNUM mode is clumsy for now"
+)
+
+-- Test file to demonstrate LuaJIT `tonumber('-0')` incorrect
+-- behaviour.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/528.
+local test = tap.test('lj-528-tonumber-0')
+test:plan(1)
+
+-- As numbers `-0 == 0`, so convert it back to string.
+test:ok(tostring(tonumber('-0')) == '-0', 'correct "-0" string parsing')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix tonumber("-0").

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, with two nits, I've fixed on the branch by
myself.

On 26.01.22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by bluecheetah001.
> 
> (cherry picked from 45a7e5073ce0a59465fef0b80bb08bd4e76b7979)
> 
> Common case for scanning a string containing a number is a string with a
> decimal number. For this case, a decimal number first casts to int32_t
> and later to double (lua_Number) if necessary. Information about sign of
> 0 is lost during the cast to int32_t. So resulting TValue contains `0.`
> instead of `-0.`.
> 
> This patch removes cast to int32_t for the case when resulting TValue
> contains double value.

Minor: I propose the following wording (it looks a bit more clear to me
at least):
| When an integral decimal number is given to `tonumber`, it is casted to
| int32_t at first and later to double (lua_Number) if necessary.
| Information about sign of 0 is lost during the cast to int32_t. So
| resulting TValue contains `0.` instead of `-0.`.
|
| This patch removes cast to int32_t for the cases when the input number
| can't be represented by an integer.

> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ---
>  src/lj_strscan.c                              |  5 ++---
>  .../lj-528-tonumber-0.test.lua                | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-528-tonumber-0.test.lua
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-528-tonumber-0.test.lua b/test/tarantool-tests/lj-528-tonumber-0.test.lua
> new file mode 100644
> index 00000000..03ba2aff
> --- /dev/null
> +++ b/test/tarantool-tests/lj-528-tonumber-0.test.lua
> @@ -0,0 +1,19 @@
> +local tap = require('tap')
> +
> +-- Test disabled for DUALNUM mode default for some arches.
> +-- See also https://github.com/LuaJIT/LuaJIT/pull/787.
> +require("utils").skipcond(
> +  jit.arch ~= "x86" and jit.arch ~= "x64",
> +  jit.arch.." in DUALNUM mode is clumsy for now"
> +)

Minor: Use single quotes instead of the double ones.

> +
> +-- Test file to demonstrate LuaJIT `tonumber('-0')` incorrect
> +-- behaviour.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/528.
> +local test = tap.test('lj-528-tonumber-0')
> +test:plan(1)
> +
> +-- As numbers `-0 == 0`, so convert it back to string.
> +test:ok(tostring(tonumber('-0')) == '-0', 'correct "-0" string parsing')
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix tonumber("-0").

[sergos via Tarantool-patches]

Hi!

Thanks for the patch, LGTM.

Sergos

> On 26 Jan 2022, at 15:19, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> Reported by bluecheetah001.
> 
> (cherry picked from 45a7e5073ce0a59465fef0b80bb08bd4e76b7979)
> 
> Common case for scanning a string containing a number is a string with a
> decimal number. For this case, a decimal number first casts to int32_t
> and later to double (lua_Number) if necessary. Information about sign of
> 0 is lost during the cast to int32_t. So resulting TValue contains `0.`
> instead of `-0.`.
> 
> This patch removes cast to int32_t for the case when resulting TValue
> contains double value.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ---
> src/lj_strscan.c                              |  5 ++---
> .../lj-528-tonumber-0.test.lua                | 19 +++++++++++++++++++
> 2 files changed, 21 insertions(+), 3 deletions(-)
> create mode 100644 test/tarantool-tests/lj-528-tonumber-0.test.lua
> 
> diff --git a/src/lj_strscan.c b/src/lj_strscan.c
> index 08f41f19..4e4ef6d3 100644
> --- a/src/lj_strscan.c
> +++ b/src/lj_strscan.c
> @@ -495,12 +495,11 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
>     /* Fast path for decimal 32 bit integers. */
>     if (fmt == STRSCAN_INT && base == 10 &&
> 	(dig < 10 || (dig == 10 && *sp <= '2' && x < 0x80000000u+neg))) {
> -      int32_t y = neg ? -(int32_t)x : (int32_t)x;
>       if ((opt & STRSCAN_OPT_TONUM)) {
> -	o->n = (double)y;
> +	o->n = neg ? -(double)x : (double)x;
> 	return STRSCAN_NUM;
>       } else {
> -	o->i = y;
> +	o->i = neg ? -(int32_t)x : (int32_t)x;
> 	return STRSCAN_INT;
>       }
>     }
> diff --git a/test/tarantool-tests/lj-528-tonumber-0.test.lua b/test/tarantool-tests/lj-528-tonumber-0.test.lua
> new file mode 100644
> index 00000000..03ba2aff
> --- /dev/null
> +++ b/test/tarantool-tests/lj-528-tonumber-0.test.lua
> @@ -0,0 +1,19 @@
> +local tap = require('tap')
> +
> +-- Test disabled for DUALNUM mode default for some arches.
> +-- See also https://github.com/LuaJIT/LuaJIT/pull/787.
> +require("utils").skipcond(
> +  jit.arch ~= "x86" and jit.arch ~= "x64",
> +  jit.arch.." in DUALNUM mode is clumsy for now"
> +)
> +
> +-- Test file to demonstrate LuaJIT `tonumber('-0')` incorrect
> +-- behaviour.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/528.
> +local test = tap.test('lj-528-tonumber-0')
> +test:plan(1)
> +
> +-- As numbers `-0 == 0`, so convert it back to string.
> +test:ok(tostring(tonumber('-0')) == '-0', 'correct "-0" string parsing')
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

[Tarantool-patches] [PATCH luajit 2/2] Fix tonumber("-0") in dual-number mode.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

For DUALNUM build `tonumber("-0x0")` or `tonumber("-0")` returns 0
instead -0 due to the STRSCAN_OPT_TOINT option of `lj_strscan_scan()`
and cast value to integer with losing information about its sign.

This patch adds special checks for these corner cases during strscan.

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

Part of tarantool/tarantool#6548
---
 src/lj_strscan.c                                |  8 ++++++--
 test/tarantool-tests/lj-528-tonumber-0.test.lua | 10 ++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/src/lj_strscan.c b/src/lj_strscan.c
index 4e4ef6d3..11d341ee 100644
--- a/src/lj_strscan.c
+++ b/src/lj_strscan.c
@@ -121,7 +121,8 @@ static StrScanFmt strscan_hex(const uint8_t *p, TValue *o,
   /* Format-specific handling. */
   switch (fmt) {
   case STRSCAN_INT:
-    if (!(opt & STRSCAN_OPT_TONUM) && x < 0x80000000u+neg) {
+    if (!(opt & STRSCAN_OPT_TONUM) && x < 0x80000000u+neg &&
+	!(x == 0 && neg)) {
       o->i = neg ? -(int32_t)x : (int32_t)x;
       return STRSCAN_INT;  /* Fast path for 32 bit integers. */
     }
@@ -498,6 +499,9 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
       if ((opt & STRSCAN_OPT_TONUM)) {
 	o->n = neg ? -(double)x : (double)x;
 	return STRSCAN_NUM;
+      } else if (x == 0 && neg) {
+	o->n = -0.0;
+	return STRSCAN_NUM;
       } else {
 	o->i = neg ? -(int32_t)x : (int32_t)x;
 	return STRSCAN_INT;
@@ -515,7 +519,7 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
       fmt = strscan_dec(sp, o, fmt, opt, ex, neg, dig);
 
     /* Try to convert number to integer, if requested. */
-    if (fmt == STRSCAN_NUM && (opt & STRSCAN_OPT_TOINT)) {
+    if (fmt == STRSCAN_NUM && (opt & STRSCAN_OPT_TOINT) && !tvismzero(o)) {
       double n = o->n;
       int32_t i = lj_num2int(n);
       if (n == (lua_Number)i) { o->i = i; return STRSCAN_INT; }
diff --git a/test/tarantool-tests/lj-528-tonumber-0.test.lua b/test/tarantool-tests/lj-528-tonumber-0.test.lua
index 03ba2aff..f3ad53d3 100644
--- a/test/tarantool-tests/lj-528-tonumber-0.test.lua
+++ b/test/tarantool-tests/lj-528-tonumber-0.test.lua
@@ -1,15 +1,9 @@
 local tap = require('tap')
 
--- Test disabled for DUALNUM mode default for some arches.
--- See also https://github.com/LuaJIT/LuaJIT/pull/787.
-require("utils").skipcond(
-  jit.arch ~= "x86" and jit.arch ~= "x64",
-  jit.arch.." in DUALNUM mode is clumsy for now"
-)
-
 -- Test file to demonstrate LuaJIT `tonumber('-0')` incorrect
 -- behaviour.
--- See also https://github.com/LuaJIT/LuaJIT/issues/528.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/528,
+-- https://github.com/LuaJIT/LuaJIT/pull/787.
 local test = tap.test('lj-528-tonumber-0')
 test:plan(1)
 
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix tonumber("-0") in dual-number mode.

[Sergey Kaplun via Tarantool-patches]

Hi, again!

On 26.01.22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Kaplun.
> 

Added the corresponding line to the commit:

| (cherry picked from commit 103c29e634d822e0affd7d3ae16a7d8a80c038d3)

Branch is force pushed.

> For DUALNUM build `tonumber("-0x0")` or `tonumber("-0")` returns 0
> instead -0 due to the STRSCAN_OPT_TOINT option of `lj_strscan_scan()`
> and cast value to integer with losing information about its sign.
> 
> This patch adds special checks for these corner cases during strscan.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ---

<snipped>

> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix tonumber("-0") in dual-number mode.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix tonumber("-0") in dual-number mode.

[sergos via Tarantool-patches]

Hi!

Thanks for the patch, jut nits in the changelog.

LGTM

Sergos

> On 26 Jan 2022, at 15:19, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> Reported by Sergey Kaplun.
> 
> For DUALNUM build `tonumber("-0x0")` or `tonumber("-0")` returns 0
    the            a call to             to
> instead -0 due to the STRSCAN_OPT_TOINT option of `lj_strscan_scan()`
        of                                        the
> and cast value to integer with losing information about its sign.
    a    of the
> 
> This patch adds special checks for these corner cases during strscan.
                                                             The
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ---
> src/lj_strscan.c                                |  8 ++++++--
> test/tarantool-tests/lj-528-tonumber-0.test.lua | 10 ++--------
> 2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/src/lj_strscan.c b/src/lj_strscan.c
> index 4e4ef6d3..11d341ee 100644
> --- a/src/lj_strscan.c
> +++ b/src/lj_strscan.c
> @@ -121,7 +121,8 @@ static StrScanFmt strscan_hex(const uint8_t *p, TValue *o,
>   /* Format-specific handling. */
>   switch (fmt) {
>   case STRSCAN_INT:
> -    if (!(opt & STRSCAN_OPT_TONUM) && x < 0x80000000u+neg) {
> +    if (!(opt & STRSCAN_OPT_TONUM) && x < 0x80000000u+neg &&
> +	!(x == 0 && neg)) {
>       o->i = neg ? -(int32_t)x : (int32_t)x;
>       return STRSCAN_INT;  /* Fast path for 32 bit integers. */
>     }
> @@ -498,6 +499,9 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
>       if ((opt & STRSCAN_OPT_TONUM)) {
> 	o->n = neg ? -(double)x : (double)x;
> 	return STRSCAN_NUM;
> +      } else if (x == 0 && neg) {
> +	o->n = -0.0;
> +	return STRSCAN_NUM;
>       } else {
> 	o->i = neg ? -(int32_t)x : (int32_t)x;
> 	return STRSCAN_INT;
> @@ -515,7 +519,7 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
>       fmt = strscan_dec(sp, o, fmt, opt, ex, neg, dig);
> 
>     /* Try to convert number to integer, if requested. */
> -    if (fmt == STRSCAN_NUM && (opt & STRSCAN_OPT_TOINT)) {
> +    if (fmt == STRSCAN_NUM && (opt & STRSCAN_OPT_TOINT) && !tvismzero(o)) {
>       double n = o->n;
>       int32_t i = lj_num2int(n);
>       if (n == (lua_Number)i) { o->i = i; return STRSCAN_INT; }
> diff --git a/test/tarantool-tests/lj-528-tonumber-0.test.lua b/test/tarantool-tests/lj-528-tonumber-0.test.lua
> index 03ba2aff..f3ad53d3 100644
> --- a/test/tarantool-tests/lj-528-tonumber-0.test.lua
> +++ b/test/tarantool-tests/lj-528-tonumber-0.test.lua
> @@ -1,15 +1,9 @@
> local tap = require('tap')
> 
> --- Test disabled for DUALNUM mode default for some arches.
> --- See also https://github.com/LuaJIT/LuaJIT/pull/787.
> -require("utils").skipcond(
> -  jit.arch ~= "x86" and jit.arch ~= "x64",
> -  jit.arch.." in DUALNUM mode is clumsy for now"
> -)
> -
> -- Test file to demonstrate LuaJIT `tonumber('-0')` incorrect
> -- behaviour.
> --- See also https://github.com/LuaJIT/LuaJIT/issues/528.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/528,
> +-- https://github.com/LuaJIT/LuaJIT/pull/787.
> local test = tap.test('lj-528-tonumber-0')
> test:plan(1)
> 
> -- 
> 2.34.1
> 

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix tonumber("-0") inconsistencies

[Igor Munkin via Tarantool-patches]

Sergey,

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

On 26.01.22, Sergey Kaplun wrote:
> This patchset fixes incorrect behaviour for `tonumber("-0")` different
> from Lua 5.1.
> 
> The first patch fixes it for non-dualnum mode (default on x86 and x64).
> The second fixes it for dual-number mode too.
> So, there is the corresponding skipcond in the test for the first patch
> that is removed in the next patch.
> 
> Related issues/PR:
> * https://github.com/LuaJIT/LuaJIT/issues/528
> * https://github.com/LuaJIT/LuaJIT/pull/787
> * https://github.com/tarantool/tarantool/issues/6548
> Branch: https://github.com/tarantool/tarantool/tree/skaplun/lj-528-tonumber-0-full-ci
> Tarantool branch: https://github.com/tarantool/luajit/tree/skaplun/lj-528-tonumber-0-full-ci
> 
> Mike Pall (2):
>   Fix tonumber("-0").
>   Fix tonumber("-0") in dual-number mode.
> 
>  src/lj_strscan.c                                | 13 ++++++++-----
>  test/tarantool-tests/lj-528-tonumber-0.test.lua | 13 +++++++++++++
>  2 files changed, 21 insertions(+), 5 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-528-tonumber-0.test.lua
> 
> -- 
> 2.34.1
> 

-- 
Best regards,
IM