On 09-08-19 19:38, Pedro Alves wrote:
> 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);
> 

Updated accordingly.

>> +}
>> +
>>  /* 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?

I went for a utility function that can be maximally reused, as opposed
to a function that maximally factors out the code in this particular
patch. But we can do both, so this patch adds:
- relocate_text_addr (I've moved it to objfiles.h/objfiles.c)
- call_site::relocated_pc

[ BTW, I should mention that this fix is dependent on the patch "[gdb]
Fix gdb.dwarf2/amd64-entry-value-param.exp with -fPIE/-pie" (
https://sourceware.org/ml/gdb-patches/2019-08/msg00215.html ). ]

OK like this?

Thanks,
- Tom