[igt-dev] [PATCH i-g-t] lib/core: Use libdw to decode stack trace with debugging symbols, v2.

[Maarten Lankhorst]

Op 29-08-18 om 13:07 schreef Petri Latvala:
> On Wed, Aug 29, 2018 at 12:43:24PM +0200, Maarten Lankhorst wrote:
>> elfutils is LGPLv3 or GPLv2, so it's ok to link against it.
>>
>> Before:
>> IGT-Version: 1.23-g8ae86abd419d (x86_64) (Linux: 4.16.0-1-amd64 x86_64)
>> Starting subtest: fail-result
>> (meta_test:29661) CRITICAL: Test assertion failure function test_result, file ../tests/meta_test.c:94:
>> (meta_test:29661) CRITICAL: Failed assertion: result == 1
>> (meta_test:29661) CRITICAL: error: 0 != 1
>> Stack trace:
>>   #0 [__igt_fail_assert+0x20a]
>>   #1 [test_result+0x7a]
>>   #2 [__real_main120+0x240]
>>   #3 [main+0x4a]
>>   #4 (../csu/libc-start.c) __libc_start_main:344
>>   #5 [_start+0x2a]
>>
>> After:
>> IGT-Version: 1.23-g8ae86abd419d (x86_64) (Linux: 4.16.0-1-amd64 x86_64)
>> Starting subtest: fail-result
>> (meta_test:1357) CRITICAL: Test assertion failure function test_result, file ../tests/meta_test.c:94:
>> (meta_test:1357) CRITICAL: Failed assertion: result == 1
>> (meta_test:1357) CRITICAL: error: 0 != 1
>> Stack trace:
>>   #0 ../lib/igt_core.c:1467 __igt_fail_assert()
>>   #1 ../tests/meta_test.c:95 test_result()
>>   #2 ../tests/meta_test.c:137 __real_main120()
>>   #3 ../tests/meta_test.c:120 main()
>>   #4 ../csu/libc-start.c:344 __libc_start_main()
>>   #5 [_start+0x2a]
> These line numbers are off just a little bit (as discussed on
> IRC). It's not a consistent off-by-one, could be that the used IP is
> off by one instruction. Either way, we can brush that off as a TODO
> bug, having a bit-off line numbers in stack traces is so much better
> than not having them at all.
>
>
>
>> Changes since v1:
>> - Add libdw dependency to readme.
>> - Change backtrace format slightly.
>>
>> Cc: Petri Latvala <petri.latvala at intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  README          |  1 +
>>  configure.ac    |  1 +
>>  lib/Makefile.am |  2 ++
>>  lib/igt_core.c  | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
>>  lib/meson.build |  1 +
>>  meson.build     |  1 +
>>  6 files changed, 51 insertions(+), 5 deletions(-)
>>
>> diff --git a/README b/README
>> index c71ecedd0e5e..f8ba5c710e81 100644
>> --- a/README
>> +++ b/README
>> @@ -148,6 +148,7 @@ the default configuration (package names may vary):
>>  	libpciaccess-dev
>>  	libprocps-dev
>>  	libunwind-dev
>> +	libdw-dev
>>  	python-docutils
>>  	x11proto-dri2-dev
>>  	xutils-dev
>> diff --git a/configure.ac b/configure.ac
>> index 72f359943779..c75ef284f3bc 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -128,6 +128,7 @@ PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10])
>>  PKG_CHECK_MODULES(KMOD, [libkmod])
>>  PKG_CHECK_MODULES(PROCPS, [libprocps])
>>  PKG_CHECK_MODULES(LIBUNWIND, [libunwind])
>> +PKG_CHECK_MODULES(LIBDW, [libdw])
>>  PKG_CHECK_MODULES(SSL, [openssl])
>>  PKG_CHECK_MODULES(VALGRIND, [valgrind], [have_valgrind=yes], [have_valgrind=no])
>>  
>> diff --git a/lib/Makefile.am b/lib/Makefile.am
>> index 6251bdb85f67..388b8b00ae36 100644
>> --- a/lib/Makefile.am
>> +++ b/lib/Makefile.am
>> @@ -60,6 +60,7 @@ AM_CFLAGS = \
>>  	    $(DRM_CFLAGS) \
>>  	    $(PCIACCESS_CFLAGS) \
>>  	    $(LIBUNWIND_CFLAGS) \
>> +	    $(LIBDW_CFLAGS) \
>>  	    $(GSL_CFLAGS) \
>>  	    $(KMOD_CFLAGS) \
>>  	    $(PROCPS_CFLAGS) \
>> @@ -86,6 +87,7 @@ libintel_tools_la_LIBADD = \
>>  	$(CAIRO_LIBS) \
>>  	$(LIBUDEV_LIBS) \
>>  	$(LIBUNWIND_LIBS) \
>> +	$(LIBDW_LIBS) \
>>  	$(TIMER_LIBS) \
>>  	$(XMLRPC_LIBS) \
>>  	$(LIBUDEV_LIBS) \
>> diff --git a/lib/igt_core.c b/lib/igt_core.c
>> index c52c081899c9..4776db93594e 100644
>> --- a/lib/igt_core.c
>> +++ b/lib/igt_core.c
>> @@ -73,6 +73,7 @@
>>  
>>  #define UNW_LOCAL_ONLY
>>  #include <libunwind.h>
>> +#include <elfutils/libdwfl.h>
>>  
>>  #ifdef HAVE_LIBGEN_H
>>  #include <libgen.h>   /* for basename() on Solaris */
>> @@ -1212,20 +1213,59 @@ static void print_backtrace(void)
>>  	unw_context_t uc;
>>  	int stack_num = 0;
>>  
>> +	Dwfl_Callbacks cbs = {
>> +		.find_elf = dwfl_linux_proc_find_elf,
>> +		.find_debuginfo = dwfl_standard_find_debuginfo,
>> +	};
>> +
>> +	Dwfl *dwfl = dwfl_begin(&cbs);
>> +
>> +	if (dwfl_linux_proc_report(dwfl, getpid())) {
>> +		dwfl_end(dwfl);
>> +		dwfl = NULL;
>> +	} else
>> +		dwfl_report_end(dwfl, NULL, NULL);
>> +
>>  	igt_info("Stack trace:\n");
>>  
>>  	unw_getcontext(&uc);
>>  	unw_init_local(&cursor, &uc);
>>  	while (unw_step(&cursor) > 0) {
>>  		char name[255];
>> -		unw_word_t off;
>> +		unw_word_t off, ip;
>> +		Dwfl_Module *mod = NULL;
>>  
>> -		if (unw_get_proc_name(&cursor, name, 255, &off) < 0)
>> -			strcpy(name, "<unknown>");
>> +		unw_get_reg(&cursor, UNW_REG_IP, &ip);
>> +
>> +		if (dwfl)
>> +			mod = dwfl_addrmodule(dwfl, ip);
>>  
>> -		igt_info("  #%d [%s+0x%x]\n", stack_num++, name,
>> -			 (unsigned int) off);
>> +		if (mod) {
>> +			const char *src, *name;
> Shadowed variable 'name' here.
>
> With that fixed and an added note that libdw is a new mandatory
> dependency in the commit message, this is
>
>
> Reviewed-by: Petri Latvala <petri.latvala at intel.com>

Thanks, pushed. :)