[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