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

[Hemant Kumar]

Hey, posted the v3.


On 03/23/2015 06:55 PM, Mark Wielaard wrote:
> 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

-- 
Thanks,
Hemant Kumar