* [Tarantool-patches] [PATCH v3 1/4] fiber: add PoC for fiber creation backtrace
2021-07-14 11:12 [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Egor Elchinov via Tarantool-patches
@ 2021-07-14 11:12 ` Egor Elchinov via Tarantool-patches
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 2/4] fiber: add option and PoC for Lua parent backtrace Egor Elchinov via Tarantool-patches
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Egor Elchinov via Tarantool-patches @ 2021-07-14 11:12 UTC (permalink / raw)
To: gorcunov, alyapunov; +Cc: tarantool-patches, Egor Elchinov
From: Egor Elchinov <elchinov.es@gmail.com>
For now fiber creation backtrace is stored
in the separate subtable of fiber.info
called backtrace_parent for convenience.
Lua stacks of fiber creation
aren't preserved in backtrace yet because
of need to somehow handle parent Lua state
inside the child fiber for this sake.
Backtrace caching and demangling
aren't present yet too as this is a
proof-of-concept implementation.
Due to the limited libunwind API under osx for now
this patch uses backtrace() from <execinfo.h> and
dladdr() from <dlfcn.h> instead of absent
libunwind routines.
Custom backtrace() function was renamed to
backtrace_to_buf() because of the name conflict
with backtrace() routine from <execinfo.h>.
Needed for: #4002
---
src/lib/core/backtrace.cc | 99 ++++++++++++++++++-
src/lib/core/backtrace.h | 10 +-
src/lib/core/crash.c | 2 +-
src/lib/core/fiber.c | 8 ++
src/lib/core/fiber.h | 16 +++
src/lua/fiber.c | 10 ++
.../gh-4002-fiber-creation-backtrace.result | 57 +++++++++++
.../gh-4002-fiber-creation-backtrace.test.lua | 27 +++++
8 files changed, 225 insertions(+), 4 deletions(-)
create mode 100644 test/app/gh-4002-fiber-creation-backtrace.result
create mode 100644 test/app/gh-4002-fiber-creation-backtrace.test.lua
diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc
index b4048089f..ef8a70f1b 100644
--- a/src/lib/core/backtrace.cc
+++ b/src/lib/core/backtrace.cc
@@ -46,6 +46,11 @@
#ifdef ENABLE_BACKTRACE
#include <libunwind.h>
+#ifdef TARGET_OS_DARWIN
+#include <execinfo.h>
+#include <dlfcn.h>
+#endif
+
#include "small/region.h"
#include "small/static.h"
/*
@@ -131,7 +136,7 @@ error:
}
char *
-backtrace(char *start, size_t size)
+backtrace_to_buf(char *start, size_t size)
{
int frame_no = 0;
unw_word_t sp = 0, old_sp = 0, ip, offset;
@@ -432,11 +437,101 @@ out:
free(demangle_buf);
}
+/**
+ * Collect up to `limit' IP register values
+ * for frames of the current stack into `ip_buf'.
+ * Must be by far faster than usual backtrace according to the
+ * libunwind doc for unw_backtrace().
+ */
+void NOINLINE
+backtrace_collect_ip(void **ip_buf, int limit)
+{
+ memset(ip_buf, 0, limit * sizeof(*ip_buf));
+#ifndef TARGET_OS_DARWIN
+ unw_backtrace(ip_buf, limit);
+#else
+ backtrace(ip_buf, limit);
+#endif
+}
+
+/**
+ * Call `cb' callback for not more than
+ * first `limit' frames present in the `ip_buf'.
+ *
+ * The implementation uses poorly documented `get_proc_name' callback
+ * from the `unw_accessors_t' to get procedure names via `ip_buf' values.
+ * Although `get_proc_name' is present on most architectures, it's an optional
+ * field, so procedure name is allowed to be absent (NULL) in `cb' call.
+ *
+ * TODO: to add cache and demangling support
+ */
+void
+backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
+ void *cb_ctx)
+{
+#ifndef TARGET_OS_DARWIN
+ char proc_name[BACKTRACE_NAME_MAX];
+ unw_word_t ip = 0, offset = 0;
+ unw_proc_info_t pi;
+ int frame_no, ret = 0;
+ char *proc = NULL;
+
+ unw_accessors_t *acc = unw_get_accessors(unw_local_addr_space);
+
+ /*
+ * RIPs collecting comes from inside a helper routine
+ * so we skip the collector function address itself thus
+ * start fetching functions with frame number = 1.
+ */
+ for (frame_no = 1; frame_no < limit && ip_buf[frame_no] != NULL;
+ frame_no++) {
+ ip = (unw_word_t)ip_buf[frame_no];
+
+ if (acc->get_proc_name == NULL) {
+ ret = unw_get_proc_info_by_ip(unw_local_addr_space,
+ ip, &pi, NULL);
+ offset = ip - pi.start_ip;
+ } else {
+ ret = acc->get_proc_name(unw_local_addr_space, ip,
+ proc_name, sizeof(proc_name),
+ &offset, NULL);
+ proc = proc_name;
+ }
+ if (ret != 0 || cb(frame_no - 1, (void *)ip, proc,
+ (size_t)offset, cb_ctx) != 0)
+ break;
+ }
+
+ if (ret != 0)
+ say_debug("unwinding error: %s", unw_strerror(ret));
+#else
+ int frame_no, ret = 1;
+ void *ip = NULL;
+ size_t offset = 0;
+ Dl_info dli;
+
+ for (frame_no = 1; frame_no < limit && ip_buf[frame_no] != NULL;
+ ++frame_no) {
+ ip = ip_buf[frame_no];
+ ret = dladdr(ip, &dli);
+ if (ret == 0)
+ break;
+ offset = (char *)ip - (char *)dli.dli_saddr;
+
+ if (cb(frame_no - 1, ip, dli.dli_sname, offset, cb_ctx) != 0)
+ break;
+ }
+
+ if (ret == 0)
+ say_debug("unwinding error: %i", ret);
+#endif
+}
+
void
print_backtrace(void)
{
char *start = (char *)static_alloc(SMALL_STATIC_SIZE);
- fdprintf(STDERR_FILENO, "%s", backtrace(start, SMALL_STATIC_SIZE));
+ fdprintf(STDERR_FILENO, "%s", backtrace_to_buf(start, SMALL_STATIC_SIZE));
}
#endif /* ENABLE_BACKTRACE */
diff --git a/src/lib/core/backtrace.h b/src/lib/core/backtrace.h
index e0ae56be4..4552c14e1 100644
--- a/src/lib/core/backtrace.h
+++ b/src/lib/core/backtrace.h
@@ -31,6 +31,7 @@
* SUCH DAMAGE.
*/
#include "trivia/config.h"
+#include "trivia/util.h"
#include <stddef.h>
#if defined(__cplusplus)
@@ -41,7 +42,7 @@ extern "C" {
#include <coro.h>
char *
-backtrace(char *start, size_t size);
+backtrace_to_buf(char *start, size_t size);
void print_backtrace(void);
@@ -55,6 +56,13 @@ backtrace_foreach(backtrace_cb cb, coro_context *coro_ctx, void *cb_ctx);
void
backtrace_proc_cache_clear(void);
+void NOINLINE
+backtrace_collect_ip(void **ip_buf, int limit);
+
+void
+backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
+ void *cb_ctx);
+
#endif /* ENABLE_BACKTRACE */
#if defined(__cplusplus)
diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c
index abb7837e6..d1990d68b 100644
--- a/src/lib/core/crash.c
+++ b/src/lib/core/crash.c
@@ -213,7 +213,7 @@ crash_collect(int signo, siginfo_t *siginfo, void *ucontext)
#ifdef ENABLE_BACKTRACE
char *start = cinfo->backtrace_buf;
- backtrace(start, sizeof(cinfo->backtrace_buf));
+ backtrace_to_buf(start, sizeof(cinfo->backtrace_buf));
#endif
#ifdef HAS_GREG
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 759c7da6a..924ff3c82 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -45,6 +45,11 @@
extern void cord_on_yield(void);
+#if ENABLE_BACKTRACE
+#include "backtrace.h" /* fast_trace */
+
+#endif /* ENABLE_BACKTRACE */
+
#if ENABLE_FIBER_TOP
#include <x86intrin.h> /* __rdtscp() */
@@ -1259,6 +1264,9 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr,
fiber->f = f;
fiber->fid = cord->next_fid;
fiber_set_name(fiber, name);
+#if ENABLE_BACKTRACE
+ backtrace_collect_ip(fiber->parent_bt_ip_buf, FIBER_PARENT_BT_MAX);
+#endif /* ENABLE_BACKTRACE */
register_fid(fiber);
fiber->csw = 0;
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index 8f4e14796..beed58866 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -111,6 +111,18 @@ struct cpu_stat {
#endif /* ENABLE_FIBER_TOP */
+#if ENABLE_BACKTRACE
+
+enum {
+ /**
+ * Maximum entries count to grab
+ * from the fiber creation backtrace.
+ */
+ FIBER_PARENT_BT_MAX = 8
+};
+
+#endif /* ENABLE_BACKTRACE */
+
enum {
/** Both limits include terminating 0. */
FIBER_NAME_INLINE = 40,
@@ -640,6 +652,10 @@ struct fiber {
*/
char *name;
char inline_name[FIBER_NAME_INLINE];
+#if ENABLE_BACKTRACE
+ /** Fiber creation backtrace chunk. */
+ void *parent_bt_ip_buf[FIBER_PARENT_BT_MAX];
+#endif /* ENABLE_BACKTRACE */
};
/** Invoke on_stop triggers and delete them. */
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 91898c283..7b21361d4 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -308,6 +308,16 @@ lbox_fiber_statof_map(struct fiber *f, void *cb_ctx, bool backtrace)
backtrace_foreach(fiber_backtrace_cb,
f != fiber() ? &f->ctx : NULL, &tb_ctx);
lua_settable(L, -3);
+
+ tb_ctx.lua_frame = 0;
+ tb_ctx.tb_frame = 0;
+ tb_ctx.R = NULL;
+ lua_pushstring(L, "backtrace_parent");
+ lua_newtable(L);
+ backtrace_foreach_ip(fiber_backtrace_cb,
+ f->parent_bt_ip_buf,
+ FIBER_PARENT_BT_MAX, &tb_ctx);
+ lua_settable(L, -3);
#endif /* ENABLE_BACKTRACE */
}
return 0;
diff --git a/test/app/gh-4002-fiber-creation-backtrace.result b/test/app/gh-4002-fiber-creation-backtrace.result
new file mode 100644
index 000000000..4934b82d6
--- /dev/null
+++ b/test/app/gh-4002-fiber-creation-backtrace.result
@@ -0,0 +1,57 @@
+-- test-run result file version 2
+yaml = require('yaml')
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+local stack_len = 0
+ | ---
+ | ...
+local parent_stack_len = 0
+ | ---
+ | ...
+
+test_run:cmd('setopt delimiter ";"')
+ | ---
+ | - true
+ | ...
+foo = function()
+ local id = fiber.self():id()
+ local info = fiber.info()[id]
+ local stack = info.backtrace
+ stack_len = stack and #stack or -1
+ local parent_stack = info.backtrace_parent
+ parent_stack_len = parent_stack and #parent_stack or -1
+end;
+ | ---
+ | ...
+
+test_run:cmd('setopt delimiter ""');
+ | ---
+ | - true
+ | ...
+
+local bar,baz
+ | ---
+ | ...
+
+bar = function(n) if n ~= 0 then baz(n-1) else fiber.create(foo) end end
+ | ---
+ | ...
+baz = function(n) bar(n) end
+ | ---
+ | ...
+
+baz(10)
+ | ---
+ | ...
+assert(parent_stack_len > 0 or stack_len == -1)
+ | ---
+ | - true
+ | ...
+
diff --git a/test/app/gh-4002-fiber-creation-backtrace.test.lua b/test/app/gh-4002-fiber-creation-backtrace.test.lua
new file mode 100644
index 000000000..24d41a860
--- /dev/null
+++ b/test/app/gh-4002-fiber-creation-backtrace.test.lua
@@ -0,0 +1,27 @@
+yaml = require('yaml')
+fiber = require('fiber')
+test_run = require('test_run').new()
+
+local stack_len = 0
+local parent_stack_len = 0
+
+test_run:cmd('setopt delimiter ";"')
+foo = function()
+ local id = fiber.self():id()
+ local info = fiber.info()[id]
+ local stack = info.backtrace
+ stack_len = stack and #stack or -1
+ local parent_stack = info.backtrace_parent
+ parent_stack_len = parent_stack and #parent_stack or -1
+end;
+
+test_run:cmd('setopt delimiter ""');
+
+local bar,baz
+
+bar = function(n) if n ~= 0 then baz(n-1) else fiber.create(foo) end end
+baz = function(n) bar(n) end
+
+baz(10)
+assert(parent_stack_len > 0 or stack_len == -1)
+
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [Tarantool-patches] [PATCH v3 2/4] fiber: add option and PoC for Lua parent backtrace
2021-07-14 11:12 [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Egor Elchinov via Tarantool-patches
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 1/4] fiber: add PoC for fiber " Egor Elchinov via Tarantool-patches
@ 2021-07-14 11:12 ` Egor Elchinov via Tarantool-patches
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 3/4] fiber: refactor lua backtrace routines Egor Elchinov via Tarantool-patches
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Egor Elchinov via Tarantool-patches @ 2021-07-14 11:12 UTC (permalink / raw)
To: gorcunov, alyapunov; +Cc: tarantool-patches, Egor Elchinov
From: Egor Elchinov <elchinov.es@gmail.com>
For now Lua backtrace is fully collected
at fiber creation moment and stored in a
dynamically allocated buffer.
This patch also adds option enabling to turn on and off
the collection of backtraces of parent fibers when
creating new fibers.
Needed for: #4002
@TarantoolBot document
Title: new option for fiber parent backtrace
`fiber.parent_bt_enable()` and `fiber.parent_bt_disable()`
turn on and off the ability to trace fiber creation stacks.
If present this option may cause up to 40 per cent
slowdown of fiber creation.
By default parent backtrace is disabled.
Note that fibers created while this option was disabled
lack their parent backtraces in `fiber.info()` forever.
---
src/lib/core/fiber.c | 29 ++-
src/lib/core/fiber.h | 18 ++
src/lua/fiber.c | 208 ++++++++++++++++--
src/lua/fiber.h | 19 ++
.../gh-4002-fiber-creation-backtrace.result | 21 ++
.../gh-4002-fiber-creation-backtrace.test.lua | 7 +
6 files changed, 287 insertions(+), 15 deletions(-)
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 924ff3c82..dd26e2f13 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -220,6 +220,10 @@ fiber_mprotect(void *addr, size_t len, int prot)
static __thread bool fiber_top_enabled = false;
#endif /* ENABLE_FIBER_TOP */
+#if ENABLE_BACKTRACE
+static __thread bool fiber_parent_bt_enabled = false;
+#endif /* ENABLE_BACKTRACE */
+
/**
* An action performed each time a context switch happens.
* Used to count each fiber's processing time.
@@ -1265,7 +1269,10 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr,
fiber->fid = cord->next_fid;
fiber_set_name(fiber, name);
#if ENABLE_BACKTRACE
- backtrace_collect_ip(fiber->parent_bt_ip_buf, FIBER_PARENT_BT_MAX);
+ if (fiber_parent_bt_enabled) {
+ backtrace_collect_ip(fiber->parent_bt_ip_buf,
+ FIBER_PARENT_BT_MAX);
+ }
#endif /* ENABLE_BACKTRACE */
register_fid(fiber);
fiber->csw = 0;
@@ -1414,6 +1421,26 @@ fiber_top_disable(void)
}
#endif /* ENABLE_FIBER_TOP */
+#if ENABLE_BACKTRACE
+bool
+fiber_parent_bt_is_enabled(void)
+{
+ return fiber_parent_bt_enabled;
+}
+
+void
+fiber_parent_bt_enable(void)
+{
+ fiber_parent_bt_enabled = true;
+}
+
+void
+fiber_parent_bt_disable(void)
+{
+ fiber_parent_bt_enabled = false;
+}
+#endif /* ENABLE_BACKTRACE */
+
size_t
box_region_used(void)
{
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index beed58866..477b68e4c 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -528,6 +528,7 @@ struct txn;
struct credentials;
struct lua_State;
struct ipc_wait_pad;
+struct parent_bt_lua;
struct fiber {
coro_context ctx;
@@ -634,6 +635,12 @@ struct fiber {
* Optional fiber.storage Lua reference.
*/
int ref;
+#if ENABLE_BACKTRACE
+ /**
+ * Lua parent backtrace (may be NULL)
+ */
+ struct parent_bt_lua *parent_bt;
+#endif /* ENABLE_BACKTRACE */
} lua;
/**
* Iproto sync.
@@ -871,6 +878,17 @@ void
fiber_top_disable(void);
#endif /* ENABLE_FIBER_TOP */
+#if ENABLE_BACKTRACE
+bool
+fiber_parent_bt_is_enabled(void);
+
+void
+fiber_parent_bt_enable(void);
+
+void
+fiber_parent_bt_disable(void);
+#endif /* ENABLE_BACKTRACE */
+
/** Useful for C unit tests */
static inline int
fiber_c_invoke(fiber_func f, va_list ap)
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 7b21361d4..fbf68c561 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -193,6 +193,18 @@ struct lua_fiber_tb_ctx {
int tb_frame;
};
+/**
+ * Lua parent traceback context.
+ */
+struct lua_parent_tb_ctx {
+ /* Lua stack to push values. */
+ struct lua_State *L;
+ /* Lua parent backtrace. */
+ struct parent_bt_lua *bt;
+ /* Count of traced frames (both C and Lua). */
+ int tb_frame;
+};
+
#ifdef ENABLE_BACKTRACE
static void
dump_lua_frame(struct lua_State *L, lua_Debug *ar, int tb_frame)
@@ -201,6 +213,24 @@ dump_lua_frame(struct lua_State *L, lua_Debug *ar, int tb_frame)
snprintf(buf, sizeof(buf), "%s in %s at line %i",
ar->name != NULL ? ar->name : "(unnamed)",
ar->source, ar->currentline);
+ buf[sizeof(buf) - 1] = 0;
+ lua_pushnumber(L, tb_frame);
+ lua_newtable(L);
+ lua_pushstring(L, "L");
+ lua_pushstring(L, buf);
+ lua_settable(L, -3);
+ lua_settable(L, -3);
+}
+
+static void
+dump_parent_lua_frame(struct lua_State *L, const char *name, const char *src,
+ int currentline, int tb_frame)
+{
+ char buf[512];
+ snprintf(buf, sizeof(buf), "%s in %s at line %i",
+ name != NULL ? name : "(unnamed)",
+ src, currentline);
+ buf[sizeof(buf) - 1] = 0;
lua_pushnumber(L, tb_frame);
lua_newtable(L);
lua_pushstring(L, "L");
@@ -246,11 +276,13 @@ fiber_backtrace_cb(int frameno, void *frameret, const char *func, size_t offset,
}
}
char buf[512];
- int l = snprintf(buf, sizeof(buf), "#%-2d %p in ", frameno, frameret);
+ int l = snprintf(buf, sizeof(buf), "#%-2d %p in ",
+ frameno, frameret);
if (func)
snprintf(buf + l, sizeof(buf) - l, "%s+%zu", func, offset);
else
snprintf(buf + l, sizeof(buf) - l, "?");
+ buf[sizeof(buf) - 1] = 0;
tb_ctx->tb_frame++;
lua_pushnumber(L, tb_ctx->tb_frame);
lua_newtable(L);
@@ -260,7 +292,117 @@ fiber_backtrace_cb(int frameno, void *frameret, const char *func, size_t offset,
lua_settable(L, -3);
return 0;
}
-#endif
+
+static int
+fiber_parent_backtrace_cb(int frameno, void *frameret, const char *func,
+ size_t offset, void *cb_ctx)
+{
+ int lua_frame = 0;
+ struct lua_parent_tb_ctx *tb_ctx = (struct lua_parent_tb_ctx *)cb_ctx;
+ struct parent_bt_lua *bt = tb_ctx->bt;
+ struct lua_State *L = tb_ctx->L;
+ /*
+ * There is impossible to get func == NULL until
+ * https://github.com/tarantool/tarantool/issues/5326
+ * will not resolved, but is possible afterwards.
+ */
+ if (bt != NULL && func != NULL && strstr(func, "lj_BC_FUNCC") == func) {
+ /* We are in the LUA vm. */
+ while (lua_frame < bt->cnt) {
+ tb_ctx->tb_frame++;
+ dump_parent_lua_frame(L, bt->names[lua_frame],
+ bt->sources[lua_frame],
+ bt->lines[lua_frame],
+ tb_ctx->tb_frame);
+ lua_frame++;
+ }
+ }
+ char buf[512];
+ int l = snprintf(buf, sizeof(buf), "#%-2d %p in ",
+ frameno, frameret);
+ if (func)
+ snprintf(buf + l, sizeof(buf) - l, "%s+%zu", func, offset);
+ else
+ snprintf(buf + l, sizeof(buf) - l, "?");
+ buf[sizeof(buf) - 1] = 0;
+ tb_ctx->tb_frame++;
+ lua_pushnumber(L, tb_ctx->tb_frame);
+ lua_newtable(L);
+ lua_pushstring(L, "C");
+ lua_pushstring(L, buf);
+ lua_settable(L, -3);
+ lua_settable(L, -3);
+ return 0;
+}
+
+static int
+fiber_parent_bt_init(struct fiber *f, struct lua_State *L)
+{
+ int lua_frame = 0, tb_frame = 0;
+ lua_Debug ar;
+ struct parent_bt_lua *bt = NULL;
+
+ bt = (struct parent_bt_lua *)malloc(sizeof(*bt));
+ if (bt == NULL){
+ diag_set(OutOfMemory, sizeof(*bt), "malloc", "bt");
+ return 1;
+ }
+
+ while (tb_frame < PARENT_BT_LUA_LEN_MAX &&
+ lua_getstack(L, lua_frame, &ar) > 0) {
+ /* Skip all following C-frames. */
+ lua_getinfo(L, "Sln", &ar);
+ if (*ar.what != 'C')
+ break;
+ bt->names[tb_frame][0] = 0;
+ bt->sources[tb_frame][0] = 0;
+ if (ar.name != NULL) {
+ /* Dump frame if it is a C built-in call. */
+ bt->lines[tb_frame] = ar.currentline;
+ strncpy(bt->names[tb_frame], ar.name,
+ PARENT_BT_LUA_NAME_MAX - 1);
+ bt->names[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
+ strncpy(bt->sources[tb_frame], ar.source,
+ PARENT_BT_LUA_NAME_MAX - 1);
+ bt->sources[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
+ tb_frame++;
+ }
+ lua_frame++;
+ }
+ while (tb_frame < PARENT_BT_LUA_LEN_MAX &&
+ lua_getstack(L, lua_frame, &ar) > 0) {
+ /* Trace Lua frame. */
+ lua_getinfo(L, "Sln", &ar);
+ if (*ar.what == 'C')
+ break;
+ bt->lines[tb_frame] = ar.currentline;
+ if (ar.name != NULL) {
+ strncpy(bt->names[tb_frame], ar.name,
+ PARENT_BT_LUA_NAME_MAX - 1);
+ } else {
+ strncpy(bt->names[tb_frame], "(unnamed)",
+ PARENT_BT_LUA_NAME_MAX - 1);
+ }
+ bt->names[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
+ strncpy(bt->sources[tb_frame], ar.source,
+ PARENT_BT_LUA_NAME_MAX - 1);
+ bt->sources[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
+ tb_frame++;
+ lua_frame++;
+ }
+
+ bt->cnt = tb_frame;
+ f->storage.lua.parent_bt = bt;
+
+ return 0;
+}
+
+static void
+fiber_parent_bt_free(struct fiber *f)
+{
+ free(f->storage.lua.parent_bt);
+}
+#endif /* ENABLE_BACKTRACE */
static int
lbox_fiber_statof_map(struct fiber *f, void *cb_ctx, bool backtrace)
@@ -299,6 +441,7 @@ lbox_fiber_statof_map(struct fiber *f, void *cb_ctx, bool backtrace)
if (backtrace) {
#ifdef ENABLE_BACKTRACE
struct lua_fiber_tb_ctx tb_ctx;
+ struct lua_parent_tb_ctx parent_tb_ctx;
tb_ctx.L = L;
tb_ctx.R = f->storage.lua.stack;
tb_ctx.lua_frame = 0;
@@ -309,15 +452,18 @@ lbox_fiber_statof_map(struct fiber *f, void *cb_ctx, bool backtrace)
f != fiber() ? &f->ctx : NULL, &tb_ctx);
lua_settable(L, -3);
- tb_ctx.lua_frame = 0;
- tb_ctx.tb_frame = 0;
- tb_ctx.R = NULL;
- lua_pushstring(L, "backtrace_parent");
- lua_newtable(L);
- backtrace_foreach_ip(fiber_backtrace_cb,
- f->parent_bt_ip_buf,
- FIBER_PARENT_BT_MAX, &tb_ctx);
- lua_settable(L, -3);
+ if (fiber_parent_bt_is_enabled()) {
+ parent_tb_ctx.L = L;
+ parent_tb_ctx.bt = f->storage.lua.parent_bt;
+ parent_tb_ctx.tb_frame = 0;
+ lua_pushstring(L, "backtrace_parent");
+ lua_newtable(L);
+ backtrace_foreach_ip(fiber_parent_backtrace_cb,
+ f->parent_bt_ip_buf,
+ FIBER_PARENT_BT_MAX,
+ &parent_tb_ctx);
+ lua_settable(L, -3);
+ }
#endif /* ENABLE_BACKTRACE */
}
return 0;
@@ -441,6 +587,22 @@ lbox_do_backtrace(struct lua_State *L)
}
return true;
}
+
+static int
+lbox_fiber_parent_bt_enable(struct lua_State *L)
+{
+ (void) L;
+ fiber_parent_bt_enable();
+ return 0;
+}
+
+static int
+lbox_fiber_parent_bt_disable(struct lua_State *L)
+{
+ (void) L;
+ fiber_parent_bt_disable();
+ return 0;
+}
#endif /* ENABLE_BACKTRACE */
/**
@@ -481,10 +643,14 @@ lua_fiber_run_f(MAYBE_UNUSED va_list ap)
* We can unref child stack here,
* otherwise we have to unref child stack in join
*/
- if (f->flags & FIBER_IS_JOINABLE)
+ if (f->flags & FIBER_IS_JOINABLE) {
lua_pushinteger(L, coro_ref);
- else
+ } else {
+#ifdef ENABLE_BACKTRACE
+ fiber_parent_bt_free(f);
+#endif
luaL_unref(L, LUA_REGISTRYINDEX, coro_ref);
+ }
return result;
}
@@ -506,6 +672,12 @@ fiber_create(struct lua_State *L)
luaT_error(L);
}
+#ifdef ENABLE_BACKTRACE
+ // TODO: error handling
+ if (fiber_parent_bt_is_enabled())
+ fiber_parent_bt_init(f, L);
+#endif
+
/* Move the arguments to the new coro */
lua_xmove(L, child_L, lua_gettop(L));
/* XXX: 'fiber' is leaked if this throws a Lua error. */
@@ -886,8 +1058,12 @@ lbox_fiber_join(struct lua_State *L)
lua_xmove(child_L, L, num_ret);
}
}
- if (child_L != NULL)
+ if (child_L != NULL) {
+#ifdef ENABLE_BACKTRACE
+ fiber_parent_bt_free(fiber);
+#endif
luaL_unref(L, LUA_REGISTRYINDEX, coro_ref);
+ }
return num_ret + 1;
}
@@ -941,6 +1117,10 @@ static const struct luaL_Reg fiberlib[] = {
{"top_enable", lbox_fiber_top_enable},
{"top_disable", lbox_fiber_top_disable},
#endif /* ENABLE_FIBER_TOP */
+#if ENABLE_BACKTRACE
+ {"parent_bt_enable", lbox_fiber_parent_bt_enable},
+ {"parent_bt_disable", lbox_fiber_parent_bt_disable},
+#endif /* ENABLE_BACKTRACE */
{"sleep", lbox_fiber_sleep},
{"yield", lbox_fiber_yield},
{"self", lbox_fiber_self},
diff --git a/src/lua/fiber.h b/src/lua/fiber.h
index e29898706..450840fb0 100644
--- a/src/lua/fiber.h
+++ b/src/lua/fiber.h
@@ -36,6 +36,25 @@ extern "C" {
struct lua_State;
+/**
+ * Maximal name length (including '\0')
+ * and backtrace length.
+ */
+enum {
+ PARENT_BT_LUA_NAME_MAX = 64,
+ PARENT_BT_LUA_LEN_MAX = 8
+};
+
+/**
+ * Stores lua parent backtrace for fiber.
+ */
+struct parent_bt_lua {
+ int cnt;
+ char names[PARENT_BT_LUA_LEN_MAX][PARENT_BT_LUA_NAME_MAX];
+ char sources[PARENT_BT_LUA_LEN_MAX][PARENT_BT_LUA_NAME_MAX];
+ int lines[PARENT_BT_LUA_LEN_MAX];
+};
+
/**
* Initialize box.fiber system
*/
diff --git a/test/app/gh-4002-fiber-creation-backtrace.result b/test/app/gh-4002-fiber-creation-backtrace.result
index 4934b82d6..6a075911d 100644
--- a/test/app/gh-4002-fiber-creation-backtrace.result
+++ b/test/app/gh-4002-fiber-creation-backtrace.result
@@ -47,6 +47,17 @@ baz = function(n) bar(n) end
| ---
| ...
+baz(10)
+ | ---
+ | ...
+assert(parent_stack_len == -1)
+ | ---
+ | - true
+ | ...
+
+if stack_len ~= -1 then fiber:parent_bt_enable() end
+ | ---
+ | ...
baz(10)
| ---
| ...
@@ -55,3 +66,13 @@ assert(parent_stack_len > 0 or stack_len == -1)
| - true
| ...
+if stack_len ~= -1 then fiber:parent_bt_disable() end
+ | ---
+ | ...
+baz(10)
+ | ---
+ | ...
+assert(parent_stack_len == -1)
+ | ---
+ | - true
+ | ...
diff --git a/test/app/gh-4002-fiber-creation-backtrace.test.lua b/test/app/gh-4002-fiber-creation-backtrace.test.lua
index 24d41a860..79a516860 100644
--- a/test/app/gh-4002-fiber-creation-backtrace.test.lua
+++ b/test/app/gh-4002-fiber-creation-backtrace.test.lua
@@ -22,6 +22,13 @@ local bar,baz
bar = function(n) if n ~= 0 then baz(n-1) else fiber.create(foo) end end
baz = function(n) bar(n) end
+baz(10)
+assert(parent_stack_len == -1)
+
+if stack_len ~= -1 then fiber:parent_bt_enable() end
baz(10)
assert(parent_stack_len > 0 or stack_len == -1)
+if stack_len ~= -1 then fiber:parent_bt_disable() end
+baz(10)
+assert(parent_stack_len == -1)
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [Tarantool-patches] [PATCH v3 3/4] fiber: refactor lua backtrace routines
2021-07-14 11:12 [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Egor Elchinov via Tarantool-patches
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 1/4] fiber: add PoC for fiber " Egor Elchinov via Tarantool-patches
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 2/4] fiber: add option and PoC for Lua parent backtrace Egor Elchinov via Tarantool-patches
@ 2021-07-14 11:12 ` Egor Elchinov via Tarantool-patches
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 4/4] fiber: refactor C backtrace and add changelog Egor Elchinov via Tarantool-patches
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Egor Elchinov via Tarantool-patches @ 2021-07-14 11:12 UTC (permalink / raw)
To: gorcunov, alyapunov; +Cc: tarantool-patches, Egor Elchinov
From: Egor Elchinov <elchinov.es@gmail.com>
Lua backtrace with callback moved to new routine
`lua_backtrace_cb()` similar to the `backtrace_cb()`
for C backtrace.
Needed for: #4002
---
src/lua/fiber.c | 170 ++++++++++++++++++++++--------------------------
1 file changed, 76 insertions(+), 94 deletions(-)
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index fbf68c561..59d72fc52 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -205,14 +205,17 @@ struct lua_parent_tb_ctx {
int tb_frame;
};
+/** Lua backtrace callback type */
+typedef int (lua_backtrace_cb)(struct lua_Debug *ar, int frame_no, void *cb_ctx);
+
#ifdef ENABLE_BACKTRACE
static void
-dump_lua_frame(struct lua_State *L, lua_Debug *ar, int tb_frame)
+dump_lua_frame(struct lua_State *L, const char *name, const char *src, int line,
+ int tb_frame)
{
char buf[512];
snprintf(buf, sizeof(buf), "%s in %s at line %i",
- ar->name != NULL ? ar->name : "(unnamed)",
- ar->source, ar->currentline);
+ name != NULL ? name : "(unnamed)", src, line);
buf[sizeof(buf) - 1] = 0;
lua_pushnumber(L, tb_frame);
lua_newtable(L);
@@ -222,21 +225,67 @@ dump_lua_frame(struct lua_State *L, lua_Debug *ar, int tb_frame)
lua_settable(L, -3);
}
+static int
+dump_lua_frame_cb(lua_Debug *ar, int tb_frame, void *cb_ctx)
+{
+ (void) tb_frame;
+ struct lua_fiber_tb_ctx *tb_ctx = (struct lua_fiber_tb_ctx *)cb_ctx;
+
+ tb_ctx->tb_frame++;
+ dump_lua_frame(tb_ctx->L, ar->name, ar->source, ar->currentline,
+ tb_ctx->tb_frame);
+
+ return 0;
+}
+
+static int
+save_lua_frame_cb(lua_Debug *ar, int tb_frame, void *cb_ctx)
+{
+ struct parent_bt_lua *bt = (struct parent_bt_lua *)cb_ctx;
+
+ if (tb_frame >= PARENT_BT_LUA_LEN_MAX)
+ return 1;
+
+ bt->cnt++;
+ bt->lines[tb_frame] = ar->currentline;
+ strncpy(bt->names[tb_frame], ar->name == NULL ? "(unnamed)" : ar->name,
+ PARENT_BT_LUA_NAME_MAX - 1);
+ bt->names[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
+ strncpy(bt->sources[tb_frame], ar->source,
+ PARENT_BT_LUA_NAME_MAX - 1);
+ bt->sources[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
+
+ return 0;
+}
+
static void
-dump_parent_lua_frame(struct lua_State *L, const char *name, const char *src,
- int currentline, int tb_frame)
+lua_backtrace_foreach(struct lua_State *L, lua_backtrace_cb cb, void *cb_ctx)
{
- char buf[512];
- snprintf(buf, sizeof(buf), "%s in %s at line %i",
- name != NULL ? name : "(unnamed)",
- src, currentline);
- buf[sizeof(buf) - 1] = 0;
- lua_pushnumber(L, tb_frame);
- lua_newtable(L);
- lua_pushstring(L, "L");
- lua_pushstring(L, buf);
- lua_settable(L, -3);
- lua_settable(L, -3);
+ int lua_frame = 0, tb_frame = 0;
+ lua_Debug ar;
+ while (lua_getstack(L, lua_frame, &ar) > 0) {
+ /* Skip all following C-frames. */
+ lua_getinfo(L, "Sln", &ar);
+ if (*ar.what != 'C')
+ break;
+ if (ar.name != NULL) {
+ /* Dump frame if it is a C built-in call. */
+ if (cb(&ar, tb_frame, cb_ctx) != 0)
+ return;
+ tb_frame++;
+ }
+ lua_frame++;
+ }
+ while (lua_getstack(L, lua_frame, &ar) > 0) {
+ /* Trace Lua frame. */
+ lua_getinfo(L, "Sln", &ar);
+ if (*ar.what == 'C')
+ break;
+ if (cb(&ar, tb_frame, cb_ctx) != 0)
+ return;
+ tb_frame++;
+ lua_frame++;
+ }
}
static int
@@ -249,31 +298,9 @@ fiber_backtrace_cb(int frameno, void *frameret, const char *func, size_t offset,
* https://github.com/tarantool/tarantool/issues/5326
* will not resolved, but is possible afterwards.
*/
- if (func != NULL && strstr(func, "lj_BC_FUNCC") == func) {
+ if (func != NULL && tb_ctx->R && strstr(func, "lj_BC_FUNCC") == func) {
/* We are in the LUA vm. */
- lua_Debug ar;
- while (tb_ctx->R && lua_getstack(tb_ctx->R, tb_ctx->lua_frame, &ar) > 0) {
- /* Skip all following C-frames. */
- lua_getinfo(tb_ctx->R, "Sln", &ar);
- if (*ar.what != 'C')
- break;
- if (ar.name != NULL) {
- /* Dump frame if it is a C built-in call. */
- tb_ctx->tb_frame++;
- dump_lua_frame(L, &ar, tb_ctx->tb_frame);
- }
- tb_ctx->lua_frame++;
- }
- while (tb_ctx->R && lua_getstack(tb_ctx->R, tb_ctx->lua_frame, &ar) > 0) {
- /* Trace Lua frame. */
- lua_getinfo(tb_ctx->R, "Sln", &ar);
- if (*ar.what == 'C') {
- break;
- }
- tb_ctx->tb_frame++;
- dump_lua_frame(L, &ar, tb_ctx->tb_frame);
- tb_ctx->lua_frame++;
- }
+ lua_backtrace_foreach(tb_ctx->R, dump_lua_frame_cb, tb_ctx);
}
char buf[512];
int l = snprintf(buf, sizeof(buf), "#%-2d %p in ",
@@ -310,10 +337,10 @@ fiber_parent_backtrace_cb(int frameno, void *frameret, const char *func,
/* We are in the LUA vm. */
while (lua_frame < bt->cnt) {
tb_ctx->tb_frame++;
- dump_parent_lua_frame(L, bt->names[lua_frame],
- bt->sources[lua_frame],
- bt->lines[lua_frame],
- tb_ctx->tb_frame);
+ dump_lua_frame(L, bt->names[lua_frame],
+ bt->sources[lua_frame],
+ bt->lines[lua_frame],
+ tb_ctx->tb_frame);
lua_frame++;
}
}
@@ -338,60 +365,15 @@ fiber_parent_backtrace_cb(int frameno, void *frameret, const char *func,
static int
fiber_parent_bt_init(struct fiber *f, struct lua_State *L)
{
- int lua_frame = 0, tb_frame = 0;
- lua_Debug ar;
struct parent_bt_lua *bt = NULL;
-
- bt = (struct parent_bt_lua *)malloc(sizeof(*bt));
+ bt = (struct parent_bt_lua *)calloc(1, sizeof(*bt));
if (bt == NULL){
- diag_set(OutOfMemory, sizeof(*bt), "malloc", "bt");
+ diag_set(OutOfMemory, sizeof(*bt), "calloc", "bt");
return 1;
}
- while (tb_frame < PARENT_BT_LUA_LEN_MAX &&
- lua_getstack(L, lua_frame, &ar) > 0) {
- /* Skip all following C-frames. */
- lua_getinfo(L, "Sln", &ar);
- if (*ar.what != 'C')
- break;
- bt->names[tb_frame][0] = 0;
- bt->sources[tb_frame][0] = 0;
- if (ar.name != NULL) {
- /* Dump frame if it is a C built-in call. */
- bt->lines[tb_frame] = ar.currentline;
- strncpy(bt->names[tb_frame], ar.name,
- PARENT_BT_LUA_NAME_MAX - 1);
- bt->names[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
- strncpy(bt->sources[tb_frame], ar.source,
- PARENT_BT_LUA_NAME_MAX - 1);
- bt->sources[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
- tb_frame++;
- }
- lua_frame++;
- }
- while (tb_frame < PARENT_BT_LUA_LEN_MAX &&
- lua_getstack(L, lua_frame, &ar) > 0) {
- /* Trace Lua frame. */
- lua_getinfo(L, "Sln", &ar);
- if (*ar.what == 'C')
- break;
- bt->lines[tb_frame] = ar.currentline;
- if (ar.name != NULL) {
- strncpy(bt->names[tb_frame], ar.name,
- PARENT_BT_LUA_NAME_MAX - 1);
- } else {
- strncpy(bt->names[tb_frame], "(unnamed)",
- PARENT_BT_LUA_NAME_MAX - 1);
- }
- bt->names[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
- strncpy(bt->sources[tb_frame], ar.source,
- PARENT_BT_LUA_NAME_MAX - 1);
- bt->sources[tb_frame][PARENT_BT_LUA_NAME_MAX - 1] = 0;
- tb_frame++;
- lua_frame++;
- }
-
- bt->cnt = tb_frame;
+ bt->cnt = 0;
+ lua_backtrace_foreach(L, save_lua_frame_cb, bt);
f->storage.lua.parent_bt = bt;
return 0;
@@ -1117,7 +1099,7 @@ static const struct luaL_Reg fiberlib[] = {
{"top_enable", lbox_fiber_top_enable},
{"top_disable", lbox_fiber_top_disable},
#endif /* ENABLE_FIBER_TOP */
-#if ENABLE_BACKTRACE
+#ifdef ENABLE_BACKTRACE
{"parent_bt_enable", lbox_fiber_parent_bt_enable},
{"parent_bt_disable", lbox_fiber_parent_bt_disable},
#endif /* ENABLE_BACKTRACE */
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [Tarantool-patches] [PATCH v3 4/4] fiber: refactor C backtrace and add changelog
2021-07-14 11:12 [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Egor Elchinov via Tarantool-patches
` (2 preceding siblings ...)
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 3/4] fiber: refactor lua backtrace routines Egor Elchinov via Tarantool-patches
@ 2021-07-14 11:12 ` Egor Elchinov via Tarantool-patches
2021-07-16 6:12 ` [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Cyrill Gorcunov via Tarantool-patches
2021-07-27 11:53 ` Vladimir Davydov via Tarantool-patches
5 siblings, 0 replies; 7+ messages in thread
From: Egor Elchinov via Tarantool-patches @ 2021-07-14 11:12 UTC (permalink / raw)
To: gorcunov, alyapunov; +Cc: tarantool-patches, Egor Elchinov
From: Egor Elchinov <elchinov.es@gmail.com>
Backtrace cache support and cxx demangling added.
DWARF version and lua test fixed
to support specific platforms.
Changelog added.
Closes: #4002
---
.../gh-4002-fiber-creation-backtrace.md | 8 +
src/lib/core/backtrace.cc | 169 +++++++++++++-----
.../gh-4002-fiber-creation-backtrace.result | 4 +-
.../gh-4002-fiber-creation-backtrace.test.lua | 4 +-
4 files changed, 135 insertions(+), 50 deletions(-)
create mode 100644 changelogs/unreleased/gh-4002-fiber-creation-backtrace.md
diff --git a/changelogs/unreleased/gh-4002-fiber-creation-backtrace.md b/changelogs/unreleased/gh-4002-fiber-creation-backtrace.md
new file mode 100644
index 000000000..1e1ac177a
--- /dev/null
+++ b/changelogs/unreleased/gh-4002-fiber-creation-backtrace.md
@@ -0,0 +1,8 @@
+## feature/fiber
+
+ * Added new subtable `parent_backtrace` to the `fiber.info()`
+ containing C and Lua backtrace chunks of fiber creation.
+ * Added `fiber.parent_bt_enable()` and `fiber.parent_bt_disable()`
+ options in order to switch on/off the ability to collect
+ parent backtraces for newly created fibers and to
+ show/hide `parent_backtrace` subtables in the `fiber.info()`.
diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc
index ef8a70f1b..355b25d7e 100644
--- a/src/lib/core/backtrace.cc
+++ b/src/lib/core/backtrace.cc
@@ -47,7 +47,6 @@
#include <libunwind.h>
#ifdef TARGET_OS_DARWIN
-#include <execinfo.h>
#include <dlfcn.h>
#endif
@@ -79,19 +78,29 @@ backtrace_proc_cache_clear(void)
proc_cache = NULL;
}
-const char *
-get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache)
+int
+backtrace_proc_cache_find(unw_word_t ip, const char **name, unw_word_t *offset)
{
- static __thread char proc_name[BACKTRACE_NAME_MAX];
- unw_word_t ip;
- unw_get_reg(unw_cur, UNW_REG_IP, &ip);
+ struct proc_cache_entry *entry;
+ mh_int_t k;
- if (skip_cache) {
- unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name),
- offset);
- return proc_name;
+ if (proc_cache != NULL) {
+ k = mh_i64ptr_find(proc_cache, ip, NULL);
+ if (k != mh_end(proc_cache)) {
+ entry = (struct proc_cache_entry *)
+ mh_i64ptr_node(proc_cache, k)->val;
+ *offset = entry->offset;
+ *name = entry->name;
+ return 0;
+ }
}
+ return -1;
+}
+
+int
+backtrace_proc_cache_put(unw_word_t ip, const char *name, unw_word_t offset)
+{
struct proc_cache_entry *entry;
struct mh_i64ptr_node_t node;
mh_int_t k;
@@ -99,39 +108,52 @@ get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache)
if (proc_cache == NULL) {
region_create(&cache_region, &cord()->slabc);
proc_cache = mh_i64ptr_new();
- if (proc_cache == NULL) {
- unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name),
- offset);
- goto error;
- }
+ if (proc_cache == NULL)
+ return -1;
+ }
+
+ size_t size;
+ entry = region_alloc_object(&cache_region, typeof(*entry),
+ &size);
+ if (entry == NULL)
+ return -1;
+
+ node.key = ip;
+ node.val = entry;
+ entry->offset = offset;
+ snprintf(entry->name, BACKTRACE_NAME_MAX - 1, "%s", name);
+ entry->name[BACKTRACE_NAME_MAX - 1] = 0;
+
+ k = mh_i64ptr_put(proc_cache, &node, NULL, NULL);
+ if (k == mh_end(proc_cache)) {
+ size_t used = region_used(&cache_region);
+ region_truncate(&cache_region, used - size);
+ return -1;
}
- k = mh_i64ptr_find(proc_cache, ip, NULL);
- if (k != mh_end(proc_cache)) {
- entry = (struct proc_cache_entry *)
- mh_i64ptr_node(proc_cache, k)->val;
- snprintf(proc_name, BACKTRACE_NAME_MAX, "%s", entry->name);
- *offset = entry->offset;
- } else {
+ return 0;
+}
+
+const char *
+get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache)
+{
+ static __thread char proc_name[BACKTRACE_NAME_MAX];
+ const char *cache_name;
+ unw_word_t ip;
+
+ if (skip_cache) {
unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name),
offset);
- size_t size;
- entry = region_alloc_object(&cache_region, typeof(*entry),
- &size);
- if (entry == NULL)
- goto error;
- node.key = ip;
- node.val = entry;
- snprintf(entry->name, BACKTRACE_NAME_MAX, "%s", proc_name);
- entry->offset = *offset;
-
- k = mh_i64ptr_put(proc_cache, &node, NULL, NULL);
- if (k == mh_end(proc_cache)) {
- free(entry);
- goto error;
- }
+ return proc_name;
+ }
+
+ unw_get_reg(unw_cur, UNW_REG_IP, &ip);
+ if (backtrace_proc_cache_find(ip, &cache_name, offset) == 0) {
+ return cache_name;
}
-error:
+
+ unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name), offset);
+ backtrace_proc_cache_put(ip, proc_name, *offset);
return proc_name;
}
@@ -450,7 +472,25 @@ backtrace_collect_ip(void **ip_buf, int limit)
#ifndef TARGET_OS_DARWIN
unw_backtrace(ip_buf, limit);
#else
- backtrace(ip_buf, limit);
+ /*
+ * This dumb implementation was chosen because the DARWIN
+ * lacks unw_backtrace() routine from libunwind and
+ * usual backtrace() from <execinfo.h> has less capabilities
+ * than the libunwind version which uses DWARF.
+ */
+ unw_cursor_t unw_cur;
+ unw_context_t unw_ctx;
+ int frame_no = 0;
+ unw_word_t ip;
+
+ unw_getcontext(&unw_ctx);
+ unw_init_local(&unw_cur, &unw_ctx);
+
+ while (frame_no < limit && unw_step(&unw_cur) > 0) {
+ unw_get_reg(&unw_cur, UNW_REG_IP, &ip);
+ ip_buf[frame_no] = (void *)ip;
+ ++frame_no;
+ }
#endif
}
@@ -469,12 +509,15 @@ void
backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
void *cb_ctx)
{
+ int demangle_status;
+ char *demangle_buf = NULL;
+ size_t demangle_buf_len = 0;
#ifndef TARGET_OS_DARWIN
char proc_name[BACKTRACE_NAME_MAX];
unw_word_t ip = 0, offset = 0;
unw_proc_info_t pi;
int frame_no, ret = 0;
- char *proc = NULL;
+ const char *proc = NULL;
unw_accessors_t *acc = unw_get_accessors(unw_local_addr_space);
@@ -487,21 +530,37 @@ backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
frame_no++) {
ip = (unw_word_t)ip_buf[frame_no];
- if (acc->get_proc_name == NULL) {
+ if (backtrace_proc_cache_find(ip, &proc, &offset) == 0) {
+ ret = 0;
+ } else if (acc->get_proc_name == NULL) {
ret = unw_get_proc_info_by_ip(unw_local_addr_space,
ip, &pi, NULL);
offset = ip - pi.start_ip;
+ proc = NULL;
+ backtrace_proc_cache_put(ip, proc, offset);
} else {
ret = acc->get_proc_name(unw_local_addr_space, ip,
proc_name, sizeof(proc_name),
&offset, NULL);
proc = proc_name;
+ backtrace_proc_cache_put(ip, proc, offset);
+ }
+
+ if (proc != NULL) {
+ char *cxxname = abi::__cxa_demangle(proc, demangle_buf,
+ &demangle_buf_len,
+ &demangle_status);
+ if (cxxname != NULL) {
+ demangle_buf = cxxname;
+ proc = cxxname;
+ }
}
if (ret != 0 || cb(frame_no - 1, (void *)ip, proc,
(size_t)offset, cb_ctx) != 0)
break;
}
+ free(demangle_buf);
if (ret != 0)
say_debug("unwinding error: %s", unw_strerror(ret));
#else
@@ -509,19 +568,37 @@ backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
void *ip = NULL;
size_t offset = 0;
Dl_info dli;
+ const char *proc = NULL;
for (frame_no = 1; frame_no < limit && ip_buf[frame_no] != NULL;
++frame_no) {
ip = ip_buf[frame_no];
- ret = dladdr(ip, &dli);
- if (ret == 0)
- break;
- offset = (char *)ip - (char *)dli.dli_saddr;
+ if (backtrace_proc_cache_find((unw_word_t)ip, &proc,
+ &offset) == 0) {
+ ret = 1;
+ } else {
+ ret = dladdr(ip, &dli);
+ if (ret == 0)
+ break;
+ offset = (char *)ip - (char *)dli.dli_saddr;
+ proc = dli.dli_sname;
+ backtrace_proc_cache_put((unw_word_t)ip, proc, offset);
+ }
- if (cb(frame_no - 1, ip, dli.dli_sname, offset, cb_ctx) != 0)
+ if (proc != NULL) {
+ char *cxxname = abi::__cxa_demangle(proc, demangle_buf,
+ &demangle_buf_len,
+ &demangle_status);
+ if (cxxname != NULL) {
+ demangle_buf = cxxname;
+ proc = cxxname;
+ }
+ }
+ if (cb(frame_no - 1, ip, proc, offset, cb_ctx) != 0)
break;
}
+ free(demangle_buf);
if (ret == 0)
say_debug("unwinding error: %i", ret);
#endif
diff --git a/test/app/gh-4002-fiber-creation-backtrace.result b/test/app/gh-4002-fiber-creation-backtrace.result
index 6a075911d..91b0cb9ce 100644
--- a/test/app/gh-4002-fiber-creation-backtrace.result
+++ b/test/app/gh-4002-fiber-creation-backtrace.result
@@ -9,10 +9,10 @@ test_run = require('test_run').new()
| ---
| ...
-local stack_len = 0
+stack_len = 0
| ---
| ...
-local parent_stack_len = 0
+parent_stack_len = 0
| ---
| ...
diff --git a/test/app/gh-4002-fiber-creation-backtrace.test.lua b/test/app/gh-4002-fiber-creation-backtrace.test.lua
index 79a516860..3c39bb48e 100644
--- a/test/app/gh-4002-fiber-creation-backtrace.test.lua
+++ b/test/app/gh-4002-fiber-creation-backtrace.test.lua
@@ -2,8 +2,8 @@ yaml = require('yaml')
fiber = require('fiber')
test_run = require('test_run').new()
-local stack_len = 0
-local parent_stack_len = 0
+stack_len = 0
+parent_stack_len = 0
test_run:cmd('setopt delimiter ";"')
foo = function()
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace
2021-07-14 11:12 [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Egor Elchinov via Tarantool-patches
` (3 preceding siblings ...)
2021-07-14 11:12 ` [Tarantool-patches] [PATCH v3 4/4] fiber: refactor C backtrace and add changelog Egor Elchinov via Tarantool-patches
@ 2021-07-16 6:12 ` Cyrill Gorcunov via Tarantool-patches
2021-07-27 11:53 ` Vladimir Davydov via Tarantool-patches
5 siblings, 0 replies; 7+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-07-16 6:12 UTC (permalink / raw)
To: Egor Elchinov via Tarantool-patches; +Cc: Egor Elchinov
On Wed, Jul 14, 2021 at 02:12:48PM +0300, Egor Elchinov via Tarantool-patches wrote:
> From: Egor Elchinov <elchinov.es@gmail.com>
>
> This patchset adds the subtable `backtrace_parent()`
> to `fiber.info()` holding backtraces of fiber
> creation and introduces `fiber.parent_bt_enable()`
> and `fiber.parent_bt_disable()` procedures managing
> the runtime ability to collect parent backtraces for
> newly created fibers.
>
> Changes in v2:
> - first 3 patches merged together
> - unnecessary changes in `ENABLE_BACKTRACE` usages reverted
>
> Changes in v3:
> - strncpy() and snprintf() usages fixed
> - error check for dladdr() under macos added
The series looks good to me, well done, Egor! Ack.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace
2021-07-14 11:12 [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Egor Elchinov via Tarantool-patches
` (4 preceding siblings ...)
2021-07-16 6:12 ` [Tarantool-patches] [PATCH v3 0/4] fiber: introduce creation backtrace Cyrill Gorcunov via Tarantool-patches
@ 2021-07-27 11:53 ` Vladimir Davydov via Tarantool-patches
5 siblings, 0 replies; 7+ messages in thread
From: Vladimir Davydov via Tarantool-patches @ 2021-07-27 11:53 UTC (permalink / raw)
To: Egor Elchinov via Tarantool-patches; +Cc: Egor Elchinov, gorcunov
On Wed, Jul 14, 2021 at 02:12:48PM +0300, Egor Elchinov via Tarantool-patches wrote:
> https://github.com/tarantool/tarantool/tree/Egor2001/gh-4002-fiber-creation-backtrace
>
> Egor Elchinov (4):
> fiber: add PoC for fiber creation backtrace
> fiber: add option and PoC for Lua parent backtrace
> fiber: refactor lua backtrace routines
> fiber: refactor C backtrace and add changelog
Kirill asked me to take a look at this series. The branch contains a
different set of patches. I'm going to review what you pushed to the
branch.
Side note: the series is not very well split IMO. The goal of splitting
is to speed up the review process so it's good to split a series in such
a way that each patch can be viewed at and applied independently of the
following patches. In particular, all code introduced in a patch should
be fully covered by existing tests or tests added in the patch so that
it can be applied before the rest of the series. In your case it isn't
so:
- fiber: add fiber creation Lua backtrace
Adds some C functions that aren't used, nor tested.
- fiber: add fiber creation C backtrace with option
Adds a knob that does nothing + some struct members that aren't used.
- backtrace: add RIP tracing and resolving API
Introduces the rest of the feature.
As a result, a reviewer has to jump between patch 3 and patches 1, 2 or
ignore the splitting and review the resulting diff instead. I'm going to
do the latter.
You do a lot of refactoring in your patch, e.g. factor out helpers to
work with proc name cache. I'd do each independent piece of refactoring
in a separate, preparatory patch.
There are two main concerns regarding this patch, which should be
addressed if possible:
- I think that we should include backtraces of all ancestors into a
fiber's backtrace, not just its parent. Without it, the feature
doesn't seem to be complete.
- There's a lot of code duplication between the code handling child and
parent backtraces. This makes the code difficult for understanding
and future support. It would be nice to have one function that
captures a backtrace and stores it in some format and one function
that dumps a captured backtrace to Lua. And use these two functions
for capturing and showing both child and parent backtraces. Can it be
achieved?
Looked through the patch. Some minor comments below.
> diff --git a/changelogs/unreleased/gh-4002-fiber-creation-backtrace.md b/changelogs/unreleased/gh-4002-fiber-creation-backtrace.md
> new file mode 100644
> index 000000000000..1e1ac177a6c6
> --- /dev/null
> +++ b/changelogs/unreleased/gh-4002-fiber-creation-backtrace.md
> @@ -0,0 +1,8 @@
> +## feature/fiber
Should be feature/core I think.
> +
> + * Added new subtable `parent_backtrace` to the `fiber.info()`
> + containing C and Lua backtrace chunks of fiber creation.
> + * Added `fiber.parent_bt_enable()` and `fiber.parent_bt_disable()`
> + options in order to switch on/off the ability to collect
> + parent backtraces for newly created fibers and to
> + show/hide `parent_backtrace` subtables in the `fiber.info()`.
Please rephrase and put under one bullet point - it's one feature that
can be enabled/disabled, not two features.
> diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc
> index 85c1aaefd009..a59b77053582 100644
> --- a/src/lib/core/backtrace.cc
> +++ b/src/lib/core/backtrace.cc
> @@ -74,12 +78,68 @@ backtrace_proc_cache_clear(void)
> proc_cache = NULL;
> }
>
> +int
> +backtrace_proc_cache_find(unw_word_t ip, const char **name, unw_word_t *offset)
Should be static.
> +int
> +backtrace_proc_cache_put(unw_word_t ip, const char *name, unw_word_t offset)
Should be static. The return value is never used, please remove.
Adding these two functions would be a good preparatory patch that could
be applied before the rest of the series is even reviewed.
> +/**
> + * Collect up to `limit' IP register values
> + * for frames of the current stack into `ip_buf'.
> + * Must be by far faster than usual backtrace according to the
> + * libunwind doc for unw_backtrace().
> + */
> +void NOINLINE
> +backtrace_collect_ip(void **ip_buf, int limit)
> +{
> + memset(ip_buf, 0, limit * sizeof(*ip_buf));
> +#ifndef TARGET_OS_DARWIN
> + unw_backtrace(ip_buf, limit);
> +#else
> + /*
> + * This dumb implementation was chosen because the DARWIN
> + * lacks unw_backtrace() routine from libunwind and
> + * usual backtrace() from <execinfo.h> has less capabilities
> + * than the libunwind version which uses DWARF.
> + */
> + unw_cursor_t unw_cur;
> + unw_context_t unw_ctx;
> + int frame_no = 0;
> + unw_word_t ip;
> +
> + unw_getcontext(&unw_ctx);
> + unw_init_local(&unw_cur, &unw_ctx);
> +
> + while (frame_no < limit && unw_step(&unw_cur) > 0) {
> + unw_get_reg(&unw_cur, UNW_REG_IP, &ip);
> + ip_buf[frame_no] = (void *)ip;
> + ++frame_no;
> + }
> +#endif
> +}
> +
> +/**
> + * Call `cb' callback for not more than
> + * first `limit' frames present in the `ip_buf'.
> + *
> + * The implementation uses poorly documented `get_proc_name' callback
> + * from the `unw_accessors_t' to get procedure names via `ip_buf' values.
> + * Although `get_proc_name' is present on most architectures, it's an optional
> + * field, so procedure name is allowed to be absent (NULL) in `cb' call.
> + *
> + * TODO: to add cache and demangling support
Who's going to do that and when? Is there a ticket for that? What won't
work without "cache and demangling support"?
It would be best to avoid introducing any TODOs in the code if possible.
If not possible, please explain thoroughly what doesn't work, what
should be done, create a ticket, and add a reference to the ticket to
the TODO, e.g.
TODO(gh-12345): ...
> + */
> +void
> +backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
> + void *cb_ctx)
Please make cb next to last argument, near cb_ctx:
backtrace_foreach_ip(ip_buf, limit, cb, cb_ctx)
> +{
> + int demangle_status;
> + char *demangle_buf = NULL;
> + size_t demangle_buf_len = 0;
> +#ifndef TARGET_OS_DARWIN
> + char proc_name[BACKTRACE_NAME_MAX];
> + unw_word_t ip = 0, offset = 0;
> + unw_proc_info_t pi;
> + int frame_no, ret = 0;
> + const char *proc = NULL;
> +
> + unw_accessors_t *acc = unw_get_accessors(unw_local_addr_space);
> +
> + /*
> + * RIPs collecting comes from inside a helper routine
> + * so we skip the collector function address itself thus
> + * start fetching functions with frame number = 1.
> + */
> + for (frame_no = 1; frame_no < limit && ip_buf[frame_no] != NULL;
Can we somehow not return the frame corresponding to
backtrace_collect_ip, since we don't need it anyway?
> + frame_no++) {
> + ip = (unw_word_t)ip_buf[frame_no];
> +
> + if (backtrace_proc_cache_find(ip, &proc, &offset) == 0) {
> + ret = 0;
> + } else if (acc->get_proc_name == NULL) {
> + ret = unw_get_proc_info_by_ip(unw_local_addr_space,
> + ip, &pi, NULL);
> + offset = ip - pi.start_ip;
> + proc = NULL;
> + backtrace_proc_cache_put(ip, proc, offset);
> + } else {
> + ret = acc->get_proc_name(unw_local_addr_space, ip,
> + proc_name, sizeof(proc_name),
> + &offset, NULL);
> + proc = proc_name;
> + backtrace_proc_cache_put(ip, proc, offset);
> + }
> +
> + if (proc != NULL) {
> + char *cxxname = abi::__cxa_demangle(proc, demangle_buf,
> + &demangle_buf_len,
> + &demangle_status);
> + if (cxxname != NULL) {
> + demangle_buf = cxxname;
> + proc = cxxname;
> + }
> + }
> + if (ret != 0 || cb(frame_no - 1, (void *)ip, proc,
> + (size_t)offset, cb_ctx) != 0)
> + break;
> + }
> +
> + free(demangle_buf);
> + if (ret != 0)
> + say_debug("unwinding error: %s", unw_strerror(ret));
> +#else
> + int frame_no, ret = 1;
> + void *ip = NULL;
> + size_t offset = 0;
> + Dl_info dli;
> + const char *proc = NULL;
> +
> + for (frame_no = 1; frame_no < limit && ip_buf[frame_no] != NULL;
> + ++frame_no) {
> + ip = ip_buf[frame_no];
> + if (backtrace_proc_cache_find((unw_word_t)ip, &proc,
> + &offset) == 0) {
> + ret = 1;
> + } else {
> + ret = dladdr(ip, &dli);
> + if (ret == 0)
> + break;
> + offset = (char *)ip - (char *)dli.dli_saddr;
> + proc = dli.dli_sname;
> + backtrace_proc_cache_put((unw_word_t)ip, proc, offset);
> + }
> +
> + if (proc != NULL) {
> + char *cxxname = abi::__cxa_demangle(proc, demangle_buf,
> + &demangle_buf_len,
> + &demangle_status);
> + if (cxxname != NULL) {
> + demangle_buf = cxxname;
> + proc = cxxname;
> + }
> + }
Looks like code duplication between #if TARGET_OS_DARWIN and #else
blocks. Is it possible to factor out only those parts that are actually
different between those two cases and put them in a separate function?
> + if (cb(frame_no - 1, ip, proc, offset, cb_ctx) != 0)
> + break;
> + }
> +
> + free(demangle_buf);
> + if (ret == 0)
> + say_debug("unwinding error: %i", ret);
> +#endif
> +}
> +
> void
> print_backtrace(void)
> {
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index 588b78504f38..f0fe6b893cd3 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -45,6 +45,11 @@
>
> extern void cord_on_yield(void);
>
> +#if ENABLE_BACKTRACE
> +#include "backtrace.h" /* fast_trace */
> +
> +#endif /* ENABLE_BACKTRACE */
> +
The #if/endif isn't needed here.
> #if ENABLE_FIBER_TOP
> #include <x86intrin.h> /* __rdtscp() */
>
> @@ -215,6 +220,10 @@ fiber_mprotect(void *addr, size_t len, int prot)
> static __thread bool fiber_top_enabled = false;
> #endif /* ENABLE_FIBER_TOP */
>
> +#if ENABLE_BACKTRACE
> +static __thread bool fiber_parent_bt_enabled = false;
> +#endif /* ENABLE_BACKTRACE */
> +
Why __thread?
> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
> index 38394666b41d..d82a3a20c132 100644
> --- a/src/lua/fiber.c
> +++ b/src/lua/fiber.c
> @@ -210,6 +226,69 @@ dump_lua_frame(struct lua_State *L, lua_Debug *ar, int tb_frame)
> lua_settable(L, -3);
> }
>
> +static void
> +lua_backtrace_foreach(struct lua_State *L, lua_backtrace_cb cb, void *cb_ctx)
This helper function could be introduced in a separate patch.
> @@ -220,38 +299,60 @@ fiber_backtrace_cb(int frameno, void *frameret, const char *func, size_t offset,
> * https://github.com/tarantool/tarantool/issues/5326
> * will not resolved, but is possible afterwards.
> */
> - if (func != NULL && strstr(func, "lj_BC_FUNCC") == func) {
> + if (func != NULL && tb_ctx->R && strstr(func, "lj_BC_FUNCC") == func) {
> /* We are in the LUA vm. */
> - lua_Debug ar;
> - while (tb_ctx->R && lua_getstack(tb_ctx->R, tb_ctx->lua_frame, &ar) > 0) {
> - /* Skip all following C-frames. */
> - lua_getinfo(tb_ctx->R, "Sln", &ar);
> - if (*ar.what != 'C')
> - break;
> - if (ar.name != NULL) {
> - /* Dump frame if it is a C built-in call. */
> - tb_ctx->tb_frame++;
> - dump_lua_frame(L, &ar, tb_ctx->tb_frame);
> - }
> - tb_ctx->lua_frame++;
> - }
> - while (tb_ctx->R && lua_getstack(tb_ctx->R, tb_ctx->lua_frame, &ar) > 0) {
> - /* Trace Lua frame. */
> - lua_getinfo(tb_ctx->R, "Sln", &ar);
> - if (*ar.what == 'C') {
> - break;
> - }
> + lua_backtrace_foreach(tb_ctx->R, dump_lua_frame_cb, tb_ctx);
> + }
> + char buf[512];
> + int l = snprintf(buf, sizeof(buf), "#%-2d %p in ",
> + frameno, frameret);
> + if (func)
> + snprintf(buf + l, sizeof(buf) - l, "%s+%zu", func, offset);
> + else
> + snprintf(buf + l, sizeof(buf) - l, "?");
> + buf[sizeof(buf) - 1] = 0;
> + tb_ctx->tb_frame++;
> + lua_pushnumber(L, tb_ctx->tb_frame);
> + lua_newtable(L);
> + lua_pushstring(L, "C");
> + lua_pushstring(L, buf);
> + lua_settable(L, -3);
> + lua_settable(L, -3);
Duplication with the code from fiber_backtrace_cb below. Should be
factored out to a separate helper function.
> + return 0;
> +}
> +
> +static int
> +fiber_parent_backtrace_cb(int frameno, void *frameret, const char *func,
> + size_t offset, void *cb_ctx)
> +{
> + int lua_frame = 0;
> + struct lua_parent_tb_ctx *tb_ctx = (struct lua_parent_tb_ctx *)cb_ctx;
> + struct parent_bt_lua *bt = tb_ctx->bt;
> + struct lua_State *L = tb_ctx->L;
> + /*
> + * There is impossible to get func == NULL until
> + * https://github.com/tarantool/tarantool/issues/5326
> + * will not resolved, but is possible afterwards.
> + */
> + if (bt != NULL && func != NULL && strstr(func, "lj_BC_FUNCC") == func) {
> + /* We are in the LUA vm. */
> + while (lua_frame < bt->cnt) {
> tb_ctx->tb_frame++;
> - dump_lua_frame(L, &ar, tb_ctx->tb_frame);
> - tb_ctx->lua_frame++;
> + dump_lua_frame(L, bt->names[lua_frame],
> + bt->sources[lua_frame],
> + bt->lines[lua_frame],
> + tb_ctx->tb_frame);
> + lua_frame++;
> }
> }
> char buf[512];
> - int l = snprintf(buf, sizeof(buf), "#%-2d %p in ", frameno, frameret);
> + int l = snprintf(buf, sizeof(buf), "#%-2d %p in ",
> + frameno, frameret);
> if (func)
> snprintf(buf + l, sizeof(buf) - l, "%s+%zu", func, offset);
> else
> snprintf(buf + l, sizeof(buf) - l, "?");
> + buf[sizeof(buf) - 1] = 0;
> tb_ctx->tb_frame++;
> lua_pushnumber(L, tb_ctx->tb_frame);
> lua_newtable(L);
> @@ -433,6 +569,22 @@ lbox_do_backtrace(struct lua_State *L)
> }
> return true;
> }
> +
> +static int
> +lbox_fiber_parent_bt_enable(struct lua_State *L)
> +{
> + (void) L;
> + fiber_parent_bt_enable();
> + return 0;
> +}
> +
> +static int
> +lbox_fiber_parent_bt_disable(struct lua_State *L)
> +{
> + (void) L;
> + fiber_parent_bt_disable();
> + return 0;
> +}
> #endif /* ENABLE_BACKTRACE */
Better have one function which would take true/false IMO, but I think we
have to wind up with two, for consistency with fiber_top_enable...
> @@ -498,6 +654,12 @@ fiber_create(struct lua_State *L)
> luaT_error(L);
> }
>
> +#ifdef ENABLE_BACKTRACE
> + // TODO: error handling
Please fix in this patch. Either with luaT_error() or with panic(),
which is okay, because malloc doesn't normally fail and if it does
there isn't much we can do.
> + if (fiber_parent_bt_is_enabled())
> + fiber_parent_bt_init(f, L);
> +#endif
> +
> diff --git a/src/lua/fiber.h b/src/lua/fiber.h
> index e298987062c5..450840fb045b 100644
> --- a/src/lua/fiber.h
> +++ b/src/lua/fiber.h
> @@ -36,6 +36,25 @@ extern "C" {
>
> struct lua_State;
>
> +/**
> + * Maximal name length (including '\0')
> + * and backtrace length.
> + */
> +enum {
> + PARENT_BT_LUA_NAME_MAX = 64,
> + PARENT_BT_LUA_LEN_MAX = 8
I think we should capture more frames, especially if we capture
backtraces of all ancestors. I think 64 would be an adequate limit.
> +};
> +
> +/**
> + * Stores lua parent backtrace for fiber.
> + */
> +struct parent_bt_lua {
> + int cnt;
> + char names[PARENT_BT_LUA_LEN_MAX][PARENT_BT_LUA_NAME_MAX];
> + char sources[PARENT_BT_LUA_LEN_MAX][PARENT_BT_LUA_NAME_MAX];
> + int lines[PARENT_BT_LUA_LEN_MAX];
> +};
Instead of having three arrays, better introduce a separate struct and
create one array of those structs.
^ permalink raw reply [flat|nested] 7+ messages in thread