Re: [PATCH][gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

[Pedro Alves <palves@redhat.com>]

On 8/9/19 4:03 PM, Tom de Vries wrote:

>  
> +/* Given an unrelocated address ADDR belonging to the text section of OBJFILE,
> +   return the relocated address.  */
> +
> +static CORE_ADDR
> +relocate_text_addr (CORE_ADDR addr, struct objfile *objfile)
> +{
> +  CORE_ADDR baseaddr
> +    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
> +  struct gdbarch *gdbarch = get_objfile_arch (objfile);
> +  addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
> +  return addr;

I'd write:

 return gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);

> +}
> +
>  /* Find PC to be unwound from THIS_FRAME.  THIS_FRAME must be a part of
>     CACHE.  */
>  
> @@ -240,14 +255,25 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache)
>    gdb_assert (next_levels >= 0);
>  
>    if (next_levels < chain->callees)
> -    return chain->call_site[chain->length - next_levels - 1]->pc;
> +    {
> +      struct call_site *call_site
> +	= chain->call_site[chain->length - next_levels - 1];
> +      struct objfile *objfile = call_site->per_cu->dwarf2_per_objfile->objfile;
> +      return relocate_text_addr (call_site->pc, objfile);
> +    }
>    next_levels -= chain->callees;
>  
>    /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS.  */
>    if (chain->callees != chain->length)
>      {
>        if (next_levels < chain->callers)
> -	return chain->call_site[chain->callers - next_levels - 1]->pc;
> +	{
> +	  struct call_site *call_site
> +	    = chain->call_site[chain->callers - next_levels - 1];
> +	  struct objfile *objfile
> +	    = call_site->per_cu->dwarf2_per_objfile->objfile;
> +	  return relocate_text_addr (call_site->pc, objfile);
> +	}

That seems fine, but it seems you could have factored out more, like:

static CORE_ADDR
call_site_relocated_pc (struct call_site *call_site)
{
  struct objfile *objfile
    = call_site->per_cu->dwarf2_per_objfile->objfile;
  CORE_ADDR baseaddr
    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  struct gdbarch *gdbarch = get_objfile_arch (objfile);
  return gdbarch_adjust_dwarf2_addr (gdbarch, call_size->pc + baseaddr);
}

Then the other hunks would look like:

  struct call_site *call_site
    = chain->call_site[chain->length - next_levels - 1];
  return call_site_relocated_pc (call_site);

...

  struct call_site *call_site
    = chain->call_site[chain->callers - next_levels - 1];
  return call_site_relocated_pc (call_site);

call_site_relocated_pc could even be a method of struct call_site, I guess,
so you'd write:

  return call_site->relocated_pc ();

Any reason you didn't do something like that?

Thanks,
Pedro Alves