Re: [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

When you will work on the new patchset version, don't forget
to rebase on the latest master. Lots of things changed in errors.

See 7 comments below.

On 24/03/2020 13:45, Leonid Vasiliev wrote:
> Lua bactrace was added to a tarantool error.
> 
> In accordance with https://github.com/tarantool/tarantool/issues/4398
> we want to have a Lua backtrace for the box.error
> 
> @TarantoolBot document
> Title: error.bt
> 
> Lua backtrace was added to a tarantool error.
> 
> Needed for #4398
> ---
> diff --git a/src/lua/error.c b/src/lua/error.c
> index d82e78d..3a12e20 100644
> --- a/src/lua/error.c
> +++ b/src/lua/error.c
> @@ -34,8 +34,44 @@
>  #include <fiber.h>
>  #include "utils.h"
>  
> +#include <string.h>
> +
>  static int CTID_CONST_STRUCT_ERROR_REF = 0;
>  
> +/*
> + * Memory for the traceback string is obtained with malloc,
> + * and can be freed with free.
> +*/
> +static char*
> +traceback (lua_State *L) {

1. Please, rename to lua_traceback(), remove whitespace between
function name and arguments, add 'struct' before lua_State,
add whitespace after 'char', add second '*' in the begining of
the comment, because we always use /** for comments not inside of
a function body.

Also the comment is not really useful. It does not say which trace
it collects - Lua, C, both? Does it resolve symbols in case of C,
or pushes raw addresses?

> +	int top = lua_gettop(L);
> +
> +	lua_getfield(L, LUA_GLOBALSINDEX, "debug");
> +	if (!lua_istable(L, -1)) {
> +		lua_settop(L, top);
> +		return NULL;
> +	}
> +	lua_getfield(L, -1, "traceback");
> +	if (!lua_isfunction(L, -1)) {
> +		lua_settop(L, top);
> +		return NULL;
> +	}
> +
> +	// call debug.traceback

2. Sorry, omit such comments please. They are really useless.
It is like to say:

    // Condition is true if it is true.
    if (true) {
        // Condition was true.
        ...

Also please ask Igor if it is possible to get Lua traceback
from C without accessing LUA_GLOBALSINDEX, 'debug.traceback'.

> +	lua_call(L, 0, 1);
> +
> +	// get result of the debug.traceback call
> +	if (!lua_isstring(L, -1)) {
> +		lua_settop(L, top);
> +		return NULL;
> +	}
> +
> +	char *bt = strdup(lua_tostring(L, -1));
> +	lua_settop(L, top);
> +
> +	return bt;
> +}
> +
>  struct error *
>  luaL_iserror(struct lua_State *L, int narg)
>  {
> @@ -85,6 +121,13 @@ luaT_pusherror(struct lua_State *L, struct error *e)
>  	 * then set the finalizer.
>  	 */
>  	error_ref(e);
> +
> +	if (e->lua_bt == NULL) {
> +		char *lua_bt = traceback(L);
> +		error_set_lua_bt(e, lua_bt);

3. This is double copying. traceback() uses strdup(),
error_set_lua_bt() copies onto a realloced string again.
You could pass traceback result directly to error_set_lua_bt().

> +		free(lua_bt);
> +	}
> +
>  	assert(CTID_CONST_STRUCT_ERROR_REF != 0);
>  	struct error **ptr = (struct error **)
>  		luaL_pushcdata(L, CTID_CONST_STRUCT_ERROR_REF);
> diff --git a/src/lua/error.h b/src/lua/error.h
> index 64fa5eb..16cdaf7 100644
> --- a/src/lua/error.h
> +++ b/src/lua/error.h
> @@ -65,6 +65,9 @@ luaT_pusherror(struct lua_State *L, struct error *e);
>  struct error *
>  luaL_iserror(struct lua_State *L, int narg);
>  
> +struct error *
> +luaL_checkerror(struct lua_State *L, int narg);

4. Why? This function is still used only inside lua/error.c.

> +
>  void
>  tarantool_lua_error_init(struct lua_State *L);
>  
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 7f24986..765ce73 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua> @@ -83,10 +84,18 @@ local function error_trace(err)
>          return {}
>      end
>      return {
> -        { file = ffi.string(err._file), line = tonumber(err._line) };
> +        { file = ffi.string(err._file), line = tonumber(err._line) }

5. Please, omit not necessary diff.

>      }
>  end
>  
> +local function error_backtrace(err)
> +    local result = "Backtrace is absent"
> +    if err.lua_bt ~= ffi.nullptr then
> +        result = ffi.string(err.lua_bt)
> +    end
> +    return result
> +end
> +
>  local function error_errno(err)
>      local e = err._saved_errno
>      if e == 0 then
> @@ -96,10 +105,11 @@ local function error_errno(err)
>  end
>  
>  local error_fields = {
> -    ["type"]        = error_type;
> -    ["message"]     = error_message;
> -    ["trace"]       = error_trace;
> -    ["errno"]       = error_errno;
> +    ["type"]        = error_type,
> +    ["message"]     = error_message,
> +    ["trace"]       = error_trace,
> +    ["errno"]       = error_errno,
> +    ["bt"]          = error_backtrace

6. Please, keep the old fields with ';' to avoid
unnecessary changes. It does not prevent you from
adding backtrace.

Also seems like 'bt' is the only contraction here. Please,
use a full name. I thing traceback would be fine, similar
to debug.traceback. Although backtrace also looks good.
The only thing is that you should be consistent. And
currently you use both backtrace and traceback in different
places.

>  }
>  
>  local function error_unpack(err)
> diff --git a/test/app/fiber.result b/test/app/fiber.result
> index 7331f61..3908733 100644
> --- a/test/app/fiber.result
> +++ b/test/app/fiber.result
> @@ -1036,14 +1036,33 @@ st;
>  ---
>  - false
>  ...
> -e:unpack();
> ----
> -- type: ClientError
> -  code: 1
> -  message: Illegal parameters, oh my
> -  trace:
> -  - file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
> -    line: 1
> +unpack_res = e:unpack();

7. Please, introduce new tests. Don't change existing. The
same for misc.test.lua.