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

[Mark Wielaard <mjw at redhat dot com>]

It seems important to get this cleaned up.
Does the review below make sense?

On Fri, 2015-02-13 at 20:13 +0100, Mark Wielaard wrote:
> On Fri, 2015-02-06 at 12:04 +0530, Hemant Kumar wrote:
> > @@ -2099,7 +2106,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 am still concerned about this lookup overriding the normal
> function_entrypc. It would be good to guard this with a check to see if
> it is necessary (only for a ppc64le target) so no extra lookups are
> necessary in the normal case. But I believe in general it is wrong since
> it only does the name lookup, and doesn't try to see if the address
> matched. Take for example this program:
> 
> ::::::::::::::
> /tmp/baz.c
> ::::::::::::::
> static int
> foo (int v)
> {
>   return v + 1;
> }
> 
> int
> bar (int i)
> {
>   return foo (i - 1);
> }
> 
> ::::::::::::::
> /tmp/main.c
> ::::::::::::::
> int foo (int v)
> {
>   return bar (v - 1);
> }
> 
> int
> main (int argc, char **argv)
> {
>   return foo (argc);
> }
> 
> gcc -g -c baz.c
> gcc -g -c main.c
> gcc -g -o prog baz.o main.o
> 
> This will have two function symbols called "foo" in the symbol table.
> Since you only match on the name, you will likely get one of them wrong.
> You could check before/after your patch with a simple script like:
> 
> stap -e 'process.function("foo") { printf ("%s: %x\n", pp(), uaddr()) }'
> -c ./prog
> 
> > @@ -8154,6 +8173,25 @@ symbol_table::get_from_elf()
> >        reject = reject_section(section);
> >  #endif
> >  
> > +     /*
> > +      * For ELF ABI v2 on PPC64 LE, we need to adjust sym.st_value corresponding
> > +      * to the bits of sym.st_other. These bits will tell us what's the offset
> > +      * of the local entry point from the global entry point.
> > +      */
> > +      Dwarf_Addr bias;
> > +      Elf* elf = (dwarf_getelf (dwfl_module_getdwarf (mod, &bias))
> > +             ?: dwfl_module_getelf (mod, &bias));
> > +
> > +      GElf_Ehdr ehdr_mem;
> > +      GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem);
> > +      if (em == 0) { DWFL_ASSERT ("dwfl_getehdr", dwfl_errno()); }
> > +      assert(em);
> 
> This sanity check seems to confuse a couple of things.
> There are 3 ways this could end up as NULL. Either dwarf_getelf or
> dwfl_module_getelf returned NULL, then gelf_getehdr would return NULL
> also. Or gelf_getehdr returns NULL because it got passed a faulty Elf.
> All cases are very unlikely, because we wouldn't have arrived at this if
> any of those cases were true. DWFL_ASSERT will print only the dwfl_errno
> when set, but not the elf_errno or dwarf_errno. There also isn't
> anything called dwfl_getehdr. Normally DWFL_ASSERT would throw on error,
> so the assert is never reached. I would simply only assert (em). Or if
> you really want to throw an error do:
>  if (em == NULL) throw SEMANTIC_ERROR (_("Couldn't get elf header"));
> 
> Also please move this out of the loop. Just fetch the ehdr once at the
> start.
> 
> > +      /* st_other is currently only used with ABIv2 on ppc64 */
> > +      if ((em->e_machine == EM_PPC64) && (GELF_ST_TYPE(sym.st_info) == STT_FUNC)
> > +	  && sym.st_other)
> > +	addr = addr + PPC64_LOCAL_ENTRY_OFFSET(sym.st_other);
> > +
> >        if (name && GELF_ST_TYPE(sym.st_info) == STT_FUNC)
> >          add_symbol(name, (GELF_ST_BIND(sym.st_info) == STB_WEAK),
> >                     reject, addr, &high_addr);
> 
> Thanks,
> 
> Mark