[Tarantool-patches] [PATCH luajit] Add stricter check for print() vs. tostring() shortcut.

[Tarantool-patches] [PATCH luajit] Add stricter check for print() vs. tostring() shortcut.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry picked from 46a1b268eb0534182eda0447303c344a071632fe)

`print()` function uses tostring to convert all incoming values to the
strings. So, if element to convert has `__tostring` metamethod it must
be used. In LuaJIT `print()` base library function has a shortcut to
detect fast path for strings and numbers. This shortcut doesn't check
metatable presence for numbers, so `__tostring` metamethod is ignored.

This patch adds the corresponding check for metatable on base number
type, when the aforementioned shortcut is set.

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

Part of tarantool/tarantool#6548
---

Issue: https://github.com/tarantool/tarantool/issues/6548
Related issue: https://github.com/tarantool/tarantool/issues/6746

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-print-tostring-number-mt-full-ci
Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-print-tostring-number-mt-full-ci

CI is red due to integration tests failes (same as on master) or due to
connection errors.

 src/lib_base.c                                |  3 +-
 .../print-tostring-number.test.lua            | 45 +++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/print-tostring-number.test.lua

diff --git a/src/lib_base.c b/src/lib_base.c
index 3a757870..d58de10b 100644
--- a/src/lib_base.c
+++ b/src/lib_base.c
@@ -493,7 +493,8 @@ LJLIB_CF(print)
     lua_gettable(L, LUA_GLOBALSINDEX);
     tv = L->top-1;
   }
-  shortcut = (tvisfunc(tv) && funcV(tv)->c.ffid == FF_tostring);
+  shortcut = (tvisfunc(tv) && funcV(tv)->c.ffid == FF_tostring)
+              && !gcrefu(basemt_it(G(L), LJ_TNUMX));
   for (i = 0; i < nargs; i++) {
     cTValue *o = &L->base[i];
     const char *str;
diff --git a/test/tarantool-tests/print-tostring-number.test.lua b/test/tarantool-tests/print-tostring-number.test.lua
new file mode 100644
index 00000000..862bd0f9
--- /dev/null
+++ b/test/tarantool-tests/print-tostring-number.test.lua
@@ -0,0 +1,45 @@
+local tap = require('tap')
+
+local test = tap.test('print-tostring-number')
+
+-- First field is type, second -- content of cmd.
+local test_data = {
+  {'nil', 'nil'},
+  {'boolean', 'true'},
+  {'userdata', 'newproxy()'},
+  {'number', '42'},
+  -- FIXME: This test case is disabled, because __tostring
+  -- metamethod isn't checked for string base metatable.
+  -- See also https://github.com/tarantool/tarantool/issues/6746.
+  -- {'string', '"teststr"'},
+  {'table', '{}'},
+  {'function', 'function() end'},
+  {'thread', 'coroutine.create(function() end)'},
+}
+
+local NTEST = #test_data
+test:plan(NTEST)
+
+local i = 0
+while arg[i] do i = i - 1 end
+local luabin = arg[i + 1]
+
+for j = 1, NTEST do
+  local prefix = '__tostring reloaded for '
+  local datatype = test_data[j][1]
+  local expected = prefix .. datatype
+
+  local cmd = luabin .. ([[ -e '
+    local testvar = %s
+    debug.setmetatable(testvar, {__tostring = function(a)
+      return "%s" .. type(a)
+    end})
+    print(testvar)
+  ']]):format(test_data[j][2], prefix)
+
+  local proc = io.popen(cmd)
+  local res = proc:read('*all'):gsub('%s+$', '')
+  test:ok(res == expected, expected)
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit] Add stricter check for print() vs. tostring() shortcut.

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!
Just a very minor updates to the message, LGTM.

Sergos


> On 27 Dec 2021, at 13:25, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> (cherry picked from 46a1b268eb0534182eda0447303c344a071632fe)
> 
> `print()` function uses tostring to convert all incoming values to the
                                 ()                                 ?^^^

> strings. So, if element to convert has `__tostring` metamethod it must
                 an                      a 

> be used. In LuaJIT `print()` base library function has a shortcut to
                   the

> detect fast path for strings and numbers. This shortcut doesn't check
> metatable presence for numbers, so `__tostring` metamethod is ignored.
a                                  the

> 
> This patch adds the corresponding check for metatable on base number
                                            ^          ^  ^
                                          the   presence  the

> type, when the aforementioned shortcut is set.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/6548
> Related issue: https://github.com/tarantool/tarantool/issues/6746
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-print-tostring-number-mt-full-ci
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-print-tostring-number-mt-full-ci
> 
> CI is red due to integration tests failes (same as on master) or due to
> connection errors.
> 
> src/lib_base.c                                |  3 +-
> .../print-tostring-number.test.lua            | 45 +++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-tests/print-tostring-number.test.lua
> 
> diff --git a/src/lib_base.c b/src/lib_base.c
> index 3a757870..d58de10b 100644
> --- a/src/lib_base.c
> +++ b/src/lib_base.c
> @@ -493,7 +493,8 @@ LJLIB_CF(print)
>     lua_gettable(L, LUA_GLOBALSINDEX);
>     tv = L->top-1;
>   }
> -  shortcut = (tvisfunc(tv) && funcV(tv)->c.ffid == FF_tostring);
> +  shortcut = (tvisfunc(tv) && funcV(tv)->c.ffid == FF_tostring)
> +              && !gcrefu(basemt_it(G(L), LJ_TNUMX));
>   for (i = 0; i < nargs; i++) {
>     cTValue *o = &L->base[i];
>     const char *str;
> diff --git a/test/tarantool-tests/print-tostring-number.test.lua b/test/tarantool-tests/print-tostring-number.test.lua
> new file mode 100644
> index 00000000..862bd0f9
> --- /dev/null
> +++ b/test/tarantool-tests/print-tostring-number.test.lua
> @@ -0,0 +1,45 @@
> +local tap = require('tap')
> +
> +local test = tap.test('print-tostring-number')
> +
> +-- First field is type, second -- content of cmd.
> +local test_data = {
> +  {'nil', 'nil'},
> +  {'boolean', 'true'},
> +  {'userdata', 'newproxy()'},
> +  {'number', '42'},
> +  -- FIXME: This test case is disabled, because __tostring
> +  -- metamethod isn't checked for string base metatable.
> +  -- See also https://github.com/tarantool/tarantool/issues/6746.
> +  -- {'string', '"teststr"'},
> +  {'table', '{}'},
> +  {'function', 'function() end'},
> +  {'thread', 'coroutine.create(function() end)'},
> +}
> +
> +local NTEST = #test_data
> +test:plan(NTEST)
> +
> +local i = 0
> +while arg[i] do i = i - 1 end
> +local luabin = arg[i + 1]
> +
> +for j = 1, NTEST do
> +  local prefix = '__tostring reloaded for '
> +  local datatype = test_data[j][1]
> +  local expected = prefix .. datatype
> +
> +  local cmd = luabin .. ([[ -e '
> +    local testvar = %s
> +    debug.setmetatable(testvar, {__tostring = function(a)
> +      return "%s" .. type(a)
> +    end})
> +    print(testvar)
> +  ']]):format(test_data[j][2], prefix)
> +
> +  local proc = io.popen(cmd)
> +  local res = proc:read('*all'):gsub('%s+$', '')
> +  test:ok(res == expected, expected)
> +end
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

Re: [Tarantool-patches] [PATCH luajit] Add stricter check for print() vs. tostring() shortcut.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 06.06.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> Just a very minor updates to the message, LGTM.

Fixed. Branch is force-pushed.

> 
> Sergos
> 
> 

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Add stricter check for print() vs. tostring() shortcut.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! In addition to Sergos' nits regarding the commit
message, I have two regarding the test:
  1. Why didn't you use <utils.selfrun> helper for your purposes? It
     fits better than the pack of hacks implementing quite a similar
     routine.
  2. Test name should start with the corresponding issue number from
     LuaJIT queue, AFAIR.

I fixed both comments within fixup commit on top of your branch[1].
Otherwise the patch LGTM, so if you're OK with the changes, I'll proceed
with it.

Last, but not least: I see no ChangeLog entry in the patch. Could you
please drop a few words regarding the change in this thread?

[1]: https://github.com/tarantool/luajit/commit/da9f089

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Add stricter check for print() vs. tostring() shortcut.

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

On 27.12.21, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> (cherry picked from 46a1b268eb0534182eda0447303c344a071632fe)
> 
> `print()` function uses tostring to convert all incoming values to the
> strings. So, if element to convert has `__tostring` metamethod it must
> be used. In LuaJIT `print()` base library function has a shortcut to
> detect fast path for strings and numbers. This shortcut doesn't check
> metatable presence for numbers, so `__tostring` metamethod is ignored.
> 
> This patch adds the corresponding check for metatable on base number
> type, when the aforementioned shortcut is set.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/6548
> Related issue: https://github.com/tarantool/tarantool/issues/6746
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-print-tostring-number-mt-full-ci
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-print-tostring-number-mt-full-ci
> 
> CI is red due to integration tests failes (same as on master) or due to
> connection errors.
> 
>  src/lib_base.c                                |  3 +-
>  .../print-tostring-number.test.lua            | 45 +++++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/print-tostring-number.test.lua
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM