[Tarantool-patches] [PATCH 0/2] [draft] fiber: introduce fiber creation backtrace

[Tarantool-patches] [PATCH 0/2] [draft] fiber: introduce fiber creation backtrace

[Egor Elchinov via Tarantool-patches]

From: Egor2001 <elchinov.es@gmail.com>

This patchset is a proof-of-concept for the fiber creation backtrace.

For testing convenience, parent backtrace is 
temporarily moved to the separate lua subtable
backtrace_parent of fiber.info.

For now only C backtrace is implemented.
For the Lua parts of backtrace parent lua state 
needs to be somehow introduced to the new fiber.
Proper solution to this issue needs to be discussed.

Issue: https://github.com/tarantool/tarantool/issues/4002
PR draft: https://github.com/tarantool/tarantool/pull/6124

Egor2001 (2):
  fiber: add PoC for fiber creation backtrace
  fiber: fix DARWIN build

 src/lib/core/backtrace.cc                     | 94 ++++++++++++++++++-
 src/lib/core/backtrace.h                      |  8 +-
 src/lib/core/crash.c                          |  2 +-
 src/lib/core/fiber.c                          |  8 ++
 src/lib/core/fiber.h                          | 16 ++++
 src/lua/fiber.c                               |  9 ++
 .../gh-4002-fiber-creation-backtrace.result   | 55 +++++++++++
 .../gh-4002-fiber-creation-backtrace.test.lua | 25 +++++
 8 files changed, 213 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

-- 
2.31.1

[Tarantool-patches] [PATCH 1/2] fiber: add PoC for fiber creation backtrace

[Egor Elchinov via Tarantool-patches]

From: Egor2001 <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.

Needed for: #4002
---
 src/lib/core/backtrace.cc                     | 66 +++++++++++++++++++
 src/lib/core/backtrace.h                      |  6 ++
 src/lib/core/fiber.c                          |  8 +++
 src/lib/core/fiber.h                          | 16 +++++
 src/lua/fiber.c                               |  9 +++
 .../gh-4002-fiber-creation-backtrace.result   | 55 ++++++++++++++++
 .../gh-4002-fiber-creation-backtrace.test.lua | 25 +++++++
 7 files changed, 185 insertions(+)
 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..276933ad6 100644
--- a/src/lib/core/backtrace.cc
+++ b/src/lib/core/backtrace.cc
@@ -432,6 +432,72 @@ 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
+fast_trace_collect(void **ip_buf, int limit)
+{
+	memset(ip_buf, 0, limit * sizeof(*ip_buf));
+	unw_backtrace(ip_buf, limit);
+}
+
+/**
+ * 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
+fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
+{
+	static __thread char proc_name[BACKTRACE_NAME_MAX];
+	int frame_no = 0;
+	unw_word_t ip = 0, offset = 0;
+	unw_proc_info_t pi;
+	int ret = 0;
+	char* proc = NULL;
+
+	unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
+	assert(acc);
+
+	for (frame_no = 0; 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 || (frame_no > 0 &&
+		    cb(frame_no - 1, (void *)ip, proc,
+	 	       (size_t)offset, cb_ctx) != 0))
+			break;
+	}
+
+#ifndef TARGET_OS_DARWIN
+	if (ret != 0)
+		say_debug("unwinding error: %s", unw_strerror(ret));
+#else
+	if (ret != 0)
+		say_debug("unwinding error: %i", ret);
+#endif
+}
+
 void
 print_backtrace(void)
 {
diff --git a/src/lib/core/backtrace.h b/src/lib/core/backtrace.h
index e0ae56be4..1560f03e3 100644
--- a/src/lib/core/backtrace.h
+++ b/src/lib/core/backtrace.h
@@ -55,6 +55,12 @@ backtrace_foreach(backtrace_cb cb, coro_context *coro_ctx, void *cb_ctx);
 void
 backtrace_proc_cache_clear(void);
 
+void
+fast_trace_collect(void **ip_buf, int limit);
+
+void
+fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx);
+
 #endif /* ENABLE_BACKTRACE */
 
 #if defined(__cplusplus)
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 759c7da6a..73500e043 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
+	fast_trace_collect(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..f5fa31443 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 = 10
+};
+
+#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..dc7051edd 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -308,6 +308,15 @@ 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);
+		fast_trace_foreach(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..185aec585
--- /dev/null
+++ b/test/app/gh-4002-fiber-creation-backtrace.result
@@ -0,0 +1,55 @@
+-- test-run result file version 2
+yaml = require('yaml')
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+stack_len = 0
+ | ---
+ | ...
+parent_stack_len = 0
+ | ---
+ | ...
+
+test_run:cmd('setopt delimiter ";"')
+ | ---
+ | - true
+ | ...
+function foo()
+    local fiber_info = fiber.info()
+    local fiber_id = fiber.self():id()
+    local parent_stack = fiber_info[fiber_id].backtrace_parent
+    stack_len = stack and #stack or 0
+    parent_stack_len = parent_stack and #parent_stack or 0
+end;
+ | ---
+ | ...
+
+function bar()
+    fiber.create(foo)
+end;
+ | ---
+ | ...
+test_run:cmd('setopt delimiter ""');
+ | ---
+ | - true
+ | ...
+
+bar()
+ | ---
+ | ...
+ -- ... -> fiber.create() -> fiber.new() -> fiber.new_ex() 
+ | ---
+ | ...
+ -- or backtrace is disabled
+ | ---
+ | ...
+parent_stack_len > 3 or stack_len == 0
+ | ---
+ | - 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..c58308eda
--- /dev/null
+++ b/test/app/gh-4002-fiber-creation-backtrace.test.lua
@@ -0,0 +1,25 @@
+yaml = require('yaml')
+fiber = require('fiber')
+test_run = require('test_run').new()
+
+stack_len = 0
+parent_stack_len = 0
+
+test_run:cmd('setopt delimiter ";"')
+function foo()
+    local fiber_info = fiber.info()
+    local fiber_id = fiber.self():id()
+    local parent_stack = fiber_info[fiber_id].backtrace_parent
+    stack_len = stack and #stack or 0
+    parent_stack_len = parent_stack and #parent_stack or 0
+end;
+
+function bar()
+    fiber.create(foo)
+end;
+test_run:cmd('setopt delimiter ""');
+
+bar()
+ -- ... -> fiber.create() -> fiber.new() -> fiber.new_ex() 
+ -- or backtrace is disabled
+parent_stack_len > 3 or stack_len == 0
-- 
2.31.1

[Tarantool-patches] [PATCH 2/2] fiber: fix DARWIN build

[Egor Elchinov via Tarantool-patches]

From: Egor2001 <elchinov.es@gmail.com>

Due to the limited libunwind API under osx
this fix 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 | 38 +++++++++++++++++++++++++++++++-------
 src/lib/core/backtrace.h  |  2 +-
 src/lib/core/crash.c      |  2 +-
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc
index 276933ad6..a1727374c 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;
@@ -442,7 +447,11 @@ void
 fast_trace_collect(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
 }
 
 /**
@@ -459,12 +468,12 @@ fast_trace_collect(void **ip_buf, int limit)
 void
 fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
 {
+#ifndef TARGET_OS_DARWIN
 	static __thread char proc_name[BACKTRACE_NAME_MAX];
-	int frame_no = 0;
+	char* proc = NULL;
+	int frame_no = 0, ret = 0;
 	unw_word_t ip = 0, offset = 0;
 	unw_proc_info_t pi;
-	int ret = 0;
-	char* proc = NULL;
 
 	unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
 	assert(acc);
@@ -489,11 +498,26 @@ fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
 			break;
 	}
 
-#ifndef TARGET_OS_DARWIN
 	if (ret != 0)
 		say_debug("unwinding error: %s", unw_strerror(ret));
 #else
-	if (ret != 0)
+	int frame_no = 0, ret = 1;
+	void *ip = NULL;
+	size_t offset = 0;
+	Dl_info dli;
+
+	for (frame_no = 0; frame_no < limit && ip_buf[frame_no] != NULL;
+	     ++frame_no) {
+		ip = ip_buf[frame_no];
+		ret = dladdr(ip, &dli);
+		offset = (char *)ip - (char *)dli.dli_saddr;
+
+		if (frame_no > 0 &&
+		    cb(frame_no - 1, ip, dli.dli_sname, offset, cb_ctx) != 0)
+			break;
+	}
+
+	if (ret == 0)
 		say_debug("unwinding error: %i", ret);
 #endif
 }
@@ -502,7 +526,7 @@ 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 1560f03e3..8bb668366 100644
--- a/src/lib/core/backtrace.h
+++ b/src/lib/core/backtrace.h
@@ -41,7 +41,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);
 
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
-- 
2.31.1

Re: [Tarantool-patches] [PATCH 1/2] fiber: add PoC for fiber creation backtrace

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Jun 04, 2021 at 02:13:10PM +0300, Egor Elchinov via Tarantool-patches wrote:
> From: Egor2001 <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.
> 
> Needed for: #4002
> ---
>  
> +/**
> + * 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
> +fast_trace_collect(void **ip_buf, int limit)
> +{
> +	memset(ip_buf, 0, limit * sizeof(*ip_buf));
> +	unw_backtrace(ip_buf, limit);
> +}

This is not guaranteed to be faster, so I would name it
backtrace_collect_ip. Also you have to mark it as NOINLINE,
otherwise IPs gonna be screwed. Moreover you put a special
offset into IPs resolving inside fast_trace_foreach to hide
the fast_trace_collect call itself, I think we should be
very loud about, otherwise other people might get confusing
what we're doing with frame numbers and why we skip the first
frame.

Also I personally not sure if we must collect fiber's creation
backtrace for every fiber in a system even if we never need it,
I'm pretty sure that backtrace is very far from cheap. But I
left it up to you to decide. I guess we might need some kind
of dynamic settings similar to fiber_top?

> +void
> +fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
> +{
> +	static __thread char proc_name[BACKTRACE_NAME_MAX];

Why do you need it be per thread? There must be very strong reason why some
data is put into TLS.

> +	int frame_no = 0;

This is shor routine and I think plain `n` would be more than enough.

> +	unw_word_t ip = 0, offset = 0;
> +	unw_proc_info_t pi;
> +	int ret = 0;
> +	char* proc = NULL;
> +
> +	unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
> +	assert(acc);
> +
> +	for (frame_no = 0; 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;

Why proc is left untouched here? it may carry old value from
previous acc->get_proc_name call, is it ok?

> +		} else {
> +			ret = acc->get_proc_name(unw_local_addr_space, ip,
> +			    			 proc_name, sizeof(proc_name),
> +			    			 &offset, NULL);
> +			proc = proc_name;
> +		}
> +
> +		if (ret != 0 || (frame_no > 0 &&
> +		    cb(frame_no - 1, (void *)ip, proc,
> +	 	       (size_t)offset, cb_ctx) != 0))
> +			break;
> +	}

Egor, I think this is very good PoC code! Here is the diff I made
on top of your patch to share ideas. Not insisting anyhow on code
refactoring, vars renaming and etc.

Also I CC Vlad, since he might have own vision on overall code design.
And since Vlad is the final line before code goes upstream better wait
for his opinion.
---
Index: tarantool.git/src/lib/core/backtrace.cc
===================================================================
--- tarantool.git.orig/src/lib/core/backtrace.cc
+++ tarantool.git/src/lib/core/backtrace.cc
@@ -438,8 +438,8 @@ out:
  * Must be by far faster than usual backtrace according to the
  * libunwind doc for unw_backtrace().
  */
-void
-fast_trace_collect(void **ip_buf, int limit)
+void NOINLINE
+backtrace_collect_ip(void **ip_buf, int limit)
 {
 	memset(ip_buf, 0, limit * sizeof(*ip_buf));
 	unw_backtrace(ip_buf, limit);
@@ -457,35 +457,38 @@ fast_trace_collect(void **ip_buf, int li
  * TODO: to add cache and demangling support
  */
 void
-fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
+backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
+		     void *cb_ctx)
 {
-	static __thread char proc_name[BACKTRACE_NAME_MAX];
-	int frame_no = 0;
+	char proc_name[BACKTRACE_NAME_MAX];
 	unw_word_t ip = 0, offset = 0;
 	unw_proc_info_t pi;
-	int ret = 0;
-	char* proc = NULL;
+	int ret, n;
+	char *proc;
+
+	unw_accessors_t *acc = unw_get_accessors(unw_local_addr_space);
 
-	unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
-	assert(acc);
+	/*
+	 * 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 (n = 1; n < limit && ip_buf[n] != NULL; n++) {
+		ip = (unw_word_t)ip_buf[n];
 
-	for (frame_no = 0; 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;
+			proc = NULL;
 		} else {
 			ret = acc->get_proc_name(unw_local_addr_space, ip,
-			    			 proc_name, sizeof(proc_name),
-			    			 &offset, NULL);
+						 proc_name, sizeof(proc_name),
+						 &offset, NULL);
 			proc = proc_name;
 		}
-
-		if (ret != 0 || (frame_no > 0 &&
-		    cb(frame_no - 1, (void *)ip, proc,
-	 	       (size_t)offset, cb_ctx) != 0))
+		if (ret != 0 || cb(n - 1, (void *)ip, proc,
+				   (size_t)offset, cb_ctx) != 0)
 			break;
 	}
 
Index: tarantool.git/src/lua/fiber.c
===================================================================
--- tarantool.git.orig/src/lua/fiber.c
+++ tarantool.git/src/lua/fiber.c
@@ -314,8 +314,9 @@ lbox_fiber_statof_map(struct fiber *f, v
 		tb_ctx.R = NULL;
 		lua_pushstring(L, "backtrace_parent");
 		lua_newtable(L);
-		fast_trace_foreach(fiber_backtrace_cb, f->parent_bt_ip_buf,
-		     		   FIBER_PARENT_BT_MAX, &tb_ctx);
+		backtrace_foreach_ip(fiber_backtrace_cb,
+				     f->parent_bt_ip_buf,
+				     FIBER_PARENT_BT_MAX, &tb_ctx);
 		lua_settable(L, -3);
 #endif /* ENABLE_BACKTRACE */
 	}
Index: tarantool.git/src/lib/core/fiber.h
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.h
+++ tarantool.git/src/lib/core/fiber.h
@@ -654,7 +654,7 @@ struct fiber {
 	char inline_name[FIBER_NAME_INLINE];
 #if ENABLE_BACKTRACE
 	/** Fiber creation backtrace chunk. */
-	void* parent_bt_ip_buf[FIBER_PARENT_BT_MAX];
+	void *parent_bt_ip_buf[FIBER_PARENT_BT_MAX];
 #endif /* ENABLE_BACKTRACE */
 };
 
Index: tarantool.git/src/lib/core/fiber.c
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.c
+++ tarantool.git/src/lib/core/fiber.c
@@ -1265,7 +1265,7 @@ fiber_new_ex(const char *name, const str
 	fiber->fid = cord->next_fid;
 	fiber_set_name(fiber, name);
 #if ENABLE_BACKTRACE
-	fast_trace_collect(fiber->parent_bt_ip_buf, FIBER_PARENT_BT_MAX);
+	backtrace_collect_ip(fiber->parent_bt_ip_buf, FIBER_PARENT_BT_MAX);
 #endif /* ENABLE_BACKTRACE */
 	register_fid(fiber);
 	fiber->csw = 0;
Index: tarantool.git/src/lib/core/backtrace.h
===================================================================
--- tarantool.git.orig/src/lib/core/backtrace.h
+++ tarantool.git/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)
@@ -55,11 +56,12 @@ backtrace_foreach(backtrace_cb cb, coro_
 void
 backtrace_proc_cache_clear(void);
 
-void
-fast_trace_collect(void **ip_buf, int limit);
+void NOINLINE
+backtrace_collect_ip(void **ip_buf, int limit);
 
 void
-fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx);
+backtrace_foreach_ip(backtrace_cb cb, void **ip_buf, int limit,
+		     void *cb_ctx);
 
 #endif /* ENABLE_BACKTRACE */
 

Re: [Tarantool-patches] [PATCH 1/2] fiber: add PoC for fiber creation backtrace

[Egor Elchinov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4174 bytes --]

Hi! Thanks for the review!

On 6/7/21 11:28 AM, Cyrill Gorcunov wrote:

> On Fri, Jun 04, 2021 at 02:13:10PM +0300, Egor Elchinov via Tarantool-patches wrote:
>> From: Egor2001 <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.
>>
>> Needed for: #4002
>> ---
>>   
>> +/**
>> + * 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
>> +fast_trace_collect(void **ip_buf, int limit)
>> +{
>> +	memset(ip_buf, 0, limit * sizeof(*ip_buf));
>> +	unw_backtrace(ip_buf, limit);
>> +}
> This is not guaranteed to be faster, so I would name it
> backtrace_collect_ip. Also you have to mark it as NOINLINE,
> otherwise IPs gonna be screwed. Moreover you put a special
> offset into IPs resolving inside fast_trace_foreach to hide
> the fast_trace_collect call itself, I think we should be
> very loud about, otherwise other people might get confusing
> what we're doing with frame numbers and why we skip the first
> frame.

Thanks, fixed.

> Also I personally not sure if we must collect fiber's creation
> backtrace for every fiber in a system even if we never need it,
> I'm pretty sure that backtrace is very far from cheap. But I
> left it up to you to decide. I guess we might need some kind
> of dynamic settings similar to fiber_top?

Good point, I've consulted with Alexander Lyapunov and
it was decided to add a dynamic setting enabling this feature.

>> +void
>> +fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
>> +{
>> +	static __thread char proc_name[BACKTRACE_NAME_MAX];
> Why do you need it be per thread? There must be very strong reason why some
> data is put into TLS.

Thanks, fixed.

>> +	int frame_no = 0;
> This is shor routine and I think plain `n` would be more than enough.

For now that's true, but `frame_no` name was chosen mostly
for consistency with the present `backtrace` routines.
Also, routine will be more complicated after addition of
DARWIN `#ifdef` and demangling support.

>> +	unw_word_t ip = 0, offset = 0;
>> +	unw_proc_info_t pi;
>> +	int ret = 0;
>> +	char* proc = NULL;
>> +
>> +	unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
>> +	assert(acc);
>> +
>> +	for (frame_no = 0; 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;
> Why proc is left untouched here? it may carry old value from
> previous acc->get_proc_name call, is it ok?

It's ok, because if `proc` gets non-NULL value then
`acc->get_proc_name` routine is present in the system and
this part of code will never be called.
Otherwise `proc` can't be retrieved by the
libunwind API and has to be NULL.

>> +		} else {
>> +			ret = acc->get_proc_name(unw_local_addr_space, ip,
>> +			    			 proc_name, sizeof(proc_name),
>> +			    			 &offset, NULL);
>> +			proc = proc_name;
>> +		}
>> +
>> +		if (ret != 0 || (frame_no > 0 &&
>> +		    cb(frame_no - 1, (void *)ip, proc,
>> +	 	       (size_t)offset, cb_ctx) != 0))
>> +			break;
>> +	}
> Egor, I think this is very good PoC code! Here is the diff I made
> on top of your patch to share ideas. Not insisting anyhow on code
> refactoring, vars renaming and etc.
>
> Also I CC Vlad, since he might have own vision on overall code design.
> And since Vlad is the final line before code goes upstream better wait
> for his opinion.

Thanks a lot for your diff!
Also I haven't decided yet how to grab Lua stacks of
fiber creation in a cheap way.
I would be glad for any suggestions on this issue too.


[-- Attachment #2: Type: text/html, Size: 6060 bytes --]