[libunwind] dwarf fixes: searching the table in eh_frame_hdr

[Max Asbock]

I split up the changes to dwarf into pieces and will start with
Gfind_proc_info-lsb.c where the main issue is the binary search table in
the DWARF eh_frame_hdr section and its encoding.

On Tue, 2004-06-08 at 16:49, David Mosberger wrote:
> Hi Max,
> 
> Here is some feedback on the x86-64 patch for libunwind:
> 
> Could you split the patch into DWARF-fixes and x86-64-enablement?
> 
>  - Gfind_proc_info-lsb.c:
> 
>     o This looks wrong to me:
> 
> -    unw_word_t start_ip_offset;
> -    unw_word_t fde_offset;
> +    int32_t start_ip;
> +    int32_t fde_addr;
> 
      DWARF3 allows for text > 4GB and I'd like to avoid introducing
>       artificial 32-bit limits into libunwind.  There are several
>       related changes in Gfind_proc_info-lsb.c which probably
>       shouldn't be there either.  Perhaps the real issue is
>       signedness?
> 
>     o Actually, I don't understand the motivation for most of the
>       changes to this file.  Can you break them up into individual
>       patches so we can discuss them group by group?
> 

The int32_t was introduced here because the actual values in the tables
for the two architectures (x86 and x86_64) we ran this code are 32bit
signed integers. This is to be seen more like a "make it work quickly"
effort. However I agree that generally this not the right way to
approach this.
The EH Frame header is defined in:
http://www.linuxbase.org/spec/refspecs/LSB_1.3.0/gLSB/gLSB/ehframehdr.html
The encoding of the binary search table entries is defined by the
table_enc entry in the dwarf_eh_frame_hdr struct. 
table_enc for both x86 and x86-64 happens to be 
DW_EH_PE_sdata4 | DW_EH_PE_datarel
where data relative means relative to the beginning of the .eh_frame_hdr
section. That means unw_word_t does not work here.
I think we really should use the dwarf_read_encoded_pointer function to
search the binary table.

Here is the full patch to Gfind_proc_info-lsb.c with annotations:
(actually the comments stop at some point. I will add more later)

diff -urN -x Makefile.in -x configure 
libunwind-0.97/src/dwarf/Gfind_proc_info-lsb.c 
libunwind-0.97-amd64/src/dwarf/Gfind_proc_info-lsb.c
--- libunwind-0.97/src/dwarf/Gfind_proc_info-lsb.c      2004-05-06 
10:52:39.000000000 -0700
+++ libunwind-0.97-amd64/src/dwarf/Gfind_proc_info-lsb.c        2004-05-10 
23:41:44.000000000 -0700
@@ -32,12 +32,13 @@
 #include <string.h>
 
>>> Include dwarf_i.h because we are using dwarf_reads32 instead of access_mem
>>> (we really should use dwarf_read_encoded_pointer instead)

 #include "dwarf-eh.h"
+#include "dwarf_i.h"
 #include "tdep.h"
 
>>> for the following see discussion about table encoding above.
 struct table_entry
   {
-    unw_word_t start_ip_offset;
-    unw_word_t fde_offset;
+    int32_t start_ip;
+    int32_t fde_addr;
   };
 
 #ifndef UNW_REMOTE_ONLY
@@ -147,13 +148,11 @@
             info->dlpi_name);
       return 0;
     }
+

>>> The following changes are also related to the table encoding issue.
>>> DW_EH_PE_datarel | DW_EH_PE_sdata4 is the table encoding used with
>>> x86 and x86-64.   
/* For now, only support binary-search tables which are
-     data-relative and whose entries have the size of a pointer.  */
-  if (!(hdr->table_enc == (DW_EH_PE_datarel | DW_EH_PE_ptr)
-       || ((sizeof (unw_word_t) == 4
-            && hdr->table_enc == (DW_EH_PE_datarel | DW_EH_PE_sdata4)))
-       || ((sizeof (unw_word_t) == 8
-            && hdr->table_enc != (DW_EH_PE_datarel | DW_EH_PE_sdata8)))))
+     data-relative and whose entries have the size of 4 bytes.
+     This is currently the case for i386 and x86-64. */
+  if (!hdr->table_enc == (DW_EH_PE_datarel | DW_EH_PE_sdata4))
     {
       Debug (1, "search table in `%s' has unexpected encoding 0x%x\n",
             info->dlpi_name, hdr->table_enc);
@@ -181,7 +180,7 @@
   di->end_ip = p_text->p_vaddr + load_base + p_text->p_memsz;
   di->u.rti.name_ptr = (unw_word_t) info->dlpi_name;
   di->u.rti.table_data = addr;

>>> Adding 2*sizeof unw_word_t looks wrong because table_len
>>> is the number of entries in the table not the number of bytes.
>>> But why add anything to the table length? What am I missing?
-  di->u.rti.table_len = fde_count + 2 * sizeof (unw_word_t);
+  di->u.rti.table_len = fde_count;

   /* For the binary-search table in the eh_frame_hdr, data-relative
      means relative to the start of that section... */
   di->u.rti.segbase = (unw_word_t) hdr;
@@ -190,6 +189,8 @@
         "table_data=0x%lx\n", (char *) di->u.rti.name_ptr,
         (long) di->u.rti.segbase, (long) di->u.rti.table_len,
         (long) di->gp, (long) di->u.rti.table_data);
+
+
   return 1;
 }
 
@@ -219,45 +220,40 @@
 }
 
 static inline const struct table_entry *
-lookup (struct table_entry *table, size_t table_size, unw_word_t rel_ip)
+lookup (struct table_entry *table, size_t table_len, int32_t rel_ip)
 {
-  unsigned long table_len = table_size / sizeof (struct table_entry);
   const struct table_entry *e = 0;
   unsigned long lo, hi, mid;
-  unw_word_t end = 0;
+
+  if (!table_len)
+    return NULL;
 
   /* do a binary search for right entry: */
   for (lo = 0, hi = table_len; lo < hi;)
     {
       mid = (lo + hi) / 2;
       e = table + mid;
-      if (rel_ip < e->start_ip_offset)
+      if (rel_ip < e->start_ip)
        hi = mid;
       else
        {
          if (mid + 1 >= table_len)
            break;
 
-         end = table[mid + 1].start_ip_offset;
-
-         if (rel_ip >= end)
+          if (rel_ip >= table[mid + 1].start_ip)
            lo = mid + 1;
          else
            break;
        }
     }
-  if (rel_ip < e->start_ip_offset || rel_ip >= end)
+  if (rel_ip < e->start_ip)
     return NULL;
+
   return e;
 }
 
 #endif /* !UNW_REMOTE_ONLY */
 
-/* Helper macro for reading an table_entry from remote memory.  */
-#define remote_read(addr, member)                                         \
-       (*a->access_mem) (as, (addr) + offsetof (struct table_entry,       \
-                                                member), &member, 0, arg)
-
 #ifndef UNW_LOCAL_ONLY
 
 /* Lookup an unwind-table entry in remote memory.  Returns 1 if an
@@ -265,36 +261,39 @@
    occurred reading remote memory.  */
 static int
 remote_lookup (unw_addr_space_t as,
-              unw_word_t table, size_t table_size, unw_word_t rel_ip,
-              struct table_entry *e, void *arg)
+              unw_word_t table_addr, size_t table_len, int32_t rel_ip,
+              struct table_entry *entry, void *arg)
 {
-  unsigned long table_len = table_size / sizeof (struct table_entry);
-  unw_word_t e_addr = 0, start_ip_offset, fde_offset;
-  unw_word_t start = ~(unw_word_t) 0, end = 0;
   unw_accessors_t *a = unw_get_accessors (as);
-  unsigned long lo, hi, mid;
+  unsigned int lo, mid = 0, hi;
   int ret;
+  unw_word_t e_addr;
+  struct table_entry *table = (struct table_entry *)table_addr;
+  int32_t start_ip, end, fde_addr;
+
+  if (!table_len)
+       return 0;
 
   /* do a binary search for right entry: */
   for (lo = 0, hi = table_len; lo < hi;)
     {
       mid = (lo + hi) / 2;
-      e_addr = table + mid * sizeof (struct table_entry);
-      if ((ret = remote_read (e_addr, start_ip_offset)) < 0)
-       return ret;
+      e_addr = (unw_word_t)&table[mid].start_ip;
+      ret = dwarf_reads32 (as, a, &e_addr, &start_ip, arg);
+      if (ret < 0)
+        return ret;
 
-      start = start_ip_offset;
-      if (rel_ip < start)
+      if (rel_ip < start_ip)
        hi = mid;
       else
        {
          if (mid + 1 >= table_len)
            break;
 
-         if ((ret = remote_read (e_addr + sizeof (struct table_entry),
-                                 start_ip_offset)) < 0)
-           return ret;
-         end = start_ip_offset;
+          e_addr = (unw_word_t)&table[mid+1].start_ip;
+          ret = dwarf_reads32 (as, a, &e_addr, &end, arg);
+          if (ret < 0)
+            return ret;
 
          if (rel_ip >= end)
            lo = mid + 1;
@@ -302,17 +301,21 @@
            break;
        }
     }
-  if (rel_ip < start || rel_ip >= end)
+  if (rel_ip < start_ip)
     return 0;
-  e->start_ip_offset = start;
 
-  if ((ret = remote_read (e_addr, fde_offset)) < 0)
+  e_addr = (unw_word_t)&table[mid].fde_addr;
+  ret = dwarf_reads32 (as, a, &e_addr, &fde_addr, arg);
+  if (ret < 0)
     return ret;
-  e->fde_offset = fde_offset;
+
+  entry->start_ip = start_ip;
+  entry->fde_addr = fde_addr;
+
   return 1;
 }
 
-#endif /* UNW_LOCAL_ONLY */
+#endif /* !UNW_LOCAL_ONLY */
 
 PROTECTED int
 dwarf_search_unwind_table (unw_addr_space_t as, unw_word_t ip,
@@ -343,15 +346,15 @@
     {
       segbase = di->u.rti.segbase;
       e = lookup ((struct table_entry *) di->u.rti.table_data,
-                 di->u.rti.table_len * sizeof (unw_word_t), ip - segbase);
+                 di->u.rti.table_len, ip - segbase);
     }
   else
 #endif
     {
 #ifndef UNW_LOCAL_ONLY
       segbase = di->u.rti.segbase;
-      if ((ret = remote_lookup (as, di->u.rti.table_data,
-                               di->u.rti.table_len * sizeof (unw_word_t),
+      if ((ret = remote_lookup (as, (struct table_entry *)di->u.rti.table_data,
+                               di->u.rti.table_len,
                                ip - segbase, &ent, arg)) < 0)
        return ret;
       if (ret)
@@ -366,9 +369,7 @@
         unwind info.  */
       return -UNW_ENOINFO;
     }
-  Debug (16, "ip=0x%lx, start_ip=0x%lx\n",
-        (long) ip, (long) (e->start_ip_offset + segbase));
-  fde_addr = e->fde_offset + segbase;
+  fde_addr = e->fde_addr + segbase;
   return dwarf_parse_fde (as, a, &fde_addr, pi, &pi->extra.dwarf_info, arg);
 }
 

max