Re: bugs in external debug info support in libbacktrace

[Ian Lance Taylor <iant@golang.org>]

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

On Mon, Nov 27, 2017 at 2:23 AM, Milian Wolff <milian.wolff@kdab.com> wrote:
>
> I was made aware that libbacktrace got support for external debug info with
> [1], great work! I have just synced the latest libbacktrace into heaptrack [2]
> in a local branch and played around with it and noticed two limitations:
>
> [1]: https://github.com/gcc-mirror/gcc/commit/
> b919941efc58035debbcf69b645c072b7dd6ba4e
> [2]: https://github.com/KDE/heaptrack
>
> a) elf_open_debugfile_by_debuglink checks the crc, even if it is not provided
> by the debug file. I.e. I have a file where `debuglink_crc == 0`, but the
> got_crc calculated from elf_crc32_file is non-zero. I have patched this
> locally with the following to make it work for me:
>
> diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c
> index 06823fcf59b..24bf58728fd 100644
> --- a/libbacktrace/elf.c
> +++ b/libbacktrace/elf.c
> @@ -1005,7 +1005,7 @@ elf_open_debugfile_by_debuglink (struct backtrace_state
> *state,
>    if (ddescriptor < 0)
>      return -1;
>
> -  got_crc = elf_crc32_file (state, ddescriptor, error_callback, data);
> +  got_crc = debuglink_crc ? elf_crc32_file (state, ddescriptor,
> error_callback, data) : 0;
>    if (got_crc != debuglink_crc)
>      {
>        backtrace_close (ddescriptor, error_callback, data);

Thanks.  I fixed this problem with a slightly different patch,
appended.  Committed to mainline.


> b) elf_add guards the code to inspect the symtab-shndx with a `&& !debuginfo`
> check in loc 2797. This results in all files with separate debug info yielding
> `found_sym = 0` when calling elf_add, and symbol resolution is broken.
> Personally I have patched this check out to make symbol resolution work for
> me:
>
> diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c
> index 06823fcf59b..6876bd3ed8e 100644
> --- a/libbacktrace/elf.c
> +++ b/libbacktrace/elf.c
> @@ -2794,7 +2794,7 @@ elf_add (struct backtrace_state *state, const char
> *filename, int descriptor,
>
>    if (symtab_shndx == 0)
>      symtab_shndx = dynsym_shndx;
> -  if (symtab_shndx != 0 && !debuginfo)
> +  if (symtab_shndx != 0)
>      {
>        const b_elf_shdr *symtab_shdr;
>        unsigned int strtab_shndx;
>
> Could you please check whether the two patches above could be upstreamed?

I don't think that's the right approach.  In the appended patch I skip
clearing *found_sym if debuginfo is set.  I hope that will fix the
problem.

Ian

2018-01-25  Ian Lance Taylor  <iant@golang.org>

* elf.c (elf_open_debugfile_by_debuglink): Don't check CRC if the
desired CRC is zero.
(elf_add): Don't clear *found_sym and *found_dwarf if debuginfo.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1223 bytes --]

Index: elf.c
===================================================================
--- elf.c	(revision 257038)
+++ elf.c	(working copy)
@@ -997,7 +997,6 @@ elf_open_debugfile_by_debuglink (struct
 				 void *data)
 {
   int ddescriptor;
-  uint32_t got_crc;
 
   ddescriptor = elf_find_debugfile_by_debuglink (state, filename,
 						 debuglink_name,
@@ -1005,11 +1004,16 @@ elf_open_debugfile_by_debuglink (struct
   if (ddescriptor < 0)
     return -1;
 
-  got_crc = elf_crc32_file (state, ddescriptor, error_callback, data);
-  if (got_crc != debuglink_crc)
+  if (debuglink_crc != 0)
     {
-      backtrace_close (ddescriptor, error_callback, data);
-      return -1;
+      uint32_t got_crc;
+
+      got_crc = elf_crc32_file (state, ddescriptor, error_callback, data);
+      if (got_crc != debuglink_crc)
+	{
+	  backtrace_close (ddescriptor, error_callback, data);
+	  return -1;
+	}
     }
 
   return ddescriptor;
@@ -2634,8 +2638,11 @@ elf_add (struct backtrace_state *state,
   unsigned int using_debug_view;
   uint16_t *zdebug_table;
 
-  *found_sym = 0;
-  *found_dwarf = 0;
+  if (!debuginfo)
+    {
+      *found_sym = 0;
+      *found_dwarf = 0;
+    }
 
   shdrs_view_valid = 0;
   names_view_valid = 0;