From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 CBDDB4696C3 for ; Wed, 15 Apr 2020 03:53:51 +0300 (MSK) Date: Wed, 15 Apr 2020 03:46:45 +0300 From: Igor Munkin Message-ID: <20200415004645.GA8314@tarantool.org> References: <0f113c2f2f746a26f85575d7904235e8920dc5a3.1585312984.git.imun@tarantool.org> <0376106b-1398-f888-16a8-1498212a2915@tarantool.org> <20200407232855.GC5713@tarantool.org> <2a709b78-4ff1-6627-4751-adcc7b38825d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2a709b78-4ff1-6627-4751-adcc7b38825d@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Vlad, Thanks for your review! On 10.04.20, Vladislav Shpilevoy wrote: > Hi! > > >>> if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR}) > >>> diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua > >>> new file mode 100755 > >>> index 000000000..602af88d4 > >>> --- /dev/null > >>> +++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua > >>> @@ -0,0 +1,30 @@ > >>> +#!/usr/bin/env tarantool > >>> + > >>> +local tap = require('tap') > >>> + > >>> +local test = tap.test('gh-4427-ffi-sandwich') > >>> + > >>> +local cmd = string.gsub( > >>> + 'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d', > >>> + '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich') > >>> + > >>> +local checks = { > >>> + { hotloop = 1, trigger = 1, success = true }, > >>> + { hotloop = 1, trigger = 2, success = false }, > >> > >> Why hotloop is needed, if it is always 1? > > > > I decided to localize instance configuration values and make an explicit > > hotloop parameter. Since the result of the tests depends on hotloop and > > trigger values ratio, it seems more convenient for further maintainence > > to store these params nearby each other. > > After the private talk I know that hotloop is some kind of thing affecting > when to start recording a trace. It is worth leaving a comment on that. > And on what is trigger (about this I still don't know). There are lots of comments in the corresponding C file regarding the trigger parameter. Please let me know, if those mentions aren't enough. > > >>> +} > >>> + > >>> +test:plan(#checks) > >>> + > >>> +for _, ch in pairs(checks) do > >>> + local res > >>> + local proc = io.popen(cmd:format(ch.hotloop, ch.trigger)) > >>> + for s in proc:lines('*l') do res = s end > >> > >> What is '*l'? > > > > Considering the doc[1] this option makes iterator read the > > stream line by line (the options passes to its iterator function > > are exactly the same as read accepts). > > In the [1] I see that lines() does not take any arguments. file:read() > does. file:lines() does not. Even for file:read() '*l' is default and > can be omitted. Agree, removed. > > >>> + assert(res, 'proc:lines failed') > >>> + if ch.success then > >>> + test:is(tonumber(res), ch.hotloop + ch.trigger + 1) > >>> + else > >>> + test:is(res, 'Lua VM re-entrancy is detected while executing the trace') > >>> + end > >>> +end > >>> + > >>> +os.exit(test:check() and 0 or 1) > >> > >> Oh God. So luajit already contains cyclic dependencies. It stores suite.ini > >> file in its test folder, which of course is useless for the luajit alone. > >> > >> Since suite.ini file is here, why do you need to run tests from there in such > >> a complex way? Test-run should already be able to peek tests from there (and > >> it does). It probably does not work for your test only because you put it into > >> a folder. I propose you to flatten it, like it is done for function1.test.lua. > >> You still can keep issue name in form of 'gh-####-' in file names to keep them > >> related. They would be > >> > >> gh-4427-ffi-sandwich.test.lua > >> gh-4427-ffi-sandwich.c > >> > >> And after build: > >> > >> + gh-4427-ffi.sandwich.so/.dylib > >> > >> Then you don't a need separate file in app-tap. > > > > A separate file in app-tap is a runner for the chunk in luajit-tap > > tests. Since I need to test a platform failure, I don't know other way > > than run one Lua script via io.popen from another one. > > > > So if my math is OK we still have two Lua chunks and a C one: > > * the Lua runner with io.popen and output analyzer > > * the Lua script to be run by Tarantool instance > > * the Dynamic library to be required by the latter Lua script and to be > > build from the corresponding C file > > > > Could you please clarify your proposal a bit if I'm wrong or missing > > something? > > The problem is that you have one test located in 2 folders and 3 files > now. I propose to make it just 3 files. We never do any test hierarchies. > Also test-run does not support nested folders with tests. > > Strictly speaking, you can make it 2 files: C and Lua. In Lua file you > do io.popen on self with an argument, which would mean the file was started > not by test-run, and which would launch the 'platform failure test'. > > In that way you will have all test code located in one place (not counting > C file). > > if arg[3] == 'do_test' then > jit.opt, C, print, ... > os.exit() > end > > -- arg[1] - tarantool, arg[2] - this file > io.popen(string.format('%s %s ...', arg[1], arg[2], 'do_test')) > > test:is(...) > ... As we discussed, I reworked the tests and dropped all excess files in app-tap directory. Since the changes also affects luajit repo, here are diffs for both tarantool and luajit repos: ## tarantool/luajit: ================================================================================ diff --git a/test/gh-4427-ffi-sandwich.skipcond b/test/gh-4427-ffi-sandwich.skipcond new file mode 100644 index 0000000..2a2ec4d --- /dev/null +++ b/test/gh-4427-ffi-sandwich.skipcond @@ -0,0 +1,7 @@ +import platform + +# Disabled on FreeBSD due to #4819. +if platform.system() == 'FreeBSD': + self.skip = 1 + +# vim: set ft=python: diff --git a/test/gh-4427-ffi-sandwich.test.lua b/test/gh-4427-ffi-sandwich.test.lua new file mode 100755 index 0000000..9d5e50f --- /dev/null +++ b/test/gh-4427-ffi-sandwich.test.lua @@ -0,0 +1,49 @@ +#!/usr/bin/env tarantool + +if #arg == 0 then + require('utils').selfrun(arg, { + { + arg = { + 1, -- hotloop (arg[1]) + 1, -- trigger (arg[2]) + }, + res = tostring(3), -- hotloop + trigger + 1 + msg = 'Trace is aborted', + }, + { + arg = { + 1, -- hotloop (arg[1]) + 2, -- trigger (arg[2]) + }, + res = 'Lua VM re-entrancy is detected while executing the trace', + msg = 'Trace is recorded', + }, + }) +end + +local cfg = { + hotloop = arg[1] or 1, + trigger = arg[2] or 1, +} + +local ffi = require('ffi') +local ffisandwich = ffi.load('libsandwich') +ffi.cdef('int increment(struct sandwich *state, int i)') + +-- Save the current coroutine and set the value to trigger +-- call the Lua routine instead of C implementation. +local sandwich = require('libsandwich')(cfg.trigger) + +-- Depending on trigger and hotloop values the following contexts +-- are possible: +-- * if trigger <= hotloop -> trace recording is aborted +-- * if trigger > hotloop -> trace is recorded but execution +-- leads to panic +jit.opt.start("3", string.format("hotloop=%d", cfg.hotloop)) + +local res +for i = 0, cfg.trigger + cfg.hotloop do + res = ffisandwich.increment(sandwich, i) +end +-- Check the resulting value if panic didn't occur earlier. +print(res) diff --git a/test/gh-4427-ffi-sandwich/libsandwich.c b/test/gh-4427-ffi-sandwich/libsandwich.c index 2a24fb2..029758b 100644 --- a/test/gh-4427-ffi-sandwich/libsandwich.c +++ b/test/gh-4427-ffi-sandwich/libsandwich.c @@ -7,12 +7,12 @@ struct sandwich { int trigger; /* Trigger for switching to Lua call */ }; -int ipp(struct sandwich *state, int i) +int increment(struct sandwich *state, int i) { if (i < state->trigger) return i + 1; - /* Sandwich is triggered and Lua ipp function is called */ + /* Sandwich is triggered and Lua increment function is called */ lua_pushnumber(state->L, state->ref); lua_gettable(state->L, LUA_REGISTRYINDEX); lua_pushnumber(state->L, i); @@ -29,9 +29,9 @@ static int init(lua_State *L) luaL_getmetatable(L, STRUCT_SANDWICH_MT); lua_setmetatable(L, -2); - /* Lua ipp function to be called when sandwich is triggered */ + /* Lua increment function to be called when sandwich is triggered */ if (luaL_dostring(L, "return function(i) return i + 1 end")) - luaL_error(L, "failed to translate Lua ipp function"); + luaL_error(L, "failed to translate Lua increment function"); state->ref = luaL_ref(L, LUA_REGISTRYINDEX); state->L = L; @@ -43,7 +43,7 @@ static int fin(lua_State *L) { struct sandwich *state = luaL_checkudata(L, 1, STRUCT_SANDWICH_MT); - /* Release the anchored ipp function */ + /* Release the anchored increment function */ luaL_unref(L, LUA_REGISTRYINDEX, state->ref); return 0; } diff --git a/test/gh-4427-ffi-sandwich/test.lua b/test/gh-4427-ffi-sandwich/test.lua deleted file mode 100644 index 3ccaee5..0000000 --- a/test/gh-4427-ffi-sandwich/test.lua +++ /dev/null @@ -1,26 +0,0 @@ -local cfg = { - hotloop = arg[1] or 1, - trigger = arg[2] or 1, -} - -local ffi = require('ffi') -local ffisandwich = ffi.load('libsandwich') -ffi.cdef('int ipp(struct sandwich *state, int i)') - --- Save the current coroutine and set the value to trigger ipp --- call the Lua routine instead of pure C implementation. -local sandwich = require('libsandwich')(cfg.trigger) - --- Depending on trigger and hotloop values the following contexts --- are possible: --- * if trigger <= hotloop -> trace recording is aborted --- * if trigger > hotloop -> trace is recorded but execution --- leads to panic -jit.opt.start("3", string.format("hotloop=%d", cfg.hotloop)) - -local res -for i = 0, cfg.trigger + cfg.hotloop do - res = ffisandwich.ipp(sandwich, i) -end --- Check the resulting value if panic didn't occur earlier. -print(res) ================================================================================ ## tarantool/tarantool: ================================================================================ diff --git a/test/app-tap/gh-4427-ffi-sandwich.skipcond b/test/app-tap/gh-4427-ffi-sandwich.skipcond deleted file mode 100644 index 2a2ec4d97..000000000 --- a/test/app-tap/gh-4427-ffi-sandwich.skipcond +++ /dev/null @@ -1,7 +0,0 @@ -import platform - -# Disabled on FreeBSD due to #4819. -if platform.system() == 'FreeBSD': - self.skip = 1 - -# vim: set ft=python: diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua deleted file mode 100755 index 829c129a0..000000000 --- a/test/app-tap/gh-4427-ffi-sandwich.test.lua +++ /dev/null @@ -1,34 +0,0 @@ -#!/usr/bin/env tarantool - -local tap = require('tap') - -local test = tap.test('gh-4427-ffi-sandwich') - -local vars = { - PATH = os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich', - SUFFIX = package.cpath:match('?.(%a+);'), -} - -local cmd = string.gsub('LUA_CPATH=/?. LD_LIBRARY_PATH= ' - .. 'tarantool 2>&1 /test.lua %d %d', '%<(%w+)>', vars) - -local checks = { - { hotloop = 1, trigger = 1, success = true }, - { hotloop = 1, trigger = 2, success = false }, -} - -test:plan(#checks) - -for _, ch in pairs(checks) do - local res - local proc = io.popen(cmd:format(ch.hotloop, ch.trigger)) - for s in proc:lines('*l') do res = s end - assert(res, 'proc:lines failed') - if ch.success then - test:is(tonumber(res), ch.hotloop + ch.trigger + 1) - else - test:is(res, 'Lua VM re-entrancy is detected while executing the trace') - end -end - -os.exit(test:check() and 0 or 1) ================================================================================ I updated the upstream branches and also sent the v2 for both series: * tarantool/luajit[1]. * tarantool/tarantool[2]. > > > [1]: https://www.lua.org/manual/5.1/manual.html#pdf-file:read [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015935.html [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015939.html -- Best regards, IM