[Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Since this build flag has been removed as a result of reverting the
tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
cmake file is irrelevant.

Furthermore, considering the breakage faced in #4770 the following tests
are introduced:
* the check whether space __pairs metamethod is set to space.pairs to
  create a Lua Fun iterator that handles __pairs manually underneath.
* the check whether pairs builtin behaviour doesn't change when __pairs
  is set e.g. on space object.

Follow-up #4560
Closes #4770

Signed-off-by: Igor Munkin <imun@tarantool.org>
---

@ChangeLog (need to be added to already released versions):
| __pairs/__ipairs metamethods handling is removed since we faced the
| issues with the backward compatibility between Lua 5.1 and Lua 5.2
| within Tarantool modules as well as other third party code (gh-4770).

Issue: https://github.com/tarantool/tarantool/issues/4770
Branch: https://github.com/tarantool/tarantool/tree/imun/gh-4770-broken-pairs

v1: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014375.html

Changes in v2:
* moved test to a separate file

 cmake/luajit.cmake                            |  1 -
 ...4770-broken-pairs-for-space-objects.result | 52 +++++++++++++++++++
 ...70-broken-pairs-for-space-objects.test.lua | 18 +++++++
 3 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 test/box/gh-4770-broken-pairs-for-space-objects.result
 create mode 100644 test/box/gh-4770-broken-pairs-for-space-objects.test.lua

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 072db8269..10df633d5 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -217,7 +217,6 @@ macro(luajit_build)
         add_definitions(-DLUAJIT_USE_ASAN=1)
         set (luajit_ldflags ${luajit_ldflags} -fsanitize=address)
     endif()
-    add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1)
     add_definitions(-DLUAJIT_SMART_STRINGS=1)
     # Add all COMPILE_DEFINITIONS to xcflags
     get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS)
diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
new file mode 100644
index 000000000..61618453e
--- /dev/null
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
@@ -0,0 +1,52 @@
+-- test-run result file version 2
+env = require('test_run')
+ | ---
+ | ...
+test_run = env.new()
+ | ---
+ | ...
+test_run:cmd("push filter 'table: .*' to 'table: <address>'")
+ | ---
+ | - true
+ | ...
+
+--
+-- gh-4770: Iteration through space with Lua builtin pairs routine
+--
+box.cfg{}
+ | ---
+ | ...
+s = box.schema.create_space('test')
+ | ---
+ | ...
+-- Check whether __pairs is set for the space object, since Lua Fun
+-- handles it manually underneath.
+getmetatable(s).__pairs == s.pairs
+ | ---
+ | - true
+ | ...
+-- Check whether pairs builtin behaviour doesn't change when the
+-- __pairs is set.
+keys = {}
+ | ---
+ | ...
+for k, v in pairs(s) do keys[k] = true end
+ | ---
+ | ...
+keys
+ | ---
+ | - engine: true
+ |   before_replace: true
+ |   field_count: true
+ |   id: true
+ |   on_replace: true
+ |   temporary: true
+ |   index: true
+ |   is_local: true
+ |   enabled: true
+ |   name: true
+ |   ck_constraint: true
+ | ...
+s:drop()
+ | ---
+ | ...
diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
new file mode 100644
index 000000000..15024a803
--- /dev/null
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
@@ -0,0 +1,18 @@
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter 'table: .*' to 'table: <address>'")
+
+--
+-- gh-4770: Iteration through space with Lua builtin pairs routine
+--
+box.cfg{}
+s = box.schema.create_space('test')
+-- Check whether __pairs is set for the space object, since Lua Fun
+-- handles it manually underneath.
+getmetatable(s).__pairs == s.pairs
+-- Check whether pairs builtin behaviour doesn't change when the
+-- __pairs is set.
+keys = {}
+for k, v in pairs(s) do keys[k] = true end
+keys
+s:drop()
-- 
2.25.0

Re: [Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Vlad,

I discussed your comments with Sasha Tu. offline and he totally agreed
with both. Fixed, squashed and force-pushed the branch. Diffs are below.

On 06.03.20, Igor Munkin wrote:
> Since this build flag has been removed as a result of reverting the
> tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
> cmake file is irrelevant.
> 
> Furthermore, considering the breakage faced in #4770 the following tests
> are introduced:
> * the check whether space __pairs metamethod is set to space.pairs to
>   create a Lua Fun iterator that handles __pairs manually underneath.
> * the check whether pairs builtin behaviour doesn't change when __pairs
>   is set e.g. on space object.
> 
> Follow-up #4560
> Closes #4770
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> @ChangeLog (need to be added to already released versions):
> | __pairs/__ipairs metamethods handling is removed since we faced the
> | issues with the backward compatibility between Lua 5.1 and Lua 5.2
> | within Tarantool modules as well as other third party code (gh-4770).
> 
> Issue: https://github.com/tarantool/tarantool/issues/4770
> Branch: https://github.com/tarantool/tarantool/tree/imun/gh-4770-broken-pairs
> 
> v1: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014375.html
> 
> Changes in v2:
> * moved test to a separate file
> 
>  cmake/luajit.cmake                            |  1 -
>  ...4770-broken-pairs-for-space-objects.result | 52 +++++++++++++++++++
>  ...70-broken-pairs-for-space-objects.test.lua | 18 +++++++
>  3 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 test/box/gh-4770-broken-pairs-for-space-objects.result
>  create mode 100644 test/box/gh-4770-broken-pairs-for-space-objects.test.lua
> 
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 072db8269..10df633d5 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -217,7 +217,6 @@ macro(luajit_build)
>          add_definitions(-DLUAJIT_USE_ASAN=1)
>          set (luajit_ldflags ${luajit_ldflags} -fsanitize=address)
>      endif()
> -    add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1)
>      add_definitions(-DLUAJIT_SMART_STRINGS=1)
>      # Add all COMPILE_DEFINITIONS to xcflags
>      get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS)
> diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
> new file mode 100644
> index 000000000..61618453e
> --- /dev/null
> +++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
> @@ -0,0 +1,52 @@
> +-- test-run result file version 2
> +env = require('test_run')
> + | ---
> + | ...
> +test_run = env.new()
> + | ---
> + | ...
> +test_run:cmd("push filter 'table: .*' to 'table: <address>'")
> + | ---
> + | - true
> + | ...
> +
> +--
> +-- gh-4770: Iteration through space with Lua builtin pairs routine
> +--
> +box.cfg{}
> + | ---
> + | ...

================================================================================

diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
index 61618453e..2e31d5fbe 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.result
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
@@ -13,9 +13,6 @@ test_run:cmd("push filter 'table: .*' to 'table: <address>'")
 --
 -- gh-4770: Iteration through space with Lua builtin pairs routine
 --
-box.cfg{}
- | ---
- | ...
 s = box.schema.create_space('test')
  | ---
  | ...

================================================================================

> +s = box.schema.create_space('test')
> + | ---
> + | ...
> +-- Check whether __pairs is set for the space object, since Lua Fun
> +-- handles it manually underneath.
> +getmetatable(s).__pairs == s.pairs
> + | ---
> + | - true
> + | ...
> +-- Check whether pairs builtin behaviour doesn't change when the
> +-- __pairs is set.
> +keys = {}
> + | ---
> + | ...
> +for k, v in pairs(s) do keys[k] = true end
> + | ---
> + | ...
> +keys
> + | ---
> + | - engine: true
> + |   before_replace: true
> + |   field_count: true
> + |   id: true
> + |   on_replace: true
> + |   temporary: true
> + |   index: true
> + |   is_local: true
> + |   enabled: true
> + |   name: true
> + |   ck_constraint: true
> + | ...

================================================================================

diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
index 2e31d5fbe..3ac5995c2 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.result
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
@@ -30,19 +30,11 @@ keys = {}
 for k, v in pairs(s) do keys[k] = true end
  | ---
  | ...
-keys
- | ---
- | - engine: true
- |   before_replace: true
- |   field_count: true
- |   id: true
- |   on_replace: true
- |   temporary: true
- |   index: true
- |   is_local: true
- |   enabled: true
- |   name: true
- |   ck_constraint: true
+keys.name, keys.id, keys.engine
+ | ---
+ | - true
+ | - true
+ | - true
  | ...
 s:drop()
  | ---

================================================================================

> +s:drop()
> + | ---
> + | ...
> diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
> new file mode 100644
> index 000000000..15024a803
> --- /dev/null
> +++ b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
> @@ -0,0 +1,18 @@
> +env = require('test_run')
> +test_run = env.new()
> +test_run:cmd("push filter 'table: .*' to 'table: <address>'")
> +
> +--
> +-- gh-4770: Iteration through space with Lua builtin pairs routine
> +--
> +box.cfg{}

================================================================================

diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
index 15024a803..7acf76eb5 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
@@ -5,7 +5,6 @@ test_run:cmd("push filter 'table: .*' to 'table: <address>'")
 --
 -- gh-4770: Iteration through space with Lua builtin pairs routine
 --
-box.cfg{}
 s = box.schema.create_space('test')
 -- Check whether __pairs is set for the space object, since Lua Fun
 -- handles it manually underneath.

================================================================================

> +s = box.schema.create_space('test')
> +-- Check whether __pairs is set for the space object, since Lua Fun
> +-- handles it manually underneath.
> +getmetatable(s).__pairs == s.pairs
> +-- Check whether pairs builtin behaviour doesn't change when the
> +-- __pairs is set.
> +keys = {}
> +for k, v in pairs(s) do keys[k] = true end
> +keys

================================================================================

diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
index 7acf76eb5..315352b15 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
@@ -13,5 +13,5 @@ getmetatable(s).__pairs == s.pairs
 -- __pairs is set.
 keys = {}
 for k, v in pairs(s) do keys[k] = true end
-keys
+keys.name, keys.id, keys.engine
 s:drop()

================================================================================

> +s:drop()
> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> --- /dev/null
> +++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
> @@ -0,0 +1,41 @@
> +-- test-run result file version 2
> +env = require('test_run')
> + | ---
> + | ...
> +test_run = env.new()
> + | ---
> + | ...
> +test_run:cmd("push filter 'table: .*' to 'table: <address>'")

Why do you need the filter? There is nothing to filter out.
You never print any 'table: .*' strings.

Re: [Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Vlad,

On 12.03.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > --- /dev/null
> > +++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
> > @@ -0,0 +1,41 @@
> > +-- test-run result file version 2
> > +env = require('test_run')
> > + | ---
> > + | ...
> > +test_run = env.new()
> > + | ---
> > + | ...
> > +test_run:cmd("push filter 'table: .*' to 'table: <address>'")
> 
> Why do you need the filter? There is nothing to filter out.
> You never print any 'table: .*' strings.
> 

Thanks for your comment! I overlooked this line when copy-pasting the
preambule from box/misc.test.lua and I agree the filter is excess.

Fixed, squashed, force-pushed to the branch. Diff is below:

================================================================================

diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
index 3ac5995c2..c678714ee 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.result
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
@@ -1,14 +1,7 @@
 -- test-run result file version 2
-env = require('test_run')
+test_run = require('test_run').new()
  | ---
  | ...
-test_run = env.new()
- | ---
- | ...
-test_run:cmd("push filter 'table: .*' to 'table: <address>'")
- | ---
- | - true
- | ...
 
 --
 -- gh-4770: Iteration through space with Lua builtin pairs routine
diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
index 315352b15..a1ba1eeab 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
@@ -1,6 +1,4 @@
-env = require('test_run')
-test_run = env.new()
-test_run:cmd("push filter 'table: .*' to 'table: <address>'")
+test_run = require('test_run').new()
 
 --
 -- gh-4770: Iteration through space with Lua builtin pairs routine

================================================================================

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Vladislav Shpilevoy]

> diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
> index 3ac5995c2..c678714ee 100644
> --- a/test/box/gh-4770-broken-pairs-for-space-objects.result
> +++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
> @@ -1,14 +1,7 @@
>  -- test-run result file version 2
> -env = require('test_run')
> +test_run = require('test_run').new()

Seems like you don't need 'test_run' object either.

But ok, LGTM.

Re: [Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Vlad,

Thanks for your review.

On 16.03.20, Vladislav Shpilevoy wrote:
> > diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
> > index 3ac5995c2..c678714ee 100644
> > --- a/test/box/gh-4770-broken-pairs-for-space-objects.result
> > +++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
> > @@ -1,14 +1,7 @@
> >  -- test-run result file version 2
> > -env = require('test_run')
> > +test_run = require('test_run').new()
> 
> Seems like you don't need 'test_run' object either.

Fixed, squashed, force-pushed to the branch. Diff is below:

================================================================================

diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.result b/test/box/gh-4770-broken-pairs-for-space-objects.result
index c678714ee..6ab778d1a 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.result
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.result
@@ -1,8 +1,4 @@
 -- test-run result file version 2
-test_run = require('test_run').new()
- | ---
- | ...
-
 --
 -- gh-4770: Iteration through space with Lua builtin pairs routine
 --
diff --git a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
index a1ba1eeab..8c36bd305 100644
--- a/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
+++ b/test/box/gh-4770-broken-pairs-for-space-objects.test.lua
@@ -1,5 +1,3 @@
-test_run = require('test_run').new()
-
 --
 -- gh-4770: Iteration through space with Lua builtin pairs routine
 --

================================================================================

> 
> But ok, LGTM.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Kirill Yukhin]

Hello,

On 06 мар 19:28, Igor Munkin wrote:
> Since this build flag has been removed as a result of reverting the
> tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
> cmake file is irrelevant.
> 
> Furthermore, considering the breakage faced in #4770 the following tests
> are introduced:
> * the check whether space __pairs metamethod is set to space.pairs to
>   create a Lua Fun iterator that handles __pairs manually underneath.
> * the check whether pairs builtin behaviour doesn't change when __pairs
>   is set e.g. on space object.
> 
> Follow-up #4560
> Closes #4770
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>

LGTM.
Checked into master, 2.3 and 2.2.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2] build: disable LUAJIT_ENABLE_PAIRSMM

[Kirill Yukhin]

On 17 мар 10:30, Kirill Yukhin wrote:
> Hello,
> 
> On 06 мар 19:28, Igor Munkin wrote:
> > Since this build flag has been removed as a result of reverting the
> > tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
> > cmake file is irrelevant.
> > 
> > Furthermore, considering the breakage faced in #4770 the following tests
> > are introduced:
> > * the check whether space __pairs metamethod is set to space.pairs to
> >   create a Lua Fun iterator that handles __pairs manually underneath.
> > * the check whether pairs builtin behaviour doesn't change when __pairs
> >   is set e.g. on space object.
> > 
> > Follow-up #4560
> > Closes #4770
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> 
> LGTM.
> Checked into master, 2.3 and 2.2.

Checked into 1.10 as well.

--
Regards, Kirill Yukhin