[Tarantool-patches] [PATCH] Increase limits for debug.traceback
Igor Munkin
imun at tarantool.org
Fri Dec 20 15:25:48 MSK 2019
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
More information about the Tarantool-patches
mailing list