[Tarantool-patches] [PATCH v2 corrected] Increase limits for debug.traceback

[Tarantool-patches] [PATCH v2 corrected] Increase limits for debug.traceback

[Sergey Voinov]

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

Re: [Tarantool-patches] [PATCH v2 corrected] Increase limits for debug.traceback

[Igor Munkin]

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

Re: [Tarantool-patches] [PATCH v2 corrected] Increase limits for debug.traceback

[Sergey Ostanevich]

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
>