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

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

[Hemant Kumar]

PPC64 ELF ABI v2 has a Global entry point and a local entry point for the
functions. We need the Local entry point in order to probe these functions.
However, the DIE for these functions in debuginfo return the function.entrypc
which is same as the global entry point. The local entry point is not encoded
in the debuginfo of the ELFs.
The offset to local entry point is however encoded in the st_other field of
these symbols in the symbol table. We need to use this field to adjust the
sym.st_value to actually point to the local entry point instead of the global
entry point.

This patch is in relation to this bug :
https://sourceware.org/bugzilla/show_bug.cgi?id=17638

So, while adding symbols to the sym_table, we add an offset of
PPC64_LOCAL_ENTRY_OFFSET(sym.st_other) to st_value.
And when the function address is queried in query_dwarf_func(), we give priority
to the cached sym_table, where we can retrieve the adjusted entry address of the
function. If we don't get any address from the symbol table, then we proceed to
get from the debuginfo.

Changes since last version:
- Removed the #if defined check for _CALL_ELF.
- Added a definition for PPC64_LOCAL_ENTRY_OFFSET to support for older elf.h.

Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tapsets.cxx |   40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/tapsets.cxx b/tapsets.cxx
index 85fd76b..40ac200 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -68,6 +68,13 @@ extern "C" {
 #define EM_AARCH64 183
 #endif
 
+// PPC64_LOCAL_ENTRY_OFFSET is not defined in older elf.h
+#ifndef PPC64_LOCAL_ENTRY_OFFSET
+#define STO_PPC64_LOCAL_BIT	5
+#define STO_PPC64_LOCAL_MASK	(7 << STO_PPC64_LOCAL_BIT)
+#define PPC64_LOCAL_ENTRY_OFFSET(other)			\
+ (((1 << (((other) & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT)) >> 2) << 2)
+#endif
 
 using namespace std;
 using namespace __gnu_cxx;
@@ -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);
@@ -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);
+
+      /* 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);

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

[Mark Wielaard]

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

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

[Mark Wielaard]

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

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

[Hemant Kumar]

On 03/23/2015 06:55 PM, Mark Wielaard wrote:
> It seems important to get this cleaned up.
> Does the review below make sense?

I replied to this mail sometime back! Probably my mail client's fault.

Working on your review comments. See my replies below.

> 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

You are right! So, will change the map of name to address mapping to a 
map of lists where each name can have multiple func_info's. And we can 
probe on multiple addresses for that name.

>>> @@ -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.

Sure. Will made the changes.

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

Testing the v3 and will send it out asap.

-- 
Thanks,
Hemant Kumar

Re: [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