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 A4B0046970E for ; Fri, 20 Dec 2019 15:27:59 +0300 (MSK) Date: Fri, 20 Dec 2019 15:25:48 +0300 From: Igor Munkin Message-ID: <20191220122548.GA31304@tarantool.org> References: <20191213085609.2516-1-sergeiv@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191213085609.2516-1-sergeiv@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] 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 Sergey, Thanks for the patch! I left a couple notes below, please consider them. On 13.12.19, 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. > > Closes: #4581 > --- > issue: https://github.com/tarantool/tarantool/issues/4581 > branch: https://github.com/tarantool/luajit/tree/servoin/gh-4581-debug-limits > src/lj_debug.c | 8 ++-- > src/luaconf.h | 5 ++- > test/tarantool-4581-traceback.result | 55 ++++++++++++++++++++++++++ > test/tarantool-4581-traceback.test.lua | 24 +++++++++++ > 4 files changed, 86 insertions(+), 6 deletions(-) > create mode 100644 test/tarantool-4581-traceback.result > create mode 100755 test/tarantool-4581-traceback.test.lua > > diff --git a/src/lj_debug.c b/src/lj_debug.c > index 73bd196..090ffaa 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 25 > +#define TRACEBACK_LEVELS2 25 I see that more convenient way is managing this limits within tarantool build system, not tuning the LuaJIT defaults. Thereby I propose to add two defines to luaconf.h | #ifndef LUA_TRACEBACK_LEVELS1 | # define LUA_TRACEBACK_LEVELS1 10 | #endif | #ifndef LUA_TRACEBACK_LEVELS2 | # define LUA_TRACEBACK_LEVELS2 25 | #endif and adjust the defines in lj_debug.c as the following. | #define TRACEBACK_LEVELS1 LUA_TRACEBACK_LEVELS1 | #define TRACEBACK_LEVELS2 LUA_TRACEBACK_LEVELS2 Then you just add the corresponding lines below into cmake/luajit.cmake | add_definitions(-DLUA_TRACEBACK_LEVELS1=25) | add_definitions(-DLUA_TRACEBACK_LEVELS2=25) | add_definitions(-DLUA_IDSIZE=128) This enhancement make the further maintenance much easier: * LuaJIT defaults are left unchanged * Tuning these values for Tarantool build becomes simple, neat and doesn't require LuaJIT version bump > > 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..c8fff22 100644 > --- a/src/luaconf.h > +++ b/src/luaconf.h > @@ -110,8 +110,9 @@ > #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 > +#define LUA_IDSIZE 128 /* Size of lua_Debug.short_src. */ > +#define LUA_IDSIZE_ERR 60 /* Size of error message */ I don't get the reason to introduce a separate constant for a single line sources. Could you please provide a bit more extended rationale? Minor: within luaconf.h tabs are used for defaults aligning. Please adjust your changes to the original code style. > /* > ** 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.result b/test/tarantool-4581-traceback.result > new file mode 100644 > index 0000000..018ceec > --- /dev/null > +++ b/test/tarantool-4581-traceback.result > @@ -0,0 +1,55 @@ > +TAP version 13 > +1..1 > +recursion limit reached! > +stack traceback: > +tarantool-4581-traceback.test.lua:13: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > + ... > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:17: in function 'frec' > +tarantool-4581-traceback.test.lua:22: in main chunk > +ok - nil Since all existing tests in luajit suite are TAP-based, I see no reason to introduce a single diff-based test. You can check everything in runtime: * the header you passed to debug.traceback * line with path and 'stack traceback' * 25 lines prior to three dots * three dots * 25 lines following to three dots Furthermore, this test is not related to LuaJIT itself but to its configuration. Yes, we have already one test with the similar nature (pairsmm_tarantool_4560.test.lua) in LuaJIT test suite, and there is a note regarding this fact. I guess it'll be great to drop a few words like those I wrote below: | -- 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. > diff --git a/test/tarantool-4581-traceback.test.lua b/test/tarantool-4581-traceback.test.lua > new file mode 100755 > index 0000000..8003665 > --- /dev/null > +++ b/test/tarantool-4581-traceback.test.lua > @@ -0,0 +1,24 @@ > +#!/usr/bin/env tarantool > + > +--- > +--- gh-4581: Increase limits for debug.traceback > +--- > + > +local test = require('tap').test('traceback test') > +test:plan(1) > + > +-- Recursive function which generates stack frames for debug.traceback > +function frec(rec_limit) > + if rec_limit <= 0 then > + local output = debug.traceback('recursion limit reached!') > + -- Strip path from the output and print it to stdout > + io.write(output:gsub("[^\n]*/", ""), "\n") > + else > + frec(rec_limit - 1) > + end > +end > + > +-- Call debug.traceback with specified recursion depth > +output = frec(100) > + > +test:ok("PASS") > -- > 2.17.1 > -- Best regards, IM