Re: [patch 6/6] gdbserver build-id attribute generator

[Aleksandar Ristovski <aristovski at qnx dot com>]

Addressed Jan's comments.



On 13-03-27 10:50 AM, Jan Kratochvil wrote:
> On Wed, 27 Mar 2013 15:38:29 +0100, Aleksandar Ristovski wrote:
> > On 13-03-26 04:41 PM, Jan Kratochvil wrote:
> > > > > +  if (build_id_list_p)
> > > > > +    qsort (VEC_address (build_id_list_s, data.list),
> > > > > +	   VEC_length (build_id_list_s, data.list),
> > > > > +	   sizeof (build_id_list_s), compare_build_id_list);
> > > It is always already sorted by Linux kernel, rather a for cycle to verify it
> > > really is sorted.
> >
> > Can we guarantee this is always the case?
>
> Yes.
>
> The problem is that if it is unsorted there is a bug somewhere and that qsort
> will hide that bug.


Qsort removed. I didn't put any traversal; we are making assumption that 
the list will be sorted. The checks in the other bits make sure that we 
either find the right mapping or none at all, so worst case scenario is 
we don't get build-id communicated to gdb.



Thanks,

Aleksandar

>From 7b49e440f3376999acf4220893944206e94597be Mon Sep 17 00:00:00 2001
From: Aleksandar Ristovski <ARistovski@qnx.com>
Date: Wed, 27 Mar 2013 11:56:57 -0400
Subject: [PATCH 6/8] gdbserver build-id attribute generator

	* doc/gdb.texinfo (Library List Format for SVR4 Targets): Add
	'build-id' in description, example, new attribute in dtd.
	* features/library-list-svr4.dtd (library-list-svr4): New
	'build-id' attribute.
	* linux-low.c (linux-maps.h, search.h): Include.
	(find_phdr_p_ftype, find_phdr, find_phdr_p): New.
	(get_dynamic): Use find_pdhr to traverse program headers.
	(struct mapping_entry): New structure.
	(mapping_entry_s): New typedef, new vector type def.
	(free_mapping_entry, compare_mapping_entry,
	 compare_mapping_entry_range, compare_mapping_entry_inode): New.
	(struct find_memory_region_callback_data): New.
	(find_memory_region_callback): New fwd. declaration.
	(read_build_id, find_memory_region_callback, get_hex_build_id): New.
	(linux_qxfer_libraries_svr4): Add optional build-id attribute
	to reply XML document.
---
 gdb/doc/gdb.texinfo                |   17 +-
 gdb/features/library-list-svr4.dtd |   13 +-
 gdb/gdbserver/linux-low.c          |  396 ++++++++++++++++++++++++++++++++++--
 3 files changed, 391 insertions(+), 35 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 38ce259..7c17209 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -40323,6 +40323,8 @@ memory address.  It is a displacement of absolute memory address against
 address the file was prelinked to during the library load.
 @item
 @code{l_ld}, which is memory address of the @code{PT_DYNAMIC} segment
+@item
+@code{build-id}, hex encoded @code{NT_GNU_BUID_ID} note, if it exists.
 @end itemize
 
 Additionally the single @code{main-lm} attribute specifies address of
@@ -40340,7 +40342,7 @@ looks like this:
   <library name="/lib/ld-linux.so.2" lm="0xe4f51c" l_addr="0xe2d000"
            l_ld="0xe4eefc"/>
   <library name="/lib/libc.so.6" lm="0xe4fbe8" l_addr="0x154000"
-           l_ld="0x152350"/>
+           l_ld="0x152350" build-id="9afccf7cc41e6293476223fe72480854"/>
 </library-list-svr>
 @end smallexample
 
@@ -40349,13 +40351,14 @@ The format of an SVR4 library list is described by this DTD:
 @smallexample
 <!-- library-list-svr4: Root element with versioning -->
 <!ELEMENT library-list-svr4  (library)*>
-<!ATTLIST library-list-svr4  version CDATA   #FIXED  "1.0">
-<!ATTLIST library-list-svr4  main-lm CDATA   #IMPLIED>
+<!ATTLIST library-list-svr4  version  CDATA   #FIXED  "1.0">
+<!ATTLIST library-list-svr4  main-lm  CDATA   #IMPLIED>
 <!ELEMENT library            EMPTY>
-<!ATTLIST library            name    CDATA   #REQUIRED>
-<!ATTLIST library            lm      CDATA   #REQUIRED>
-<!ATTLIST library            l_addr  CDATA   #REQUIRED>
-<!ATTLIST library            l_ld    CDATA   #REQUIRED>
+<!ATTLIST library            name     CDATA   #REQUIRED>
+<!ATTLIST library            lm       CDATA   #REQUIRED>
+<!ATTLIST library            l_addr   CDATA   #REQUIRED>
+<!ATTLIST library            l_ld     CDATA   #REQUIRED>
+<!ATTLIST library            build-id CDATA   #IMPLIED>
 @end smallexample
 
 @node Memory Map Format
diff --git a/gdb/features/library-list-svr4.dtd b/gdb/features/library-list-svr4.dtd
index cae7fd8..fdd6ec0 100644
--- a/gdb/features/library-list-svr4.dtd
+++ b/gdb/features/library-list-svr4.dtd
@@ -6,11 +6,12 @@
 
 <!-- library-list-svr4: Root element with versioning -->
 <!ELEMENT library-list-svr4  (library)*>
-<!ATTLIST library-list-svr4  version CDATA   #FIXED  "1.0">
-<!ATTLIST library-list-svr4  main-lm CDATA   #IMPLIED>
+<!ATTLIST library-list-svr4  version  CDATA   #FIXED  "1.0">
+<!ATTLIST library-list-svr4  main-lm  CDATA   #IMPLIED>
 
 <!ELEMENT library            EMPTY>
-<!ATTLIST library            name    CDATA   #REQUIRED>
-<!ATTLIST library            lm      CDATA   #REQUIRED>
-<!ATTLIST library            l_addr  CDATA   #REQUIRED>
-<!ATTLIST library            l_ld    CDATA   #REQUIRED>
+<!ATTLIST library            name     CDATA   #REQUIRED>
+<!ATTLIST library            lm       CDATA   #REQUIRED>
+<!ATTLIST library            l_addr   CDATA   #REQUIRED>
+<!ATTLIST library            l_ld     CDATA   #REQUIRED>
+<!ATTLIST library            build-id CDATA   #IMPLIED>
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 72c51e0..f820792 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -20,6 +20,7 @@
 #include "linux-low.h"
 #include "linux-osdata.h"
 #include "agent.h"
+#include "linux-maps.h"
 
 #include "gdb_wait.h"
 #include <stdio.h>
@@ -43,6 +44,7 @@
 #include "gdb_stat.h"
 #include <sys/vfs.h>
 #include <sys/uio.h>
+#include <search.h>
 #ifndef ELFMAG0
 /* Don't include <linux/elf.h> here.  If it got included by gdb_proc_service.h
    then ELFMAG0 will have been defined.  If it didn't get included by
@@ -5432,6 +5434,70 @@ get_phdr_phnum_from_proc_auxv (const int pid, const int is_elf64,
   return 0;
 }
 
+
+/* Predicate function type returns 1 if the given phdr is what is
+   being looked for.  Returns 0 otherwise.  */
+
+typedef int (*find_phdr_p_ftype)(const void *phdr, int is_elf64,
+				 const void *data);
+
+/* Linearly traverse pheaders given in PHDR until supplied
+   predicate function returns 1.  If supplied predicate function
+   did return 1, stop traversal and return that PHDR.  */
+
+static const void *
+find_phdr (int is_elf64, const void *const phdr_begin,
+	   const void *const phdr_end, find_phdr_p_ftype find_phdr_p,
+	   const void *const data)
+{
+#define SIZEOFHDR(hdr) (is_elf64? sizeof((hdr)._64) : sizeof((hdr)._32))
+#define PHDR_NEXT(hdrp) ((void *) ((char *)(hdrp) + SIZEOFHDR(*hdrp)))
+
+  union ElfXX_Phdr
+    {
+      Elf32_Phdr _32;
+      Elf64_Phdr _64;
+    } const *phdr = phdr_begin;
+
+  if (phdr == NULL)
+    return NULL;
+
+  while (PHDR_NEXT (phdr) <= phdr_end)
+    {
+      if (find_phdr_p (phdr, is_elf64, data) == 1)
+	return phdr;
+      phdr = PHDR_NEXT (phdr);
+    }
+
+  return NULL;
+#undef PHDR_NEXT
+#undef SIZEOFHDR
+}
+
+
+static int
+find_phdr_p (const void *const phdr, const int is_elf64,
+		const void *const data)
+{
+  const ULONGEST *const type = data;
+
+  if (is_elf64)
+    {
+      const Elf64_Phdr *const p = phdr;
+
+      if (p->p_type == *type)
+	return 1;
+    }
+  else
+    {
+      const Elf32_Phdr *const p = phdr;
+
+      if (p->p_type == *type)
+	return 1;
+    }
+  return 0;
+}
+
 /* Return &_DYNAMIC (via PT_DYNAMIC) in the inferior, or 0 if not present.  */
 
 static CORE_ADDR
@@ -5441,6 +5507,8 @@ get_dynamic (const int pid, const int is_elf64)
   int num_phdr, i;
   unsigned char *phdr_buf;
   const int phdr_size = is_elf64 ? sizeof (Elf64_Phdr) : sizeof (Elf32_Phdr);
+  const void *phdr;
+  ULONGEST p_type;
 
   if (get_phdr_phnum_from_proc_auxv (pid, is_elf64, &phdr_memaddr, &num_phdr))
     return 0;
@@ -5454,21 +5522,24 @@ get_dynamic (const int pid, const int is_elf64)
   /* Compute relocation: it is expected to be 0 for "regular" executables,
      non-zero for PIE ones.  */
   relocation = -1;
-  for (i = 0; relocation == -1 && i < num_phdr; i++)
-    if (is_elf64)
-      {
-	Elf64_Phdr *const p = (Elf64_Phdr *) (phdr_buf + i * phdr_size);
+  p_type = PT_PHDR;
+  phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
+		    find_phdr_p, &p_type);
+  if (phdr)
+    {
+      if (is_elf64)
+	{
+	  const Elf64_Phdr *const p = phdr;
 
-	if (p->p_type == PT_PHDR)
 	  relocation = phdr_memaddr - p->p_vaddr;
-      }
-    else
-      {
-	Elf32_Phdr *const p = (Elf32_Phdr *) (phdr_buf + i * phdr_size);
+	}
+      else
+	{
+	  const Elf32_Phdr *const p = phdr;
 
-	if (p->p_type == PT_PHDR)
 	  relocation = phdr_memaddr - p->p_vaddr;
-      }
+	}
+    }
 
   if (relocation == -1)
     {
@@ -5485,21 +5556,25 @@ get_dynamic (const int pid, const int is_elf64)
       return 0;
     }
 
+  p_type = PT_DYNAMIC;
+  phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
+		    find_phdr_p, &p_type);
+
   for (i = 0; i < num_phdr; i++)
     {
       if (is_elf64)
 	{
-	  Elf64_Phdr *const p = (Elf64_Phdr *) (phdr_buf + i * phdr_size);
+	  const Elf64_Phdr *const p
+	    = (Elf64_Phdr *) (phdr_buf + i * phdr_size);
 
-	  if (p->p_type == PT_DYNAMIC)
-	    return p->p_vaddr + relocation;
+	  return p->p_vaddr + relocation;
 	}
       else
 	{
-	  Elf32_Phdr *const p = (Elf32_Phdr *) (phdr_buf + i * phdr_size);
+	  const Elf32_Phdr *const p
+	    = (Elf32_Phdr *) (phdr_buf + i * phdr_size);
 
-	  if (p->p_type == PT_DYNAMIC)
-	    return p->p_vaddr + relocation;
+	  return p->p_vaddr + relocation;
 	}
     }
 
@@ -5641,6 +5716,265 @@ struct link_map_offsets
     int l_prev_offset;
   };
 
+
+/* Structure for holding a mapping.  Only mapping
+   containing l_ld can have hex_build_id set.
+
+   Fields are populated from linux_find_memory_region parameters.  */
+
+struct mapping_entry
+{
+  ULONGEST vaddr;
+  ULONGEST size;
+  ULONGEST offset;
+  ULONGEST inode;
+
+  /* Hex encoded string allocated using xmalloc, and
+     needs to be freed.  It can be NULL.  */
+
+  char *hex_build_id;
+};
+
+typedef struct mapping_entry mapping_entry_s;
+
+DEF_VEC_O(mapping_entry_s);
+
+static void
+free_mapping_entry (VEC (mapping_entry_s) *lst)
+{
+  int ix;
+  mapping_entry_s *p;
+
+  for (ix = 0; VEC_iterate (mapping_entry_s, lst, ix, p); ++ix)
+    xfree (p->hex_build_id);
+
+  VEC_free (mapping_entry_s, lst);
+}
+
+/* Used for finding a mapping containing the given
+   l_ld passed in K.  */
+
+static int
+compare_mapping_entry_range (const void *const k, const void *const b)
+{
+  const ULONGEST key = *(CORE_ADDR*) k;
+  const mapping_entry_s *const p = b;
+
+  if (key < p->vaddr)
+    return -1;
+
+  if (key < p->vaddr + p->size)
+    return 0;
+
+  return 1;
+}
+
+struct find_memory_region_callback_data
+{
+  unsigned is_elf64;
+
+  /* Return.  Ordered list of all object mappings sorted in
+     ascending order by VADDR.  Must be freed with free_mapping_entry.  */
+  VEC (mapping_entry_s) *list;
+};
+
+static linux_find_memory_region_ftype find_memory_region_callback;
+
+/* Read .note.gnu.build-id from PT_NOTE.  */
+
+static void
+read_build_id (struct find_memory_region_callback_data *const p,
+	       mapping_entry_s *const bil, const CORE_ADDR load_addr,
+	       const CORE_ADDR l_addr)
+{
+  union ElfXX_Ehdr
+    {
+      Elf32_Ehdr _32;
+      Elf64_Ehdr _64;
+    } ehdr;
+  union ElfXX_Phdr
+    {
+      Elf32_Phdr _32;
+      Elf64_Phdr _64;
+    } const *phdr;
+  union ElfXX_Nhdr
+    {
+      Elf32_Nhdr _32;
+      Elf64_Nhdr _64;
+    } *nhdr;
+#define HDR(hdr, fld) (((p)->is_elf64)? (hdr)._64.fld : (hdr)._32.fld)
+#define SIZEOFHDR(hdr) (((p)->is_elf64)?sizeof((hdr)._64):sizeof((hdr)._32))
+  if (linux_read_memory (load_addr, (unsigned char *) &ehdr, SIZEOFHDR (ehdr))
+      == 0
+      && HDR (ehdr, e_ident[EI_MAG0]) == ELFMAG0
+      && HDR (ehdr, e_ident[EI_MAG1]) == ELFMAG1
+      && HDR (ehdr, e_ident[EI_MAG2]) == ELFMAG2
+      && HDR (ehdr, e_ident[EI_MAG3]) == ELFMAG3)
+    {
+      void *phdr_buf;
+      const ULONGEST p_type = PT_NOTE;
+
+      gdb_assert (HDR (ehdr, e_phnum) < 100);  /* Basic sanity check.  */
+      gdb_assert (HDR (ehdr, e_phentsize) == SIZEOFHDR (*phdr));
+      phdr_buf = alloca (HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize));
+
+      if (linux_read_memory (load_addr + HDR (ehdr, e_phoff), phdr_buf,
+			     HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize))
+	  != 0)
+	{
+	  warning ("Could not read program header.");
+	  return;
+	}
+
+      phdr = phdr_buf;
+
+      while ((phdr = find_phdr (p->is_elf64, phdr, (char *) phdr_buf
+		       + HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize),
+		       find_phdr_p, &p_type)) != NULL)
+	{
+	  void *const pt_note = xmalloc (HDR (*phdr, p_memsz));
+	  const void *const pt_end
+	    = (char*) pt_note + HDR (*phdr, p_memsz);
+
+	  if (linux_read_memory (HDR (*phdr, p_vaddr) + l_addr,
+		pt_note, HDR (*phdr, p_memsz)) != 0)
+	    {
+	      xfree (pt_note);
+	      warning ("Could not read note.");
+	      break;
+	    }
+
+	  nhdr = pt_note;
+	  while ((void *) nhdr < pt_end)
+	    {
+	      const size_t note_sz
+		= HDR (*nhdr, n_namesz) + HDR (*nhdr, n_descsz)
+		  + SIZEOFHDR (*nhdr);
+
+	      if (((char *) nhdr + note_sz) > (char *) pt_end)
+		{
+		  warning ("Malformed PT_NOTE\n");
+		  break;
+		}
+	      if (HDR (*nhdr, n_type) == NT_GNU_BUILD_ID)
+		{
+		  bil->hex_build_id = xmalloc (note_sz * 2 + 1);
+		  bin2hex ((gdb_byte*) nhdr, bil->hex_build_id, note_sz);
+		  xfree (pt_note);
+		  return;
+		}
+	      nhdr = (void*) ((char *) nhdr + note_sz);
+	    }
+	  xfree (pt_note);
+	}
+    }
+  else
+    warning ("Reading build-id failed.");
+#undef HDR
+#undef SIZEOFHDR
+}
+
+
+/* Add mapping_entry.  */
+
+static int
+find_memory_region_callback (ULONGEST vaddr, ULONGEST size, ULONGEST offset,
+			     ULONGEST inode, int read, int write, int exec,
+			     int modified, const char *filename, void *data)
+{
+  if (inode != 0)
+    {
+      struct find_memory_region_callback_data *const p = data;
+      mapping_entry_s bil;
+
+      bil.vaddr = vaddr;
+      bil.size = size;
+      bil.offset = offset;
+      bil.inode = inode;
+      bil.hex_build_id = NULL;
+
+      VEC_safe_push (mapping_entry_s, p->list, &bil);
+    }
+
+  /* Continue the traversal.  */
+  return 0;
+}
+
+/* Linear reverse find starting from RBEGIN towards REND looking for
+   the lowest vaddr mapping of the same inode and zero offset.  */
+
+static mapping_entry_s *
+lrfind_mapping_entry (mapping_entry_s *const rbegin,
+		      const mapping_entry_s *const rend)
+{
+  mapping_entry_s *p;
+
+  for (p = rbegin - 1; p >= rend && p->inode == rbegin->inode; --p)
+    {
+      if (p->offset == 0)
+	{
+	  /* Check if the mapping is sane: */
+	  if (p->vaddr + rbegin->offset == rbegin->vaddr)
+	    return p;
+	  warning ("Mapping with offset 0 found but does not correspond to "
+	           " the given l_ld");
+	  /* Nothing returned in this case. */
+	  break;
+	}
+    }
+
+  return NULL;
+}
+
+/* Get build-id for the given L_LD.  DATA must point to
+   already filled list of mapping_entry elements.
+
+   Return build_id as stored in the list element corresponding
+   to L_LD.
+
+   NULL may be returned if build-id could not be fetched.
+
+   Returned string must not be freed explicitly.  */
+
+static const char *
+get_hex_build_id (const CORE_ADDR l_addr, const CORE_ADDR l_ld,
+		  struct find_memory_region_callback_data *const data)
+{
+  mapping_entry_s *bil;
+
+  if (VEC_address (mapping_entry_s, data->list) == NULL)
+    return NULL;
+
+  bil = bsearch (&l_ld, VEC_address (mapping_entry_s, data->list),
+		 VEC_length (mapping_entry_s, data->list),
+		 sizeof (mapping_entry_s), compare_mapping_entry_range);
+
+  if (bil == NULL)
+    return NULL;
+
+  if (bil->hex_build_id == NULL)
+    {
+      CORE_ADDR load_addr;
+      mapping_entry_s *const bil_min
+	= lrfind_mapping_entry (bil,
+				VEC_address (mapping_entry_s, data->list));
+      if (bil_min != NULL)
+	{
+	  load_addr = bil_min->vaddr;
+	  read_build_id (data, bil, load_addr, l_addr);
+	}
+      else
+	{
+	  /* Do not try to find hex_build_id again.  */
+	  bil->hex_build_id = xstrdup ("");
+	  warning ("Could not determine load address; "
+		   "build_id can not be used.");
+	}
+    }
+
+  return bil->hex_build_id;
+}
+
 /* Construct qXfer:libraries-svr4:read reply.  */
 
 static int
@@ -5653,6 +5987,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   struct process_info_private *const priv = current_process ()->private;
   char filename[PATH_MAX];
   int pid, is_elf64;
+  struct find_memory_region_callback_data data;
 
   static const struct link_map_offsets lmo_32bit_offsets =
     {
@@ -5688,6 +6023,14 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   is_elf64 = elf_64_file_p (filename, &machine);
   lmo = is_elf64 ? &lmo_64bit_offsets : &lmo_32bit_offsets;
 
+  data.is_elf64 = is_elf64;
+  data.list = NULL;
+  VEC_reserve (mapping_entry_s, data.list, 16);
+  if (linux_find_memory_regions_full (
+	lwpid_of (get_thread_lwp (current_inferior)),
+	find_memory_region_callback, &data, NULL) < 0)
+    warning ("Finding memory regions failed");
+
   if (priv->r_debug == 0)
     priv->r_debug = get_r_debug (pid, is_elf64);
 
@@ -5762,6 +6105,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 	      /* 6x the size for xml_escape_text below.  */
 	      size_t len = 6 * strlen ((char *) libname);
 	      char *name;
+	      const char *hex_enc_build_id = NULL;
 
 	      if (!header_done)
 		{
@@ -5770,21 +6114,28 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 		  header_done = 1;
 		}
 
-	      while (allocated < p - document + len + 200)
+	      name = xml_escape_text ((char *) libname);
+	      hex_enc_build_id = get_hex_build_id (l_addr, l_ld, &data);
+
+	      while (allocated < (p - document + len + 200
+				  + (hex_enc_build_id != NULL
+				     ? strlen (hex_enc_build_id) : 0)))
 		{
 		  /* Expand to guarantee sufficient storage.  */
-		  uintptr_t document_len = p - document;
+		  const ptrdiff_t document_len = p - document;
 
-		  document = xrealloc (document, 2 * allocated);
 		  allocated *= 2;
+		  document = xrealloc (document, allocated);
 		  p = document + document_len;
 		}
 
-	      name = xml_escape_text ((char *) libname);
 	      p += sprintf (p, "<library name=\"%s\" lm=\"0x%lx\" "
-			       "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
+			       "l_addr=\"0x%lx\" l_ld=\"0x%lx\"",
 			    name, (unsigned long) lm_addr,
 			    (unsigned long) l_addr, (unsigned long) l_ld);
+	      if (hex_enc_build_id != NULL)
+		p += sprintf (p, " build-id=\"%s\"", hex_enc_build_id);
+	      p += sprintf(p, "/>");
 	      free (name);
 	    }
 	  else if (lm_prev == 0)
@@ -5819,6 +6170,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 
   memcpy (readbuf, document + offset, len);
   xfree (document);
+  free_mapping_entry (data.list);
 
   return len;
 }
-- 
1.7.10.4