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

[Igor Munkin <imun@tarantool.org>]

Leonid,

Thanks for the patch! It's not a review, I'm just replying the comment I
was mentioned in (Vlad, feel free to CC me next time, I'm totally OK
with it).

On 06.04.20, Vladislav Shpilevoy wrote:
> 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(),

Oh, God, please, no!!! lua_* and luaL_* are Lua standart namespaces,
please stop spoiling them (even if the function is localized within a
translation unit and not exported to public API). Furthermore, it
doesn't respect lua_CFunction signature and lua_* prefix is definitely
a misguiding one here. Let's try to use other letters here for prefix.

> 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'.

There is a Lua-C API analogue for <debug.traceback>: luaL_traceback[1].
It was introduced in Lua 5.2 but was backported to LuaJIT and requires
no additional build flags to be enabled.

Unfortunately, I'm totally out of context, so I'm curious what prevents
us to inherit and adjust already implemented machinery presented in
<fiber_backtrace_cb>?

> 
> > +	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));

Minor: it's not the safe way to obtain Lua string data (I've already
mentioned it here[2]).

> > +	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);

One more time: please use another prefix (especially for an extern API).

> 
> 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.

[1]: https://www.lua.org/manual/5.2/manual.html#luaL_traceback
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015498.html

-- 
Best regards,
IM