This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


>  @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.


> +/* 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.

> +
> +/* 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.


> +#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)))


> +
> +  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.


> +
> +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.


> +{
> +  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.  */


> +  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.


> +
> +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.


> +  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.

find_phdr_p probably should not be passed, as discussed above.

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.
+
	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.


> +		{
> +		  warning ("Malformed PT_NOTE\n");
> +		  break;
> +		}

You need to check here also the name content, it is "GNU\x00" (n_namesz == 4).


> +	      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.


> +		  xfree (pt_note);
> +		  return;
> +		}
> +	      nhdr = (void*) ((char *) nhdr + note_sz);

I wrote last time:
GNU Coding Standard formatting + simplification:
                  nhdr = (void *) nhdr + note_sz;


> +	    }
> +	  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.


> +#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.


> +
> +  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));

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.


> +	}
> +    }
> +
> +  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.

> +	lwpid_of (get_thread_lwp (current_inferior)),

When the parameters are too long for proper indentation use a temporary
variable for the value.


> +	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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]