From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 4A1CD469719 for ; Fri, 21 Feb 2020 14:33:57 +0300 (MSK) Date: Fri, 21 Feb 2020 14:33:54 +0300 From: Sergey Ostanevich Message-ID: <20200221113354.GD68447@tarantool.org> References: <20200122143613.15590-1-sergeiv@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200122143613.15590-1-sergeiv@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 corrected] Increase limits for debug.traceback List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Voinov Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the patch! I wonder why did you introduce yet another hardcoded limit of LUA_IDSIZE_ERR alongside the moving the LUA_IDSIZE under cmake? Was it a reuse of the original define for a different purpose? Please, add some comment in the message. Otherwise LGTM. Sergos On 22 янв 17:36, Sergey Voinov wrote: > Currently debug.traceback cuts out path prefixes > and removes intermediate frames. This makes it harder > to debug and navigate to files right from terminal. > > This change increases traceback limits to 25 stack frames > and maximum source file name length to 128 characters. > > There are also changes in main Tarantool trunk in > cmake/luajit.cmake (see corresponding commit for it). > > Closes: #4581 > --- > Changes in v2: > - fixed code and test in accordance with review > > src/lj_debug.c | 8 +-- > src/luaconf.h | 14 ++++- > test/tarantool-4581-traceback.test.lua | 79 ++++++++++++++++++++++++++ > 3 files changed, 95 insertions(+), 6 deletions(-) > create mode 100755 test/tarantool-4581-traceback.test.lua > > diff --git a/src/lj_debug.c b/src/lj_debug.c > index 73bd196..e65eb05 100644 > --- a/src/lj_debug.c > +++ b/src/lj_debug.c > @@ -332,11 +332,11 @@ void lj_debug_shortname(char *out, GCstr *str, BCLine line) > strcpy(out, src); > } else { /* Output [string "string"] or [builtin:name]. */ > size_t len; /* Length, up to first control char. */ > - for (len = 0; len < LUA_IDSIZE-12; len++) > + for (len = 0; len < LUA_IDSIZE_ERR-12; len++) > if (((const unsigned char *)src)[len] < ' ') break; > strcpy(out, line == ~(BCLine)0 ? "[builtin:" : "[string \""); out += 9; > if (src[len] != '\0') { /* Must truncate? */ > - if (len > LUA_IDSIZE-15) len = LUA_IDSIZE-15; > + if (len > LUA_IDSIZE_ERR-15) len = LUA_IDSIZE_ERR-15; > strncpy(out, src, len); out += len; > strcpy(out, "..."); out += 3; > } else { > @@ -651,8 +651,8 @@ void lj_debug_dumpstack(lua_State *L, SBuf *sb, const char *fmt, int depth) > #endif > > /* Number of frames for the leading and trailing part of a traceback. */ > -#define TRACEBACK_LEVELS1 12 > -#define TRACEBACK_LEVELS2 10 > +#define TRACEBACK_LEVELS1 LUA_TRACEBACK_LEVELS1 > +#define TRACEBACK_LEVELS2 LUA_TRACEBACK_LEVELS2 > > LUALIB_API void luaL_traceback (lua_State *L, lua_State *L1, const char *msg, > int level) > diff --git a/src/luaconf.h b/src/luaconf.h > index 60cb928..69992b3 100644 > --- a/src/luaconf.h > +++ b/src/luaconf.h > @@ -109,9 +109,19 @@ > #define LUA_MAXINPUT 512 /* Max. input line length. */ > #endif > > +#ifndef LUA_TRACEBACK_LEVELS1 > +# define LUA_TRACEBACK_LEVELS1 12 > +#endif > +#ifndef LUA_TRACEBACK_LEVELS2 > +# define LUA_TRACEBACK_LEVELS2 10 > +#endif > + > /* Note: changing the following defines breaks the Lua 5.1 ABI. */ > -#define LUA_INTEGER ptrdiff_t > -#define LUA_IDSIZE 60 /* Size of lua_Debug.short_src. */ > +#define LUA_INTEGER ptrdiff_t > +#ifndef LUA_IDSIZE > +# define LUA_IDSIZE 128 /* Size of lua_Debug.short_src. */ > +#endif > +#define LUA_IDSIZE_ERR 60 /* Size of error message */ > /* > ** Size of lauxlib and io.* on-stack buffers. Weird workaround to avoid using > ** unreasonable amounts of stack space, but still retain ABI compatibility. > diff --git a/test/tarantool-4581-traceback.test.lua b/test/tarantool-4581-traceback.test.lua > new file mode 100755 > index 0000000..595871b > --- /dev/null > +++ b/test/tarantool-4581-traceback.test.lua > @@ -0,0 +1,79 @@ > +#!/usr/bin/env tarantool > + > +--- > +--- gh-4581: Increase limits for debug.traceback > +--- > + > +local test = require('tap').test('traceback test') > +test:plan(1) > + > +-- Currently there is no Lua way to detect the values of > +-- LUA_TRACEBACK_LEVEL1, LUA_TRACEBACK_LEVEL2, LUA_IDSIZE. Therefore > +-- taking into account the default values configured for Tarantool > +-- build and considering the existing testing pipeline we can hardcode > +-- the corresponding values within this test suite to check the output > +-- produced via debug.backtrace. > +local LUA_TRACEBACK_LEVEL1 = 25 > +local LUA_TRACEBACK_LEVEL2 = 25 > + > +local TRACEBACK_MSG1 = 'recursion limit reached!' > +local TRACEBACK_MSG2 = 'stack traceback:' > +local TRACEBACK_PATTERN = 'tarantool%-4581%-traceback%.test%.lua:[0-9]+: in .*' > + > +-- We use local output variable instead of returning function > +-- value recursively since luajit optimizes end recursion > +-- and in that case we would only get 2 stack frames for traceback > +local output > + > +-- Recursive function which generates stack frames for debug.traceback > +local function frec(rec_limit) > + if rec_limit <= 0 then > + output = debug.traceback(TRACEBACK_MSG1) > + -- Strip path from the output and return it > + output = output:gsub("[^\n]*/", "") > + else > + frec(rec_limit - 1) > + end > +end > + > +-- Call debug.traceback with specified recursion depth > +frec(100) > + > +local test_ok = false > + > +-- Split output into strings > +local strings = {} > +for each in output:gmatch("([^\n]+)") do > + table.insert(strings, each) > +end > + > +-- Total output string count > +local count = 0 > + + 2 -- header > + + (LUA_TRACEBACK_LEVEL1-1) -- first part > + + 1 -- three dots > + + (LUA_TRACEBACK_LEVEL2) -- second part > + > +-- Check output strings > +if count == table.getn(strings) then > + for i = 1,count,1 do > + local pattern > + > + if i == 1 then > + pattern = TRACEBACK_MSG1 > + elseif i == 2 then > + pattern = TRACEBACK_MSG2 > + elseif i ~= LUA_TRACEBACK_LEVEL1 + 2 then > + pattern = TRACEBACK_PATTERN > + else > + pattern = '%.%.%.' > + end > + > + test_ok = string.match(strings[i], pattern) ~= nil > + if not test_ok then > + break > + end > + end > +end > + > +test:is(test_ok, true) > -- > 2.17.1 >