From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 501E14696C4 for ; Mon, 6 Apr 2020 01:14:37 +0300 (MSK) References: <3a14bd84545d33eaded01ca0f23e12caba7eba9f.1585053742.git.lvasiliev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4f1cec3e-2e30-bb81-24aa-98f5b66a5c34@tarantool.org> Date: Mon, 6 Apr 2020 00:14:34 +0200 MIME-Version: 1.0 In-Reply-To: <3a14bd84545d33eaded01ca0f23e12caba7eba9f.1585053742.git.lvasiliev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev , alexander.turenko@tarantool.org Cc: tarantool-patches@dev.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 > #include "utils.h" > > +#include > + > 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.