From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 61F7546970F for ; Thu, 21 Nov 2019 19:59:02 +0300 (MSK) Date: Thu, 21 Nov 2019 19:56:55 +0300 From: Igor Munkin Message-ID: <20191121165655.GF18878@tarantool.org> References: <20191121135352.1080-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20191121135352.1080-1-arkholga@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3] lua: added pairs/ipairs mm to LUA 5.1 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.tarantool.org Olya, Thanks for the patch. Beside some minor flaws left since my previous review, there is a major bug within VM ports for ARM/ARM64. Please consider my comments below. On 21.11.19, Olga Arkhangelskaia wrote: Please adjust the commit subject regarding our contribution guide[1]: |> Use the imperative mood in the subject line. A properly formed Git |> commit subject line should always be able to complete the following |> sentence: “If applied, this commit will /your subject line here/”. Minor: I see the commit subject as the following one: | build: introduce a build flag for __pairs/__ipairs However, feel free to left it as is. > Although we do not turn on full compatibility with Lua 5.2 we want to take > advantage of metamethods. Patch introduces LUAJIT_ENABLE_PAIRSMM flag. > If the flag is set - metamethods are available, no otherwise. Minor: I see the prior sentence as the following one: | If the new flag is set, __pairs/__ipairs are taken into account as | with enabled LUAJIT_ENABLE_LUA52COMPAT. > --- > Part of #4560 Please, append the label to the commit message by moving it prior to three dashes as I proposed within my previous review. > Branch:https://github.com/tarantool/luajit/tree/OKriw/gh-4560-Enable-LUAJIT_ENABLE_LUA52COMPAT-when-bulding-luajit > > Changes in v2: > - flag is renamed > - now flag is used only for pairsmm > - added test case > > Changes in v3: > - added flag to other vm* > - changed comments and commit msg > > src/Makefile | 3 +++ > src/lib_base.c | 4 ++-- > src/lj_arch.h | 6 +++++ > src/lj_obj.h | 2 +- > src/vm_arm.dasc | 4 ++-- > src/vm_arm64.dasc | 4 ++-- > src/vm_mips.dasc | 4 ++-- > src/vm_mips64.dasc | 4 ++-- > src/vm_ppc.dasc | 4 ++-- > src/vm_x64.dasc | 4 ++-- > src/vm_x86.dasc | 4 ++-- > test/mmpairs_test.lua | 54 +++++++++++++++++++++++++++++++++++++++++++ > 12 files changed, 80 insertions(+), 17 deletions(-) > create mode 100755 test/mmpairs_test.lua > > diff --git a/src/Makefile b/src/Makefile > index aca656d..a1a39d2 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -101,6 +101,9 @@ XCFLAGS= > # Note: this does not provide full compatibility with Lua 5.2 at this time. > #XCFLAGS+= -DLUAJIT_ENABLE_LUA52COMPAT > # > +#Enable pairs/ipairs methametods from Lua 5.2. > +#XCFLAGS+= -DLUAJIT_ENABLE_PAIRSMM > +# > # Disable the JIT compiler, i.e. turn LuaJIT into a pure interpreter. > #XCFLAGS+= -DLUAJIT_DISABLE_JIT > # > diff --git a/src/lib_base.c b/src/lib_base.c > index 3a75787..950a580 100644 > --- a/src/lib_base.c > +++ b/src/lib_base.c > @@ -81,12 +81,12 @@ LJLIB_ASM(next) > return FFH_UNREACHABLE; > } > > -#if LJ_52 || LJ_HASFFI > +#if LJ_PAIRSMM || LJ_HASFFI > static int ffh_pairs(lua_State *L, MMS mm) > { > TValue *o = lj_lib_checkany(L, 1); > cTValue *mo = lj_meta_lookup(L, o, mm); > - if ((LJ_52 || tviscdata(o)) && !tvisnil(mo)) { > + if ((LJ_PAIRSMM || tviscdata(o)) && !tvisnil(mo)) { > L->top = o+1; /* Only keep one argument. */ > copyTV(L, L->base-1-LJ_FR2, mo); /* Replace callable. */ > return FFH_TAILCALL; > diff --git a/src/lj_arch.h b/src/lj_arch.h > index c8d7138..0c5230c 100644 > --- a/src/lj_arch.h > +++ b/src/lj_arch.h > @@ -564,4 +564,10 @@ > #define LJ_52 0 > #endif > > +/* Enabling pairs/ipairs metamethods for Lua 5.1 */ > +#if defined(LUAJIT_ENABLE_PAIRSMM) || defined (LUAJIT_ENABLE_LUA52COMPAT) > +#define LJ_PAIRSMM 1 > +#else > +#define LJ_PAIRSMM 0 > +#endif > #endif > diff --git a/src/lj_obj.h b/src/lj_obj.h > index f368578..4d0693f 100644 > --- a/src/lj_obj.h > +++ b/src/lj_obj.h > @@ -530,7 +530,7 @@ enum { > #define MMDEF_FFI(_) > #endif > > -#if LJ_52 || LJ_HASFFI > +#if LJ_HASFFI || LJ_PAIRSMM Minor: I see no reason for swapping the defines, but saving the original order seems to be a convenient way to s/LJ_52/LJ_PAIRSMM/ here considering the patch context. > #define MMDEF_PAIRS(_) _(pairs) _(ipairs) > #else > #define MMDEF_PAIRS(_) > diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc > index d4cdaf5..bddbd54 100644 > --- a/src/vm_arm.dasc > +++ b/src/vm_arm.dasc > @@ -1133,7 +1133,7 @@ static void build_subroutines(BuildCtx *ctx) > | > |.ffunc_1 pairs > | checktab CARG2, ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | ldr TAB:RB, TAB:CARG1->metatable > #endif > | ldrd CFUNC:CARG34, CFUNC:CARG3->upvalue[0] There are couple instructions below to fallback on fast function handler where metatable handling is implemented. That pad, wrapped in LJ_52 define, also need to be adjusted regarding the introduced flag. This remark is also related to ipairs. Side note: I'm totally not an ARM master and I have no idea why Mike separate metatable handling into 2 parts. I see no data dependency here so it seems to be a platform specific optimization (considering the fact this part differs for vm_arm/vm_mips/vm_x86). I can only cite one of my favourite authors here: | The Creator had a lot of remarkably good ideas when he put the world | together, but making it understandable hadn't been one of them. | -- Terry Pratchett (Novel "Mort") CCed Sergos as an expert in various hardware architectures. > @@ -1183,7 +1183,7 @@ static void build_subroutines(BuildCtx *ctx) > | > |.ffunc_1 ipairs > | checktab CARG2, ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | ldr TAB:RB, TAB:CARG1->metatable > #endif > | ldrd CFUNC:CARG34, CFUNC:CARG3->upvalue[0] > diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc > index 3eaf376..1053fde 100644 > --- a/src/vm_arm64.dasc > +++ b/src/vm_arm64.dasc > @@ -1103,7 +1103,7 @@ static void build_subroutines(BuildCtx *ctx) > | > |.ffunc_1 pairs > | checktp TMP1, CARG1, LJ_TTAB, ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | ldr TAB:CARG2, TAB:TMP1->metatable > #endif > | ldr CFUNC:CARG4, CFUNC:CARG3->upvalue[0] Please consider the remark above for this arch too. > @@ -1146,7 +1146,7 @@ static void build_subroutines(BuildCtx *ctx) > | > |.ffunc_1 ipairs > | checktp TMP1, CARG1, LJ_TTAB, ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | ldr TAB:CARG2, TAB:TMP1->metatable > #endif > | ldr CFUNC:CARG4, CFUNC:CARG3->upvalue[0] > diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc > index 1afd611..81de610 100644 > --- a/src/vm_mips.dasc > +++ b/src/vm_mips.dasc > @@ -1292,7 +1292,7 @@ static void build_subroutines(BuildCtx *ctx) > | li AT, LJ_TTAB > | bne SFARG1HI, AT, ->fff_fallback > |. lw PC, FRAME_PC(BASE) > -#if LJ_52 > +#if LJ_PAIRSMM > | lw TAB:TMP2, TAB:SFARG1LO->metatable > | lw TMP0, CFUNC:RB->upvalue[0].u32.hi > | lw TMP1, CFUNC:RB->upvalue[0].u32.lo > @@ -1358,7 +1358,7 @@ static void build_subroutines(BuildCtx *ctx) > | li AT, LJ_TTAB > | bne SFARG1HI, AT, ->fff_fallback > |. lw PC, FRAME_PC(BASE) > -#if LJ_52 > +#if LJ_PAIRSMM > | lw TAB:TMP2, TAB:SFARG1LO->metatable > | lw TMP0, CFUNC:RB->upvalue[0].u32.hi > | lw TMP1, CFUNC:RB->upvalue[0].u32.lo > diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc > index c06270a..4cdff5e 100644 > --- a/src/vm_mips64.dasc > +++ b/src/vm_mips64.dasc > @@ -1306,7 +1306,7 @@ static void build_subroutines(BuildCtx *ctx) > |.ffunc_1 pairs > | checktp TAB:TMP1, CARG1, -LJ_TTAB, ->fff_fallback > | ld PC, FRAME_PC(BASE) > -#if LJ_52 > +#if LJ_PAIRSMM > | ld TAB:TMP2, TAB:TMP1->metatable > | ld TMP0, CFUNC:RB->upvalue[0] > | bnez TAB:TMP2, ->fff_fallback > @@ -1359,7 +1359,7 @@ static void build_subroutines(BuildCtx *ctx) > |.ffunc_1 ipairs > | checktp TAB:TMP1, CARG1, -LJ_TTAB, ->fff_fallback > | ld PC, FRAME_PC(BASE) > -#if LJ_52 > +#if LJ_PAIRSMM > | ld TAB:TMP2, TAB:TMP1->metatable > | ld CFUNC:TMP0, CFUNC:RB->upvalue[0] > | bnez TAB:TMP2, ->fff_fallback > diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc > index b4260eb..4570c12 100644 > --- a/src/vm_ppc.dasc > +++ b/src/vm_ppc.dasc > @@ -1454,7 +1454,7 @@ static void build_subroutines(BuildCtx *ctx) > | checktab CARG3 > | lwz PC, FRAME_PC(BASE) > | bne ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | lwz TAB:TMP2, TAB:CARG1->metatable > | lfd f0, CFUNC:RB->upvalue[0] > | cmplwi TAB:TMP2, 0 > @@ -1540,7 +1540,7 @@ static void build_subroutines(BuildCtx *ctx) > | checktab CARG3 > | lwz PC, FRAME_PC(BASE) > | bne ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | lwz TAB:TMP2, TAB:CARG1->metatable > | lfd f0, CFUNC:RB->upvalue[0] > | cmplwi TAB:TMP2, 0 > diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc > index 80753e0..2785f68 100644 > --- a/src/vm_x64.dasc > +++ b/src/vm_x64.dasc > @@ -1391,7 +1391,7 @@ static void build_subroutines(BuildCtx *ctx) > | mov TAB:RB, [BASE] > | mov TMPR, TAB:RB > | checktab TAB:RB, ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | cmp aword TAB:RB->metatable, 0; jne ->fff_fallback > #endif > | mov CFUNC:RD, [BASE-16] > @@ -1460,7 +1460,7 @@ static void build_subroutines(BuildCtx *ctx) > | mov TAB:RB, [BASE] > | mov TMPR, TAB:RB > | checktab TAB:RB, ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | cmp aword TAB:RB->metatable, 0; jne ->fff_fallback > #endif > | mov CFUNC:RD, [BASE-16] > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc > index 56bee14..eae07f7 100644 > --- a/src/vm_x86.dasc > +++ b/src/vm_x86.dasc > @@ -1724,7 +1724,7 @@ static void build_subroutines(BuildCtx *ctx) > |.ffunc_1 pairs > | mov TAB:RB, [BASE] > | cmp dword [BASE+4], LJ_TTAB; jne ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | cmp dword TAB:RB->metatable, 0; jne ->fff_fallback > #endif > | mov CFUNC:RB, [BASE-8] > @@ -1791,7 +1791,7 @@ static void build_subroutines(BuildCtx *ctx) > |.ffunc_1 ipairs > | mov TAB:RB, [BASE] > | cmp dword [BASE+4], LJ_TTAB; jne ->fff_fallback > -#if LJ_52 > +#if LJ_PAIRSMM > | cmp dword TAB:RB->metatable, 0; jne ->fff_fallback > #endif > | mov CFUNC:RB, [BASE-8] > diff --git a/test/mmpairs_test.lua b/test/mmpairs_test.lua > new file mode 100755 > index 0000000..380a106 > --- /dev/null > +++ b/test/mmpairs_test.lua > @@ -0,0 +1,54 @@ > +#!/usr/bin/env tarantool > + > +tap = require('tap') > + > +test = tap.test("PAIRSMM-is-set") > +test:plan(2) > + > +--- To make sure that we are using only pairs/ipairs mmethods within Lua5.1 > +--- we use os.execute, because its interface has changed significantly in 5.2 > +--- Currently there is no Lua way to detect whether LJ_PAIRSMM is enabled > +--- or not. Therefore taking into account the fact it's enabled by default > +--- in tarantool built and considering the existing testing pipeline we > +--- can check the following scenario: while we can overload the > +--- pairs/ipairs behaviour via metamethod as designed in Lua 5.2, > +--- os.execute still preserves the Lua 5.1 interface. I see my previous remark, related to this comment, was a little bit confusing. I meant not literally extending but replacing first two lines with more words containing a more verbose rationale. Furthermore, comment lines in Lua are prefixed with two dashes, thus the third one is excess. > + > +local mt = { > + __pairs = function(self) > + local function stateless_iter(tbl, k) > + local v > + k, v = next(tbl, k) > + while k and v > 0 do k, v = next(tbl, k) end > + if v then return k,v end > + end > + return stateless_iter, self, nil > + end, > + __ipairs = function(self) > + local function stateless_iter(tbl, k) > + local v > + k, v = next(tbl, k) > + while k and v < 0 do k, v = next(tbl, k) end > + if v then return k,v end > + end > + return stateless_iter, self, nil > + end > +} > + > +local t = setmetatable({ }, mt) > +t[1] = 10 > +t[2] = 20 > +t[3] = -10 > +t[4] = -20 > + > +local pairs_res = 0 > +local ipairs_res = 0 > +for k, v in pairs(t) do > + pairs_res = v + pairs_res > +end > +for k, v in ipairs(t) do > + ipairs_res = v + ipairs_res > +end > +test:is(pairs_res + ipairs_res, 0) > +os_exec_res = os.execute() > +test:is(os_exec_res, 1) > -- > 2.20.1 (Apple Git-117) > [1]: https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/ -- Best regards, IM