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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Tue, 09 Apr 2013 17:27:43 +0200, Aleksandar Ristovski wrote:
[...]
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
[...]
> @@ -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))

Macros should not depend on external variables, that makes them tricky, it was
acceptable when the variable was present in the scope where the macro was
defined.  But that is no longer true when the macro is global.

is_elf64 should be a parameter of each of these macros.


> +#define ELFXX_ROUNDUP(what) ((is_elf64) ? (((what) + sizeof (Elf64_Word) - 1) \
> +					   & ~(sizeof (Elf64_Word) - 1))      \
> +					: (((what) + sizeof (Elf32_Word) - 1) \
> +					   & ~(sizeof (Elf32_Word) - 1)))

This is overcomplicated.  The ELF standard defines it as "4-byte alignment".
While both sizeof (Elf64_Word) == 4 and sizeof (Elf32_Word) == 4 I find that
incorrect, the standard talks about 4 bytes (for both elf32 and elf64), not
about sizeof of anything.


> +#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,43 @@ get_phdr_phnum_from_proc_auxv (const int pid, const int is_elf64,
>    return 0;
>  }
>  
> +/* 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.  */

There is no predicate function anymore.


> +
> +static const void *
> +find_phdr (const int is_elf64, const void *const phdr_begin,
> +	   const void *const phdr_end, const ULONGEST p_type)
> +{
> +#define PHDR_NEXT(hdrp) ((const void *) ((const gdb_byte *) (hdrp) + \
> +			 ELFXX_SIZEOF (*hdrp)))
> +
> +  const ElfXX_Phdr *phdr = phdr_begin;
> +


> +  if (phdr == NULL)
> +    return NULL;

I do not see a reason for this check, callers never pass it NULL.   It should
be rather gdb_assert in such case if anything.


> +
> +  while (PHDR_NEXT (phdr) <= phdr_end)
> +    {
> +      if (ELFXX_FLD (*phdr, p_type) == p_type)
> +	return phdr;
> +      phdr = PHDR_NEXT (phdr);
> +    }
> +
> +  return NULL;
> +#undef PHDR_NEXT
> +}
> +
>  /* 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;
>  
>    if (get_phdr_phnum_from_proc_auxv (pid, is_elf64, &phdr_memaddr, &num_phdr))
>      return 0;
> @@ -5454,21 +5511,23 @@ 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);
> +  phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
> +		    PT_PHDR);
> +  if (phdr != NULL)
> +    {
> +      if (is_elf64)
> +	{
> +	  const Elf64_Phdr *const p = phdr;

When the 32-vs-64 ELF framework is provided here I expected this existing code
could be simplified on top of it.  Provided such patch at the end of this
mail.


>  
> -	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 +5544,22 @@ get_dynamic (const int pid, const int is_elf64)
>        return 0;
>      }
>  
> -  for (i = 0; i < num_phdr; i++)
> +  phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
> +		    PT_DYNAMIC);
> +
> +  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 +5701,253 @@ 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)

It does not free mapping_entry.  It frees the vector of them.  Therefore it
should be name for example free_mapping_entry_vec.  And the function is
missing comment.


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

GNU Coding Standards:
	const ULONGEST key = *(CORE_ADDR *) k;
And when it is all const it should not be temporarily de-const-ed:
	const ULONGEST key = *(const 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.  */

free_mapping_entry name should be updated after the rename above.


> +  VEC (mapping_entry_s) *list;
> +};
> +
> +static linux_find_memory_region_ftype find_memory_region_callback;

Why is it here?  It should be before the find_memory_region_callback
definition.


> +
> +/* Read build-id from PT_NOTE.  */

Describe function parameters.  It is essential to see the LOAD_ADDR vs. L_ADDR
difference.


> +
> +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 unsigned e_phentsize = ELFXX_FLD (ehdr, e_phentsize);
> +
> +      gdb_assert (ELFXX_FLD (ehdr, e_phnum) < 100);  /* Basic sanity check.  */

I am aware a similar gdb_assert is in get_dynamic but that is a bug.
gdbserver should not crash on weird inferior data.
There should be a warning + return.

Also comments should be on their own line:
      /* Basic sanity check.  */
      gdb_assert (ELFXX_FLD (ehdr, e_phnum) < 100);


> +      gdb_assert (e_phentsize == ELFXX_SIZEOF (*phdr));

Likewise.


> +      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,
> +			    PT_NOTE);
> +	  if (phdr == NULL)
> +	    break;
> +	  pt_note = xmalloc (ELFXX_FLD (*phdr, p_memsz));
> +	  pt_end = (gdb_byte*) pt_note + ELFXX_FLD (*phdr, p_memsz);

GNU Coding Standards:
	  pt_end = (gdb_byte *) pt_note + ELFXX_FLD (*phdr, p_memsz);
But pt_note is already gdb_byte * so that cast is useless, therefore:
	  pt_end = 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.");

Print also the note address when there is a warning at all.


> +	      break;
> +	    }
> +
> +	  nhdr = (void *) pt_note;
> +	  while ((void *) nhdr < (void *) pt_end)

When it is all const there should be (const void *).  But in fact it is easier
to use const gdb_byte * when pt_end is already such type:
	  while ((const gdb_byte *) nhdr < 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

It should be (const gdb_byte *) because nhdr is already const *.


> +		  || descsz == 0)
> +		{
> +		  warning ("Malformed PT_NOTE\n");

Print also the note address when there is a warning at all.


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

It should be (const char *) because nhdr is already const *.


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

It should be (const gdb_byte *) because pname is already const *.

Additionally according to the GNU Coding Standards spacing it should be:
		      bin2hex ((const 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.  */

When the line
	static linux_find_memory_region_ftype find_memory_region_callback;
will be here the parameters would be described.  But still one could write:
	/* Add mapping_entry.  See linux_find_memory_region_ftype for the
	   parameters description.  */


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

Maybe one could describe more what is "L_LD".
	L_LD is the link_map.l_ld field from libc shared library list.

And that L_ADDR parameter is also:
	L_ADDR is the link_map.l_addr field from libc shared library list.


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

I do not think this check should be / needs to be here.  bsearch with NMEMB ==
0 should 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.");

You should print some identification for troubleshooting when the warning is
there at all, probably L_LD here is the best one.


> +	}
> +    }
> +
> +  return bil->hex_build_id;
> +}
> +
>  /* Construct qXfer:libraries-svr4:read reply.  */
>  
>  static int
> @@ -5653,6 +5960,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 +5996,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 +6077,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 +6086,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;
>  		}

This "code cleanup" change is unrelated to this patch.  But it is IMO not
worth checking in separately, it does not improve it anyhow IMO.


>  
> -	      name = xml_escape_text ((char *) libname);

Why did you move this line to several lines above?  It is a needless change.


>  	      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, "/>");

GNU Coding Standards:
	      p += sprintf (p, "/>");


>  	      free (name);
>  	    }
>  	  else if (lm_prev == 0)
> @@ -5819,6 +6143,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


diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index d819b21..cf5be8c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5465,7 +5465,7 @@ get_phdr_phnum_from_proc_auxv (const int pid, const int is_elf64,
    predicate function returns 1.  If supplied predicate function
    did return 1, stop traversal and return that PHDR.  */
 
-static const void *
+static const ElfXX_Phdr *
 find_phdr (const int is_elf64, const void *const phdr_begin,
 	   const void *const phdr_end, const ULONGEST p_type)
 {
@@ -5496,8 +5496,8 @@ get_dynamic (const int pid, const int is_elf64)
   CORE_ADDR phdr_memaddr, relocation;
   int num_phdr;
   unsigned char *phdr_buf;
-  const int phdr_size = is_elf64 ? sizeof (Elf64_Phdr) : sizeof (Elf32_Phdr);
-  const void *phdr;
+  const ElfXX_Phdr *phdr;
+  const int phdr_size = ELFXX_SIZEOF (*phdr);
 
   if (get_phdr_phnum_from_proc_auxv (pid, is_elf64, &phdr_memaddr, &num_phdr))
     return 0;
@@ -5510,26 +5510,9 @@ 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;
   phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
 		    PT_PHDR);
-  if (phdr != NULL)
-    {
-      if (is_elf64)
-	{
-	  const Elf64_Phdr *const p = phdr;
-
-	  relocation = phdr_memaddr - p->p_vaddr;
-	}
-      else
-	{
-	  const Elf32_Phdr *const p = phdr;
-
-	  relocation = phdr_memaddr - p->p_vaddr;
-	}
-    }
-
-  if (relocation == -1)
+  if (phdr == NULL)
     {
       /* PT_PHDR is optional, but necessary for PIE in general.  Fortunately
 	 any real world executables, including PIE executables, have always
@@ -5543,27 +5526,13 @@ get_dynamic (const int pid, const int is_elf64)
 
       return 0;
     }
+  relocation = phdr_memaddr - ELFXX_FLD (*phdr, p_vaddr);
 
   phdr = find_phdr (is_elf64, phdr_buf, phdr_buf + num_phdr * phdr_size,
 		    PT_DYNAMIC);
-
-  if (phdr != NULL)
-    {
-      if (is_elf64)
-	{
-	  const Elf64_Phdr *const p = phdr;
-
-	  return p->p_vaddr + relocation;
-	}
-      else
-	{
-	  const Elf32_Phdr *const p = phdr;
-
-	  return p->p_vaddr + relocation;
-	}
-    }
-
-  return 0;
+  if (phdr == NULL)
+    return 0;
+  return ELFXX_FLD (*phdr, p_vaddr) + relocation;
 }
 
 /* Return &_r_debug in the inferior, or -1 if not present.  Return value