GNOME Bugzilla – Bug 772555
debug: Implement gst_debug_print_stack_trace with libunwind/backtrace when avalaible
Last modified: 2016-11-04 17:27:36 UTC
Instead of only our own grown (weird?) implementation
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
Created attachment 337142 [details] [review] tracers: leaks: Use the new gst_debug_get_stack_trace And remove the local implementation of it.
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
Created attachment 337147 [details] [review] tracers: leaks: Use the new gst_debug_get_stack_trace And remove the local implementation of it.
Review of attachment 337147 [details] [review]: ++
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
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
Created attachment 337311 [details] [review] tracers: leaks: Use the new gst_debug_get_stack_trace And remove the local implementation of it.
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'
Erg, sorry I thought I had already tested with autotools.
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
Created attachment 337314 [details] [review] tracers: leaks: Use the new gst_debug_get_stack_trace And remove the local implementation of it.
(actually I tested autotools only inside pitivi flatpak env that does not have either of the 2 libs. I believe it should work now.)
Review of attachment 337314 [details] [review]: I think you can remove UNWIND_LIBS from plugins/tracers/Makefile.am
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
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
Created attachment 337335 [details] [review] tracers: leaks: Use the new gst_debug_get_stack_trace And remove the local implementation of it.
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.
Review of attachment 337335 [details] [review]: ++
Review of attachment 337334 [details] [review]: Works fine here and look good to me (but I'm not an official gst reviewer).
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 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.
(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 :)
I'm ok with removing it, it's just removing functionality on some platforms. Unlike the commit message says there is no full replacement :)
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
Created attachment 338122 [details] [review] tracers: leaks: Use the new gst_debug_get_stack_trace And remove the local implementation of it.
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.
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
Anything left here?
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.
(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.
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.
Sure, let's get it in, but let's also submit it to GLib please :)
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