Re: [PATCH 2/3] relocate the entry point addess when used

[Pedro Alves <palves at redhat dot com>]

On 01/06/2014 08:30 PM, Tom Tromey wrote:
> This changes the entry point to be unrelocated in the objfile, and
> instead applies the relocation when it is used.
> 
> I think the existing code here is wrong.  It computes the entry point
> address directly from the BFD, not applying any runtime offsets.
> However, then objfile_relocate1 passes this address to find_pc_section
> -- which does use the offsets .  So, it seems to me that the current
> code can only find the correct address by luck.

It's twisted, but I don't think it's luck.  You can convince yourself
it works by debugging a PIE, and trying a backtrace past main
("set backtrace past-main" will then trigger the code to stop the
backtrace at the entry point), or doing "info files" (shows the entry).

Thing is, find_pc_section uses obj_section_addr/obj_section_endaddr
for comparison:

/* Bsearch comparison function.  */

static int
bsearch_cmp (const void *key, const void *elt)
{
  const CORE_ADDR pc = *(CORE_ADDR *) key;
  const struct obj_section *section = *(const struct obj_section **) elt;

  if (pc < obj_section_addr (section))
    return -1;
  if (pc < obj_section_endaddr (section))
    return 0;
  return 1;
}

And obj_section_addr indeed applies the offsets:

/* Relocation offset applied to S.  */
#define obj_section_offset(s)						\
  (((s)->objfile->section_offsets)->offsets[gdb_bfd_section_index ((s)->objfile->obfd, (s)->the_bfd_section)])

/* The memory address of section S (vma + offset).  */
#define obj_section_addr(s)				      		\
  (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section)		\
   + obj_section_offset (s))

/* The one-passed-the-end memory address of section S
   (vma + size + offset).  */
#define obj_section_endaddr(s)						\
  (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section)		\
   + bfd_get_section_size ((s)->the_bfd_section)			\
   + obj_section_offset (s))


But, objfile_relocate1 only sets the offsets to the obj_sections
_after_ looking up the entry point:

  if (objfile->ei.entry_point_p)
    {
      /* Relocate ei.entry_point with its section offset, use SECT_OFF_TEXT
	 only as a fallback.  */
      struct obj_section *s;
      s = find_pc_section (objfile->ei.entry_point);
      if (s)
	{
	  int idx = gdb_bfd_section_index (objfile->obfd, s->the_bfd_section);

	  objfile->ei.entry_point += ANOFFSET (delta, idx);
	}
      else
        objfile->ei.entry_point += ANOFFSET (delta, SECT_OFF_TEXT (objfile));
    }

  {
    int i;

    for (i = 0; i < objfile->num_sections; ++i)
      (objfile->section_offsets)->offsets[i] = ANOFFSET (new_offsets, i);
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  }

So find_pc_section is still using the unrelocated section addresses
when looking up the entry point's unrelocated address.

> +
> +      ALL_OBJFILE_OSECTIONS (objfile, osect)
> +	{
> +	  struct bfd_section *sect = osect->the_bfd_section;
> +
> +	  if (entry_point >= bfd_get_section_vma (objfile->obfd, sect)
> +	      && entry_point < (bfd_get_section_vma (objfile->obfd, sect)
> +				+ bfd_get_section_size (sect)))
> +	    {
> +	      objfile->ei.the_bfd_section_index
> +		= gdb_bfd_section_index (objfile->obfd, sect);
> +	      break;
> +	    }
> +	}
> +
> +      if (osect == NULL)

This is assuming osect ends up NULL after iterating over all.
It's violating the abstraction of the macro.  And, actually,
it's wrong, showing exactly why such assumptions are a bad idea:

#define ALL_OBJFILE_OSECTIONS(objfile, osect)	\
  for (osect = objfile->sections; osect < objfile->sections_end; osect++) \
    if (osect->the_bfd_section == NULL)					\
      {									\
	/* Nothing.  */							\
      }									\
    else

OSECT doesn't end up NULL at the end of the iteration at all.

It's better to, make the "break" above when the section is found
be a return instead.

> +	objfile->ei.the_bfd_section_index = SECT_OFF_TEXT (objfile);
>      }
>  }
>  
> 


-- 
Pedro Alves