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

[Aleksandar Ristovski <aristovski@qnx.com>]

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

Attached patch addresses your comments. See inline for details.


On 13-03-31 01:43 PM, Jan Kratochvil wrote:
> On Thu, 28 Mar 2013 21:53:38 +0100, Aleksandar Ristovski wrote:
>> Fixed patch. I haven't addressed any of your concerns except fixed
>> what was broken. There are two things changed:
>>
>> 1) get_dynamic, you will see I left it unfinished when switched to
>> generic "find_phdr" to address phdr traversal duplication code.
>
> I wrote at "find_phdr_p":
> 	But I do not understand why this function exists
>
> which probably corresponds to this your comment.
>
>
>> 2) lrfind_mapping_entry can not check for vaddr + offset as offset
>> is file offset, and for some shared objects this will not match even
>> though the vaddr of the entry with zero offset is valid.
>
> Oops, you are right:
>
> 7ffff7ddc000-7ffff7dfd000 r-xp 00000000 fd:01 51415762                   /usr/lib64/ld-2.17.so
> 7ffff7ffc000-7ffff7ffe000 rw-p 00020000 fd:01 51415762                   /usr/lib64/ld-2.17.so
> (gdb) p/x 0x7ffff7ffc000-0x7ffff7ddc000
>                            $1 = 0x220000
>
> It is mapped by additional displacement of 2MB.  Adjustment is suggested below.
>
>
> BTW for gdbserver compatibility I have posted:
> 	[patch 1/2+7.6] /proc/PID/smaps: filename fix
> 	http://sourceware.org/ml/gdb-patches/2013-03/msg01120.html
> 	[patch 2/2+7.6] /proc/PID/smaps: Linux kernel 3.8.3 compat.
> 	http://sourceware.org/ml/gdb-patches/2013-03/msg01119.html
> but you probably did not face this compat. problem.
>
>
>> Stepping through code now shows some of the things you couldn't see,
>> like e.g. why is there so->build_id, and where is it being set (you
>> couldn't see it being set before as qXfer_library was broken).
>>
>>
>> ---
>> Aleksandar
>>
>>
>>
>> On 13-03-27 04:17 PM, Aleksandar Ristovski wrote:
>>> 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 80cd24335bcff6625b5c69c1b2f2d43142db08d1 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          |  388 +++++++++++++++++++++++++++++++++---
>>   3 files changed, 381 insertions(+), 37 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.
>
> Typo: NT_GNU_BUILD_ID

[AR]
Done.

>
>
>>   @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..aa248e9 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,15 +5434,81 @@ get_phdr_phnum_from_proc_auxv (const int pid, const int is_elf64,
>>     return 0;
>>   }
>>
>> +
>
> Extra empty line, there should be only one empty line.
>

[AR] Ok.

>
>> +/* 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);
>
> GNU Coding Standards formatting:
>     typedef int (*find_phdr_p_ftype) (const void *phdr, int is_elf64,
>
> GDB uses *_ftype types without that * pointer,
> use then find_phdr_p_ftype *find_phdr_p.


[AR] Ok.

>
>> +
>> +/* Linearly traverse pheaders given in PHDR until supplied
>                          program headers given between PHDR_BEGIN and PHDR_END
>
>> +   predicate function returns 1.  If supplied predicate function
>> +   did return 1, stop traversal and return that PHDR.  */
>                                                that program header.
>
>> +
>> +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,
>
> Use find_phdr_p_ftype *find_phdr_p.
>
>> +	   const void *const data)
>> +{
>> +#define SIZEOFHDR(hdr) (is_elf64? sizeof((hdr)._64) : sizeof((hdr)._32))
>
> GNU Coding Standards formatting:
>     #define SIZEOFHDR(hdr) (is_elf64 ? sizeof ((hdr)._64) : sizeof ((hdr)._32))
>
> But in fact I do not see why you define a macro which is used only once.

[AR]: For readability, so the generic part of the code does not mention 
"64" or "32".

>
>
>> +#define PHDR_NEXT(hdrp) ((void *) ((char *)(hdrp) + SIZEOFHDR(*hdrp)))
>
> GNU Coding Standards formatting and also parentheses around each macro
> parameter:
>     #define PHDR_NEXT(hdrp) ((void *) ((char *) (hdrp) + SIZEOFHDR (*hdrp)))
>
> But as GDB codebase allows void * arithmetics it can be simplified to (also
> putting there const otherwise it deconstifies the passed in pointer):
>     #define PHDR_NEXT(hdrp) (((const void *) (hdrp) + SIZEOFHDR (*hdrp)))

[AR]: void* arithmetic is not defined even if gcc allows it. Changed to 
casting to const gdb_byte for the arithmetic part.

>
>
>> +
>> +  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
>> +}
>> +
>
> Missing function comment.

[AR] Ok.

>
>
>> +
>> +static int
>> +find_phdr_p (const void *const phdr, const int is_elf64,
>> +		const void *const data)
>
> But I do not understand why this function exists - it could be integrated in
> find_phdr as very every caller of find_phdr passse as find_phdr_p parameter
> this find_phdr_p implementation.


[AR] Yes, but I am eyeing solib-svr4.c loops over pheaders 
generalization - find_phdr could be moved to 'common' and possibly 
called 'iterate_phdrs' with the ability to pass in any function, not 
necessarily a predicate only. E.g. svr4_exec_displacement, etc...)

>
>
>> +{
>> +  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
>>   get_dynamic (const int pid, const int is_elf64)
>>   {
>>     CORE_ADDR phdr_memaddr, relocation;
>> -  int num_phdr, i;
>> +  int num_phdr;
>>     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 != NULL)
>> +    {
>> +      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,23 @@ get_dynamic (const int pid, const int is_elf64)
>>         return 0;
>>       }
>>
>> -  for (i = 0; i < num_phdr; i++)
>> +  p_type = PT_DYNAMIC;
>> +  phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
>> +		    find_phdr_p, &p_type);
>> +
>> +  if (phdr != NULL)
>>       {
>>         if (is_elf64)
>>   	{
>> -	  Elf64_Phdr *const p = (Elf64_Phdr *) (phdr_buf + i * phdr_size);
>> +	  const Elf64_Phdr *const p = phdr;
>>
>> -	  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 = phdr;
>>
>> -	  if (p->p_type == PT_DYNAMIC)
>> -	    return p->p_vaddr + relocation;
>> +	  return p->p_vaddr + relocation;
>>   	}
>>       }
>>
>> @@ -5641,6 +5714,255 @@ 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
>> +{
>
> Here should be the line:
> 	/* Fields are populated from linux_find_memory_region parameters.  */

[AR] ok.

>
>
>> +  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.  */
>
> It is NT_GNU_BUILD_ID note from PT_NOTE segment.
>
> .note.gnu.build-id is section name, PT_NOTE is segment name.  Those do not
> match.

[AR] ok.

>
>
>> +
>> +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))
>
> These macros are already defined above, use only one definition.  It would be
> appropriate to make their name slightly longer in such case, not required.


[AR]. Moved unions and defines up, removed "undef". Changed naming to be 
slightly less prone to collisions and clearer.


>
>
>> +  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)
>
> phdr_buf is void * and GDB codebase allows void * arithmetics so the cast
> could be removed.

[AR] I really dislike using void* even if gdb uses it. It's not correct.

>
> find_phdr_p probably should not be passed, as discussed above.

[AR] I can remove it, but the idea is to make generic phdr iterator 
similar to 'maps' parser.

>
> Assignment needs to be on a separate line according to GNU Coding Standards,
> therefore:
>        for (;;)
> 	{
>            phdr = find_phdr (p->is_elf64, phdr,
> 	                    (phdr_buf
> 			     + HDR (ehdr, e_phnum) * HDR (ehdr, e_phentsize)),
> 			    find_phdr_p, &p_type);
> 	  if (phdr == NULL)
> 	    break;
>
>
>> +	{
>> +	  void *const pt_note = xmalloc (HDR (*phdr, p_memsz));
>> +	  const void *const pt_end
>> +	    = (char*) pt_note + HDR (*phdr, p_memsz);
>
> When it does not fit on a single line use separate declaration line:
> 	  const void *pt_end;
>
> 	  pt_end = (gdb_byte *) pt_note + HDR (*phdr, p_memsz);
>
> And also you use 'char' for byte but GDB - even with recent Pedro's changes
> - prefers to use gdb_byte in such case.  It is not a printable character.
>
>
>> +
>> +	  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);
>
> Again it is more readable if split, also please keep the order as present in
> the file:
> 	      const size_t note_sz;
>
> 	      note_sz = (SIZEOFHDR (*nhdr) + HDR (*nhdr, n_namesz)
> 			 + HDR (*nhdr, n_descsz));
>
> But there is also missing alignment to 4 bytes of both n_namesz and n_descsz:
> 	Padding is present, if necessary, to ensure 4-byte alignment for the
> 	descriptor. Such padding is not included in namesz.

[AR] Ok, code re-arranged; sizes rounded up to appropriate size.

> +
> 	Padding is present, if necessary, to ensure 4-byte alignment for the
> 	next note entry. Such padding is not included in descsz.
>
>
>> +
>> +	      if (((char *) nhdr + note_sz) > (char *) pt_end)
>
> gdb_byte *
>
> And I asked for checking NOTE_SZ == 0 but you still do not check it.  If there
> will be NOTE_SZ == 0 then bin2hex below will use the code path:
>    /* May use a length, or a nul-terminated string as input.  */
>    if (count == 0)
>      count = strlen ((char *) bin);
>
> which may in a worst case even crash GDB on invalid file running out through
> non-zero bytes out of mapped memory.

[AR] It is now being checked.

>
>
>> +		{
>> +		  warning ("Malformed PT_NOTE\n");
>> +		  break;
>> +		}
>
> You need to check here also the name content, it is "GNU\x00" (n_namesz == 4).

[AR], can it be any other name if type is NT_GNU_BUILD_ID? Added the 
check but seems like an overkill to me.

>
>
>> +	      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);
>
> I wrote last time:
> GNU Coding Standard formatting:
>                        bin2hex ((gdb_byte *) nhdr, bil->hex_build_id, note_sz);
>
>
> But this is not the build-id.
>
> readelf -n
> Build ID: 1dfca42f0dac568cf81fedc2a9a37a98789a03e4
>
> vs.  gdbserver:
>
> <library name="/lib64/ld-linux-x86-64.so.2" lm="0x7ffff7ffd998" l_addr="0x7ffff7ddc000" l_ld="0x7ffff7ffcdf0" build-id="040000001400000003000000474e55001dfca42f0dac568cf81fedc2a9a37a98789a03e4"/>
>
> You need to send only the real build-id bytes, omitting the note header and the
> note name ("GNU\0").
>
> Then it will not match GDB itself, it also needs to be updated to process only
> the build-id bytes, not the header.

[AR] Ok.

>
>
>> +		  xfree (pt_note);
>> +		  return;
>> +		}
>> +	      nhdr = (void*) ((char *) nhdr + note_sz);
>
> I wrote last time:
> GNU Coding Standard formatting + simplification:
>                    nhdr = (void *) nhdr + note_sz;

[AR] I re-arranged the code. Void * arithmetic is not right, so I'm 
avoiding it.

>
>
>> +	    }
>> +	  xfree (pt_note);
>> +	}
>> +    }
>> +  else
>> +    warning ("Reading build-id failed.");
>
> I would omit these warnings.  But otherwise please put there some better
> identifiers, such as vaddr and/or filename.  Seeing just many such errors is
> not too useful as I have found during the debugging today.

[AR] Removed the warning.

>
>
>> +#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)
>> +      return p;
>
> I had here this layout:
> 7ffff7ddc000-7ffff7dfd000 r-xp 00000000 fd:01 51415762                   /usr/lib64/ld-2.17.so
> 7ffff7ff9000-7ffff7ffa000 rw-p 00000000 00:00 0
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
> 7ffff7ffc000-7ffff7ffe000 rw-p 00020000 fd:01 51415762                   /usr/lib64/ld-2.17.so
>
> so it should rather be:
>    for (p = rbegin - 1; p >= rend; --p)
>      if (p->offset == 0 && p->inode == rbegin->inode)
>        return p;
>
> Fortunately it should not have any real performance impact.

[AR] Ok, though we are not adding mappings with inode == 0. See 
'find_memory_region_callback'. I changed it anyway, seems a bit more 
robust at the expense of rather unlikely event where mapping with offset 
0 does not exist.


>
>
>> +
>> +  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;
>
> This variable declaration should be moved to the more inner block below.
>
>
>> +      mapping_entry_s *const bil_min
>> +	= lrfind_mapping_entry (bil,
>> +				VEC_address (mapping_entry_s, data->list));
>
> When it does not fit the line make the declaration separate, such as:
>
>        mapping_entry_s *const bil_min;
>
>        bil_min = lrfind_mapping_entry (bil, VEC_address (mapping_entry_s,
>                                                          data->list));

[AR] Not sure what is the advantage, but ok.

>
> There should be an empty line between declarations and code statements.
>
>> +      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.");
>                      build-id
>
> The name of the feature is "build-id" so it always should be presented this
> way to the user.  Variable names are not interesting to users.

[AR] Ok. Also, please note the change: I assign BUILD_ID_INVALID here 
rather than "" so that we can determine this at document creation time. 
I'd rather not emit build-id attribute at all than emitting empty 
build-id when it could not be fetched.


>
>
>> +	}
>> +    }
>> +
>> +  return bil->hex_build_id;
>> +}
>> +
>>   /* Construct qXfer:libraries-svr4:read reply.  */
>>
>>   static int
>> @@ -5653,6 +5975,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 +6011,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 (
>
> There should not be opening paren at the end.

[AR] Ok.

>
>> +	lwpid_of (get_thread_lwp (current_inferior)),
>
> When the parameters are too long for proper indentation use a temporary
> variable for the value.

[AR] Ok, used 'pid' which had been there already.

>
>
>> +	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 +6093,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 +6102,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 +6158,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
>>
>
>
>
> Thanks,
> Jan
>


Thanks,

Aleksandar


[-- Attachment #2: 0006-gdbserver-build-id-attribute-generator.patch --]
[-- Type: text/x-patch, Size: 19046 bytes --]

From 799c0ca7884b2f3761762928e23ff755fe7f41b0 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.
	(ElfXX_Ehdr, ElfXX_Phdr, ElfXX_Nhdr): New.
	(ELFXX_FLD, ELFXX_SIZEOF, ELFXX_ROUNDUP, BUILD_ID_INVALID): New.
    	(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          |  409 +++++++++++++++++++++++++++++++++---
 3 files changed, 402 insertions(+), 37 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 3b63d01..25d8eea 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -40329,6 +40329,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_BUILD_ID} note, if it exists.
 @end itemize
 
 Additionally the single @code{main-lm} attribute specifies address of
@@ -40346,7 +40348,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
 
@@ -40355,13 +40357,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..61da37c 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
@@ -118,6 +120,33 @@ typedef struct
 } Elf64_auxv_t;
 #endif
 
+typedef union ElfXX_Ehdr
+{
+  Elf32_Ehdr _32;
+  Elf64_Ehdr _64;
+} ElfXX_Ehdr;
+
+typedef union ElfXX_Phdr
+{
+  Elf32_Phdr _32;
+  Elf64_Phdr _64;
+} ElfXX_Phdr;
+
+typedef union ElfXX_Nhdr
+{
+  Elf32_Nhdr _32;
+  Elf64_Nhdr _64;
+} ElfXX_Nhdr;
+
+#define ELFXX_FLD(hdr, fld) ((is_elf64) ? (hdr)._64.fld : (hdr)._32.fld)
+#define ELFXX_SIZEOF(hdr) ((is_elf64) ? sizeof ((hdr)._64) \
+				      : sizeof ((hdr)._32))
+#define ELFXX_ROUNDUP(what) ((is_elf64) ? (((what) + sizeof (Elf64_Word) - 1) \
+					   & ~(sizeof (Elf64_Word) - 1))      \
+					: (((what) + sizeof (Elf32_Word) - 1) \
+					   & ~(sizeof (Elf32_Word) - 1)))
+#define BUILD_ID_INVALID "?"
+
 /* ``all_threads'' is keyed by the LWP ID, which we use as the GDB protocol
    representation of the thread ID.
 
@@ -5432,15 +5461,76 @@ 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 *const find_phdr_p,
+	   const void *const data)
+{
+#define PHDR_NEXT(hdrp) ((const void *) ((const gdb_byte *) (hdrp) + \
+			 ELFXX_SIZEOF (*hdrp)))
+
+  const ElfXX_Phdr *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
+}
+
+/* Predicate function for find_phdr iteration.  */
+
+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
 get_dynamic (const int pid, const int is_elf64)
 {
   CORE_ADDR phdr_memaddr, relocation;
-  int num_phdr, i;
+  int num_phdr;
   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 +5544,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 != NULL)
+    {
+      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 +5578,23 @@ get_dynamic (const int pid, const int is_elf64)
       return 0;
     }
 
-  for (i = 0; i < num_phdr; i++)
+  p_type = PT_DYNAMIC;
+  phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
+		    find_phdr_p, &p_type);
+
+  if (phdr != NULL)
     {
       if (is_elf64)
 	{
-	  Elf64_Phdr *const p = (Elf64_Phdr *) (phdr_buf + i * phdr_size);
+	  const Elf64_Phdr *const p = phdr;
 
-	  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 = phdr;
 
-	  if (p->p_type == PT_DYNAMIC)
-	    return p->p_vaddr + relocation;
+	  return p->p_vaddr + relocation;
 	}
     }
 
@@ -5641,6 +5736,254 @@ struct link_map_offsets
     int l_prev_offset;
   };
 
+
+/* Structure for holding a mapping.  Only mapping
+   containing l_ld can have hex_build_id set.  */
+
+struct mapping_entry
+{
+  /* Fields are populated from linux_find_memory_region parameters.  */
+
+  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 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)
+{
+  const int is_elf64 = p->is_elf64;
+  ElfXX_Ehdr ehdr;
+
+  if (linux_read_memory (load_addr, (unsigned char *) &ehdr,
+			 ELFXX_SIZEOF (ehdr)) == 0
+      && ELFXX_FLD (ehdr, e_ident[EI_MAG0]) == ELFMAG0
+      && ELFXX_FLD (ehdr, e_ident[EI_MAG1]) == ELFMAG1
+      && ELFXX_FLD (ehdr, e_ident[EI_MAG2]) == ELFMAG2
+      && ELFXX_FLD (ehdr, e_ident[EI_MAG3]) == ELFMAG3)
+    {
+      const ElfXX_Phdr *phdr;
+      void *phdr_buf;
+      const ULONGEST p_type = PT_NOTE;
+      const unsigned e_phentsize = ELFXX_FLD (ehdr, e_phentsize);
+
+      gdb_assert (ELFXX_FLD (ehdr, e_phnum) < 100);  /* Basic sanity check.  */
+      gdb_assert (e_phentsize == ELFXX_SIZEOF (*phdr));
+      phdr_buf = alloca (ELFXX_FLD (ehdr, e_phnum) * e_phentsize);
+
+      if (linux_read_memory (load_addr + ELFXX_FLD (ehdr, e_phoff), phdr_buf,
+			     ELFXX_FLD (ehdr, e_phnum) * e_phentsize) != 0)
+	{
+	  warning ("Could not read program header.");
+	  return;
+	}
+
+      phdr = phdr_buf;
+
+      for (;;)
+	{
+	  gdb_byte *pt_note;
+	  const gdb_byte *pt_end;
+	  const ElfXX_Nhdr *nhdr;
+
+	  phdr = find_phdr (p->is_elf64, phdr, (gdb_byte *) phdr_buf
+			    + ELFXX_FLD (ehdr, e_phnum) * e_phentsize,
+			    find_phdr_p, &p_type);
+	  if (phdr == NULL)
+	    break;
+	  pt_note = xmalloc (ELFXX_FLD (*phdr, p_memsz));
+	  pt_end = (gdb_byte*) pt_note + ELFXX_FLD (*phdr, p_memsz);
+
+	  if (linux_read_memory (ELFXX_FLD (*phdr, p_vaddr) + l_addr, pt_note,
+				 ELFXX_FLD (*phdr, p_memsz)) != 0)
+	    {
+	      xfree (pt_note);
+	      warning ("Could not read note.");
+	      break;
+	    }
+
+	  nhdr = (void *) pt_note;
+	  while ((void *) nhdr < (void *) pt_end)
+	    {
+	      const size_t namesz
+		= ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_namesz));
+	      const size_t descsz
+		= ELFXX_ROUNDUP (ELFXX_FLD (*nhdr, n_descsz));
+	      const size_t note_sz = ELFXX_SIZEOF (*nhdr) + namesz + descsz;
+
+	      if (((gdb_byte *) nhdr + note_sz) > pt_end || note_sz == 0
+		  || descsz == 0)
+		{
+		  warning ("Malformed PT_NOTE\n");
+		  break;
+		}
+	      if (ELFXX_FLD (*nhdr, n_type) == NT_GNU_BUILD_ID
+		  && ELFXX_FLD (*nhdr, n_namesz) == 4)
+		{
+		  const char gnu[4] = "GNU\0";
+		  const char *const pname
+		    = (char *) nhdr + ELFXX_SIZEOF (*nhdr);
+
+		  if (memcmp (pname, gnu, 4) == 0)
+		    {
+		      const size_t n_descsz = ELFXX_FLD (*nhdr, n_descsz);
+
+		      bil->hex_build_id = xmalloc (n_descsz * 2 + 1);
+		      bin2hex ((gdb_byte*) pname + namesz, bil->hex_build_id,
+					   n_descsz);
+		      xfree (pt_note);
+		      return;
+		    }
+		}
+	      nhdr = (void*) ((gdb_byte *) nhdr + note_sz);
+	    }
+	  xfree (pt_note);
+	}
+    }
+}
+
+/* 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)
+    if (p->offset == 0 && p->inode == rbegin->inode)
+      return p;
+
+  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)
+    {
+      mapping_entry_s *bil_min;
+
+      bil_min = lrfind_mapping_entry (bil, VEC_address (mapping_entry_s,
+							data->list));
+      if (bil_min != NULL)
+	read_build_id (data, bil, bil_min->vaddr, l_addr);
+      else
+	{
+	  /* Do not try to find hex_build_id again.  */
+	  bil->hex_build_id = xstrdup (BUILD_ID_INVALID);
+	  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 +5996,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 +6032,13 @@ 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 (pid, 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 +6113,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 +6122,29 @@ 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
+		  && strcmp (hex_enc_build_id, BUILD_ID_INVALID) != 0)
+		p += sprintf (p, " build-id=\"%s\"", hex_enc_build_id);
+	      p += sprintf(p, "/>");
 	      free (name);
 	    }
 	  else if (lm_prev == 0)
@@ -5819,6 +6179,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