After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772555 - debug: Implement gst_debug_print_stack_trace with libunwind/backtrace when avalaible
debug: Implement gst_debug_print_stack_trace with libunwind/backtrace when av...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-10-07 10:08 UTC by Thibault Saunier
Modified: 2016-11-04 17:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gst: Use libunwind/libdw to generate backtraces if avalaible (7.99 KB, patch)
2016-10-07 10:08 UTC, Thibault Saunier
none Details | Review
tracers: leaks: Use the new gst_debug_get_stack_trace (3.28 KB, patch)
2016-10-07 10:09 UTC, Thibault Saunier
none Details | Review
gst: Use libunwind/libdw to generate backtraces if avalaible (9.45 KB, patch)
2016-10-07 10:21 UTC, Thibault Saunier
none Details | Review
tracers: leaks: Use the new gst_debug_get_stack_trace (3.28 KB, patch)
2016-10-07 10:21 UTC, Thibault Saunier
none Details | Review
gst: Use libunwind/libdw to generate backtraces if avalaible (9.94 KB, patch)
2016-10-10 13:31 UTC, Thibault Saunier
none Details | Review
tracers: leaks: Use the new gst_debug_get_stack_trace (3.28 KB, patch)
2016-10-10 13:31 UTC, Thibault Saunier
none Details | Review
gst: Use libunwind/libdw to generate backtraces if avalaible (10.61 KB, patch)
2016-10-10 13:47 UTC, Thibault Saunier
none Details | Review
tracers: leaks: Use the new gst_debug_get_stack_trace (3.28 KB, patch)
2016-10-10 13:47 UTC, Thibault Saunier
none Details | Review
gst: Use libunwind/libdw to generate backtraces if avalaible (10.77 KB, patch)
2016-10-10 14:28 UTC, Thibault Saunier
none Details | Review
gst: Use libunwind/libdw to generate backtraces if avalaible (10.80 KB, patch)
2016-10-10 15:37 UTC, Thibault Saunier
none Details | Review
tracers: leaks: Use the new gst_debug_get_stack_trace (3.80 KB, patch)
2016-10-10 15:37 UTC, Thibault Saunier
none Details | Review
debug: Remove the Gst only based stack trace printing implementation (3.47 KB, patch)
2016-10-10 15:37 UTC, Thibault Saunier
none Details | Review
gst: Use libunwind/libdw to generate backtraces if avalaible (11.80 KB, patch)
2016-10-20 18:36 UTC, Thibault Saunier
committed Details | Review
tracers: leaks: Use the new gst_debug_get_stack_trace (3.80 KB, patch)
2016-10-20 18:36 UTC, Thibault Saunier
committed Details | Review
debug: Remove the Gst only based stack trace printing implementation (3.41 KB, patch)
2016-10-20 18:37 UTC, Thibault Saunier
committed Details | Review

Description Thibault Saunier 2016-10-07 10:08:51 UTC
Instead of only our own grown (weird?) implementation
Comment 1 Thibault Saunier 2016-10-07 10:08:55 UTC
Created attachment 337141 [details] [review]
gst: Use libunwind/libdw to generate backtraces if avalaible

Making the gst_debug_print_trace function more generally useful.

API:
  + gst_debug_get_trace
Comment 2 Thibault Saunier 2016-10-07 10:09:00 UTC
Created attachment 337142 [details] [review]
tracers: leaks: Use the new gst_debug_get_stack_trace

And remove the local implementation of it.
Comment 3 Thibault Saunier 2016-10-07 10:21:24 UTC
Created attachment 337146 [details] [review]
gst: Use libunwind/libdw to generate backtraces if avalaible

Making the gst_debug_print_trace function more generally useful.

API:
  + gst_debug_get_trace
Comment 4 Thibault Saunier 2016-10-07 10:21:30 UTC
Created attachment 337147 [details] [review]
tracers: leaks: Use the new gst_debug_get_stack_trace

And remove the local implementation of it.
Comment 5 Guillaume Desmottes 2016-10-10 10:49:27 UTC
Review of attachment 337147 [details] [review]:

++
Comment 6 Guillaume Desmottes 2016-10-10 12:26:35 UTC
Review of attachment 337146 [details] [review]:

::: configure.ac
@@ -792,0 +792,8 @@
+if test "x$HAVE_UNWIND" = "xyes"; then
+  AC_DEFINE(HAVE_UNWIND, 1, [libunwind available])
+fi
... 5 more ...

Add it to the config summary displayed at the end of configure?

::: gst/gstinfo.c
@@ +2387,2 @@
  *
  * If GST_ENABLE_FUNC_INSTRUMENTATION is defined a stacktrace is available for

The doc is miss-leading as we have now an implementation working even if GST_ENABLE_FUNC_INSTRUMENTATION isn't defined.

@@ +2431,3 @@
+#ifdef HAVE_DW
+#include <elfutils/libdwfl.h>
+#include <stdlib.h>

It's already included above.

@@ +2441,3 @@
+    .find_elf = dwfl_linux_proc_find_elf,
+    .find_debuginfo = dwfl_standard_find_debuginfo,
+#include <assert.h>

We're not responsible of freeing this? (just double checking).

@@ +2445,3 @@
+
+  Dwfl *dwfl = dwfl_begin (&callbacks);
+#ifdef HAVE_DW

any reason to not use g_assert()?
I guess you just copy/paste this magic rune but may be worth making it a bit glib-ish.

@@ +2454,3 @@
+  Dwfl_Module *module = dwfl_addrmodule (dwfl, addr);
+
+{

Move var declarations at the start of the function?

@@ +2546,3 @@
+#endif /* HAVE_BACKTRACE */
+
+

Double \n
Comment 7 Thibault Saunier 2016-10-10 13:31:34 UTC
Created attachment 337310 [details] [review]
gst: Use libunwind/libdw to generate backtraces if avalaible

Making the gst_debug_print_trace function more generally useful.

API:
  + gst_debug_get_trace
Comment 8 Thibault Saunier 2016-10-10 13:31:42 UTC
Created attachment 337311 [details] [review]
tracers: leaks: Use the new gst_debug_get_stack_trace

And remove the local implementation of it.
Comment 9 Guillaume Desmottes 2016-10-10 13:34:58 UTC
Review of attachment 337146 [details] [review]:

I get linking errors when trying to build your branch. I guess we need to add DW_LIBS to gst's LIBADD.

.libs/libgstreamer-1.0.so: undefined reference to `dwfl_begin'
./.libs/libgstreamer-1.0.so: undefined reference to `_Ux86_64_getcontext'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_module_info'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_report_end'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_linux_proc_report'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_standard_find_debuginfo'
./.libs/libgstreamer-1.0.so: undefined reference to `_ULx86_64_get_reg'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_getsrc'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_module_addrname'
./.libs/libgstreamer-1.0.so: undefined reference to `_ULx86_64_step'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_lineinfo'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_addrmodule'
./.libs/libgstreamer-1.0.so: undefined reference to `_ULx86_64_init_local'
./.libs/libgstreamer-1.0.so: undefined reference to `dwfl_linux_proc_find_elf'
Comment 10 Thibault Saunier 2016-10-10 13:39:34 UTC
Erg, sorry I thought I had already tested with autotools.
Comment 11 Thibault Saunier 2016-10-10 13:47:49 UTC
Created attachment 337313 [details] [review]
gst: Use libunwind/libdw to generate backtraces if avalaible

Making the gst_debug_print_trace function more generally useful.

API:
  + gst_debug_get_trace
Comment 12 Thibault Saunier 2016-10-10 13:47:57 UTC
Created attachment 337314 [details] [review]
tracers: leaks: Use the new gst_debug_get_stack_trace

And remove the local implementation of it.
Comment 13 Thibault Saunier 2016-10-10 13:49:49 UTC
(actually I tested autotools only inside pitivi flatpak env that does not have either of the 2 libs. I believe it should work now.)
Comment 14 Guillaume Desmottes 2016-10-10 14:09:37 UTC
Review of attachment 337314 [details] [review]:

I think you can remove UNWIND_LIBS from plugins/tracers/Makefile.am
Comment 15 Thibault Saunier 2016-10-10 14:28:50 UTC
Created attachment 337320 [details] [review]
gst: Use libunwind/libdw to generate backtraces if avalaible

Making the gst_debug_print_trace function more generally useful.

API:
  + gst_debug_get_trace
Comment 16 Thibault Saunier 2016-10-10 15:37:42 UTC
Created attachment 337334 [details] [review]
gst: Use libunwind/libdw to generate backtraces if avalaible

Making the gst_debug_print_trace function more generally useful.

API:
  + gst_debug_get_trace
Comment 17 Thibault Saunier 2016-10-10 15:37:49 UTC
Created attachment 337335 [details] [review]
tracers: leaks: Use the new gst_debug_get_stack_trace

And remove the local implementation of it.
Comment 18 Thibault Saunier 2016-10-10 15:37:59 UTC
Created attachment 337336 [details] [review]
debug: Remove the Gst only based stack trace printing implementation

We now have 2 other implementations that should work better.
Comment 19 Guillaume Desmottes 2016-10-11 12:56:39 UTC
Review of attachment 337335 [details] [review]:

++
Comment 20 Guillaume Desmottes 2016-10-11 13:09:31 UTC
Review of attachment 337334 [details] [review]:

Works fine here and look good to me (but I'm not an official gst reviewer).
Comment 21 Sebastian Dröge (slomo) 2016-10-20 10:36:19 UTC
Review of attachment 337334 [details] [review]:

::: gst/gstinfo.c
@@ +2423,3 @@
+#include <stdarg.h>
+#include <unistd.h>
+#include <errno.h>

Including things in the middle of the file between other functions often causes problems. Better put this at the top in a separate #ifdef

@@ +2439,3 @@
+    .find_elf = dwfl_linux_proc_find_elf,
+    .find_debuginfo = dwfl_standard_find_debuginfo,
+    .debuginfo_path = &debuginfo_path,

Isn't this C99?

@@ +2465,3 @@
+        &nline, NULL, NULL, NULL);
+
+    g_string_append_printf (trace, "%s:%d", strrchr (filename, '/') + 1, nline);

Instead of '/', use the path separator #define from GLib

::: meson.build
@@ +243,3 @@
+else
+  if cc.has_function('backtrace')
+    cdata.set('HAVE_BACKTRACE', 1)

In meson you have either BACKTRACE or UNWIND, in autotools you can have both at once
Comment 22 Sebastian Dröge (slomo) 2016-10-20 10:37:30 UTC
Comment on attachment 337336 [details] [review]
debug: Remove the Gst only based stack trace printing implementation

... except that this one would also work on Windows with mingw/cygwin. And the new one only works where libunwind/libdw is supported.
Comment 23 Thibault Saunier 2016-10-20 11:43:23 UTC
(In reply to Sebastian Dröge (slomo) from comment #22)
> Comment on attachment 337336 [details] [review] [review]
> debug: Remove the Gst only based stack trace printing implementation
> 
> ... except that this one would also work on Windows with mingw/cygwin. And
> the new one only works where libunwind/libdw is supported.

This also work whenever built against the glibc (using the backtrace () glibc 'extension')

I do not care about removing that imple, just have the impression it is not really a useful one and am not convinced anyone ever use it, but might be wrong :)
Comment 24 Sebastian Dröge (slomo) 2016-10-20 12:10:41 UTC
I'm ok with removing it, it's just removing functionality on some platforms. Unlike the commit message says there is no full replacement :)
Comment 25 Thibault Saunier 2016-10-20 18:36:45 UTC
Created attachment 338121 [details] [review]
gst: Use libunwind/libdw to generate backtraces if avalaible

Making the gst_debug_print_trace function more generally useful.

API:
  + gst_debug_get_trace
Comment 26 Thibault Saunier 2016-10-20 18:36:53 UTC
Created attachment 338122 [details] [review]
tracers: leaks: Use the new gst_debug_get_stack_trace

And remove the local implementation of it.
Comment 27 Thibault Saunier 2016-10-20 18:37:00 UTC
Created attachment 338123 [details] [review]
debug: Remove the Gst only based stack trace printing implementation

We now have 2 other implementations that should work better.
Comment 28 Thibault Saunier 2016-10-20 18:37:01 UTC
Review of attachment 337334 [details] [review]:

::: gst/gstinfo.c
@@ +2439,3 @@
+    .find_elf = dwfl_linux_proc_find_elf,
+    .find_debuginfo = dwfl_standard_find_debuginfo,
+#ifdef HAVE_DW

Seems to be C89
Comment 29 Thibault Saunier 2016-11-03 15:42:45 UTC
Anything left here?
Comment 30 Tim-Philipp Müller 2016-11-03 15:54:20 UTC
I wonder if this shouldn't really go into glib instead? :)

And if it would make sense to return the stack trace as array one per line rather than single string? But I guess one usually just wants to print it, so I guess it's fine and people can always split it if needed.
Comment 31 Thibault Saunier 2016-11-03 15:57:12 UTC
(In reply to Tim-Philipp Müller from comment #30)
> I wonder if this shouldn't really go into glib instead? :)

Yeah as many other things, I agree! I could push forward on it maybe, but I am not really adding any API here, just changing the implementation to make it more generally useful.
Comment 32 Sebastian Dröge (slomo) 2016-11-04 15:06:42 UTC
IMHO we should add this to make our existing API more useful, and additionally get something like this as API added to GLib for the future.
Comment 33 Tim-Philipp Müller 2016-11-04 15:27:20 UTC
Sure, let's get it in, but let's also submit it to GLib please :)
Comment 34 Thibault Saunier 2016-11-04 17:26:01 UTC
Attachment 338121 [details] pushed as a8d4857 - gst: Use libunwind/libdw to generate backtraces if avalaible
Attachment 338122 [details] pushed as 85179a6 - tracers: leaks: Use the new gst_debug_get_stack_trace
Attachment 338123 [details] pushed as e6c8c53 - debug: Remove the Gst only based stack trace printing implementation