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 75DF546970E for ; Sat, 25 Jan 2020 22:04:32 +0300 (MSK) Date: Sat, 25 Jan 2020 22:02:15 +0300 From: Igor Munkin Message-ID: <20200125190215.GI26983@tarantool.org> References: <20200122143613.15590-1-sergeiv@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline 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 Sergey, Thanks, the patch is nice, except the changes around LUA_IDSIZE_ERR I was going to reply (but you've sent the second version in a jiffy). Furthermore I left a few polishing-aimed comments below, please consider them too. On 22.01.20, 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. AFAICS the change just introduce the compile-time constants to be tuned for Tarantool built. The defaults are left the same, so please adjust the commit message. > > There are also changes in main Tarantool trunk in > cmake/luajit.cmake (see corresponding commit for it). You can move this remark to the changelog below, since the patch can be applied w/o the corresponding changes in Tarantool repo. > > Closes: #4581 Please, don't use ':' in gh tags. It just doesn't respect our commit message style. > --- > 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 Minor: I guess the wrapped LUA_IDSIZE can be placed alongside with the introduced LUA_TRACEBAKLEVELS{1,2} for better constants locality. > +#define LUA_IDSIZE_ERR 60 /* Size of error message */ I moved our corresponding discussion from the previous patch below. |> > 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? |> |> The problem here is that if I allow 128 for size of error messages, all |> error outputs will be longer and there will be need to fix vast amount |> of test result outputs (.result files). Also, these long outputs will |> flood tarantool output. I don't think there is a reason to increase |> error message size. Do you? I expect the following comment doesn't look offensive, but in case it does, I definitely mean nothing abusive. I still do not understand the reason to introduce a separate constant for oneline sources. |> there will be need to fix vast amount of test result outputs (.result |> files). AFAIR *.result' files can be adjusted automatically and committed with the cmake related changes. Since the changes are trivial and atomic I see nothing bad. |> Also, these long outputs will flood tarantool output. As well as long filenames, won't they? |> Do you? Yes, I do. Foremost I see no reason to split the entity (i.e. short source name) into a separate ones considering with respect to its origin (a source file or a oneliner). IMHO, this change just introduces excess complexity with no benefits. Furthermore, there is no static assert and even comment regarding the LUA_IDSIZE_ERR value: it have to be less or equal than LUA_IDSIZE. Since the latter one can be tuned at compile-time there is no guarantee the buffer overflow doesn't occur. Considering all said above I'm strictly against introducing a separate LUA_IDSIZE_ERR constant and urge you to revert the related changes. > /* > ** 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 Nice! > +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 Minor: The third parameter is an excess one here, thus I guess it can be dropped. > + local pattern > + > + if i == 1 then Minor: Minor: A trailing whitespace is left above. > + pattern = TRACEBACK_MSG1 > + elseif i == 2 then Minor: Minor: A trailing whitespace is left above. > + 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 > -- Best regards, IM