Re: [RFC PATCH] Fix PPC64 ELF ABI v2 symbol address retrieval

[Mark Wielaard <mjw at redhat dot com>]

On Mon, 2015-02-02 at 18:00 +0530, Hemant Kumar wrote:
> On 01/23/2015 07:30 PM, Mark Wielaard wrote:
> >> diff --git a/tapsets.cxx b/tapsets.cxx
> >> index 85fd76b..d1382e4 100644
> >> --- a/tapsets.cxx
> >> +++ b/tapsets.cxx
> >> @@ -2099,7 +2099,19 @@ query_dwarf_func (Dwarf_Die * func, dwarf_query * q)
> >>             q->dw.function_line (&func.decl_line);
> >>   
> >>             Dwarf_Addr entrypc;
> >> -          if (q->dw.function_entrypc (&entrypc))
> >> +          func.entrypc = 0;
> >> +          /* Giving priority to sym_table */
> >> +          if (q->dw.mod_info->sym_table)
> >> +            {
> >> +              func_info * fi;
> >> +              fi = q->dw.mod_info->sym_table->lookup_symbol(func.name);
> >> +              if (fi)
> >> +                {
> >> +                  func.entrypc = fi->addr;
> >> +                  q->filtered_functions.push_back(func);
> >> +                }
> >> +            }
> >> +          if (!func.entrypc && q->dw.function_entrypc (&entrypc))
> >>               {
> >>                 func.entrypc = entrypc;
> >>                 q->filtered_functions.push_back (func);
> > I think this should be the other way around. q->dw.function_entrypc ()
> > will normally do the right thing (and take DW_AT_entrypc into account,
> > which might be important for some cases, even if it isn't currently used
> > for the PPC64 ELF ABI v2 case). Then when you do get the func.entrypc
> > and func.name you look the name up in the symbol table and adjust it if
> > the entrypc matches the symbol value.
> If we do this the other way around, func.entrypc and the value from the 
> symbol table won't match here, because the query from the symbol table 
> returns the adjusted value of the symbol (value from st_other field 
> already added in the rest of the patch below in 
> symbol_table::get_from_elf()).
> 
> Please correct me if I misunderstood something here.

You are right. I had not realized that the sym_table was completely
preprocessed ahead of time, and not during the lookup_symbol call. I
thought it would iterate over the symbol table. Which would be bad,
because that is slow. So that was one of my concerns. I do think we
should find a way to not do lookup_symbol unless really necessary. It
isn't as slow as I thought, but it is an unnecessary thing in most
cases.

BTW. See the comment in the code dwfl_module_getsym_info does leave
st_value alone and returns the adjusted address separately, so it isn't
adjusted in that case (but that needs elfutils 0.158+).