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

[RFC PATCH] 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.

Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
---
 tapsets.cxx |   34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

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);
@@ -8154,6 +8166,26 @@ 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.
+ */
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+      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);
+
+      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);
+#endif
+
       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: [RFC PATCH] Fix PPC64 ELF ABI v2 symbol address retrieval

[Mark Wielaard]

On Fri, 2015-01-16 at 17:24 +0530, Hemant Kumar wrote:
> 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.

BTW. Would it make sense to let GCC encode this in the DWARF output?
DWARF 4, 2.18 Entry Address says:

        Any debugging information entry describing an entity that has a
        range of code addresses, which includes compilation units,
        module initialization, subroutines, ordinary blocks, try/catch
        blocks, and the like, may have a DW_AT_entry_pc attribute to
        indicate the first executable instruction within that range of
        addresses. The value of the DW_AT_entry_pc attribute is a
        relocated address. If no DW_AT_entry_pc attribute is present,
        then the entry address is assumed to be the same as the value of
        the DW_AT_low_pc attribute, if present; otherwise, the entry
        address is unknown.

Which seems to describe the situation exactly. And would make elfutils
libdw dwarf_entrypc () do the right thing automagically (it will fall
back to low_pc as described) without needing any extra lookups through
symbol tables.

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

The function_entrypc () part looks good (see some small nitpicks below).
But I think the priority/fixup needs to be done the other way around
(see also below).

> Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
> ---
>  tapsets.cxx |   34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> 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.

> @@ -8154,6 +8166,26 @@ 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.
> + */
> +#if defined(_CALL_ELF) && _CALL_ELF == 2

This won't work correctly when stap is ran remotely on a different
architecture. That is not a very common situation, but I believe we do
want to support that. So you will just have to always inspect the Elf
ehdr.

> +      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);
> +
> +      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);

Here you also want to have a check for EF_PPC64_ABI in e_flags then.

Note that older elf.h files won't have those defines, so you should
either check they are defined or explicitly define them here to make
sure things will keep building on older systems.

> +#endif
> +
>        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: [RFC PATCH] Fix PPC64 ELF ABI v2 symbol address retrieval

[Ulrich Weigand]

Mark Wielaard <mjw@redhat.com> wrote on 23.01.2015 15:00:35:

> BTW. Would it make sense to let GCC encode this in the DWARF output?
> DWARF 4, 2.18 Entry Address says:
>
>         Any debugging information entry describing an entity that has a
>         range of code addresses, which includes compilation units,
>         module initialization, subroutines, ordinary blocks, try/catch
>         blocks, and the like, may have a DW_AT_entry_pc attribute to
>         indicate the first executable instruction within that range of
>         addresses. The value of the DW_AT_entry_pc attribute is a
>         relocated address. If no DW_AT_entry_pc attribute is present,
>         then the entry address is assumed to be the same as the value of
>         the DW_AT_low_pc attribute, if present; otherwise, the entry
>         address is unknown.
>
> Which seems to describe the situation exactly. And would make elfutils
> libdw dwarf_entrypc () do the right thing automagically (it will fall
> back to low_pc as described) without needing any extra lookups through
> symbol tables.

Well, not really.  The global entry point is also a possible entry point,
and there are exectuable instructions between the global and the local
entry point as well.  DW_AT_entry_pc is intended for the case where
execution *always* starts at some other place than DW_AT_low_pc.  There
is no provision in DWARF right now to describe that a function has
*multiple* possible entry points.

We chose to have the DWARF address range cover the whole function
(including both entry points), and use the global entry point as
the "DWARF entry point", since that seemed to work best for most
users of that information.  For example, GDB relies on that behaviour
(e.g. if you look up the current function by PC, you want to find
the function even if you stopped in between the two entry points).

Some few use cases (like yours) do require the local entry point, but
you can always find the local entry point via the symbol table if you
know the global entry point (but not necessarily vice versa).

Bye,
Ulrich

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

[Mark Wielaard]

On Fri, 2015-01-23 at 16:17 +0100, Ulrich Weigand wrote:
> Mark Wielaard <mjw@redhat.com> wrote on 23.01.2015 15:00:35:
> 
> > BTW. Would it make sense to let GCC encode this in the DWARF output?
> > DWARF 4, 2.18 Entry Address says:
> >
> >         Any debugging information entry describing an entity that has a
> >         range of code addresses, which includes compilation units,
> >         module initialization, subroutines, ordinary blocks, try/catch
> >         blocks, and the like, may have a DW_AT_entry_pc attribute to
> >         indicate the first executable instruction within that range of
> >         addresses. The value of the DW_AT_entry_pc attribute is a
> >         relocated address. If no DW_AT_entry_pc attribute is present,
> >         then the entry address is assumed to be the same as the value of
> >         the DW_AT_low_pc attribute, if present; otherwise, the entry
> >         address is unknown.
> >
> > Which seems to describe the situation exactly. And would make elfutils
> > libdw dwarf_entrypc () do the right thing automagically (it will fall
> > back to low_pc as described) without needing any extra lookups through
> > symbol tables.
> 
> Well, not really.  The global entry point is also a possible entry point,
> and there are exectuable instructions between the global and the local
> entry point as well.  DW_AT_entry_pc is intended for the case where
> execution *always* starts at some other place than DW_AT_low_pc.  There
> is no provision in DWARF right now to describe that a function has
> *multiple* possible entry points.

That is a strict reading, but you are right. The execution can indeed
start at either point. So an attribute on the subroutine might be the
wrong thing to use. What about using a DW_TAG_entry_point (DWARF 3.3 "An
alternate entry point") as child of the DW_TAG_subroutine to describe
the local entry point?

> We chose to have the DWARF address range cover the whole function
> (including both entry points), and use the global entry point as
> the "DWARF entry point", since that seemed to work best for most
> users of that information.  For example, GDB relies on that behaviour
> (e.g. if you look up the current function by PC, you want to find
> the function even if you stopped in between the two entry points).

Yes, it is certainly a good choice to make the subprogram DIE cover the
whole function (including both entry points).

> Some few use cases (like yours) do require the local entry point, but
> you can always find the local entry point via the symbol table if you
> know the global entry point (but not necessarily vice versa).

But I do think it is a fairly common use case. Users often want to make
sure they can monitor all calls to a particular function. The lookup
through the symbol table is possible, but not very efficient. Because
there is no direct link between the two, just an indirect match by
checking the sym value and name against the DIE low_pc and (mangled)
name. Although I admit that is partly because elfutils doesn't provide
an efficient symbol table search, just a linear one (I do have to fix
that).

Cheers,

Mark

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

[Hemant Kumar]

Hi Mark,

On 01/23/2015 07:30 PM, Mark Wielaard wrote:
> On Fri, 2015-01-16 at 17:24 +0530, Hemant Kumar wrote:
>> 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.
> BTW. Would it make sense to let GCC encode this in the DWARF output?
> DWARF 4, 2.18 Entry Address says:
>
>          Any debugging information entry describing an entity that has a
>          range of code addresses, which includes compilation units,
>          module initialization, subroutines, ordinary blocks, try/catch
>          blocks, and the like, may have a DW_AT_entry_pc attribute to
>          indicate the first executable instruction within that range of
>          addresses. The value of the DW_AT_entry_pc attribute is a
>          relocated address. If no DW_AT_entry_pc attribute is present,
>          then the entry address is assumed to be the same as the value of
>          the DW_AT_low_pc attribute, if present; otherwise, the entry
>          address is unknown.
>
> Which seems to describe the situation exactly. And would make elfutils
> libdw dwarf_entrypc () do the right thing automagically (it will fall
> back to low_pc as described) without needing any extra lookups through
> symbol tables.
>
>> 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.
> The function_entrypc () part looks good (see some small nitpicks below).
> But I think the priority/fixup needs to be done the other way around
> (see also below).
>
>> Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
>> ---
>>   tapsets.cxx |   34 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> 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.

>> @@ -8154,6 +8166,26 @@ 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.
>> + */
>> +#if defined(_CALL_ELF) && _CALL_ELF == 2
> This won't work correctly when stap is ran remotely on a different
> architecture. That is not a very common situation, but I believe we do
> want to support that. So you will just have to always inspect the Elf
> ehdr.

Yes. Thanks for pointing that out. Will remove the #if defined statements.

>> +      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);
>> +
>> +      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);
> Here you also want to have a check for EF_PPC64_ABI in e_flags then.

Ok.

> Note that older elf.h files won't have those defines, so you should
> either check they are defined or explicitly define them here to make
> sure things will keep building on older systems.

Right. Will add the required checks.

>> +#endif
>> +
>>         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 a lot for reviewing this patch. Will make the required changes.

-- 
Hemant Kumar

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

[Ulrich Weigand]

Mark Wielaard <mjw@redhat.com> wrote on 26.01.2015 15:09:08:

> That is a strict reading, but you are right. The execution can indeed
> start at either point. So an attribute on the subroutine might be the
> wrong thing to use. What about using a DW_TAG_entry_point (DWARF 3.3 "An
> alternate entry point") as child of the DW_TAG_subroutine to describe
> the local entry point?

Well, this DWARF feature also doesn't really match very well to the
PowerPC situation.  DW_TAG_entry_point is supposed to be used for
languages (like some Fortran versions, I think) that support multiple
entry point *at the source level*, where the alternate entry point has
its own name and argument list.  This is why DW_TAG_entry_point requires
its own DW_AT_name attribute, and needs to provide its own argument and
return type information ...

While of course it might be possible to create all that information for
an ELFv2 local entry point (synthesizing a name and duplicating the
type information), that seems needlessly complex.

As as I said before, I don't think there is any feature in DWARF4 that
is intended to allow specifying something like ELFv2 local entry points.
I guess there would be one way or another to do so by non-standard use
of some feature, or by making use of a platform-specific extension;
one relatively straightforward way would be to use a set of private
values of the DW_AT_calling_convention attribute to specify the entry
point offset values that can be encoded in st_other.

However, given that at this stage we already probably have on the order
of 100000 binaries in the field that were compiled without that
information, and none of the existing compilers in production use
(which is not just GCC, but at least also LLVM and XLC) produces that
information, I'm wondering how useful it would be to make such a change
at this stage.  Tools would have to implement the fallback to look at the
symbol table anyway to cope with existing binaries, and once that is
implemented, having the information duplicated in DWARF doesn't really
buy you anything ...

> > Some few use cases (like yours) do require the local entry point, but
> > you can always find the local entry point via the symbol table if you
> > know the global entry point (but not necessarily vice versa).
>
> But I do think it is a fairly common use case. Users often want to make
> sure they can monitor all calls to a particular function. The lookup
> through the symbol table is possible, but not very efficient. Because
> there is no direct link between the two, just an indirect match by
> checking the sym value and name against the DIE low_pc and (mangled)
> name. Although I admit that is partly because elfutils doesn't provide
> an efficient symbol table search, just a linear one (I do have to fix
> that).

That would seem useful anyway, there's other interesting info in the
symbol table (like symbol visibility etc.).

Bye,
Ulrich

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

[Hemant Kumar]

On 01/23/2015 07:30 PM, Mark Wielaard wrote:
> On Fri, 2015-01-16 at 17:24 +0530, Hemant Kumar wrote:
>> 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.
> BTW. Would it make sense to let GCC encode this in the DWARF output?
> DWARF 4, 2.18 Entry Address says:
>
>          Any debugging information entry describing an entity that has a
>          range of code addresses, which includes compilation units,
>          module initialization, subroutines, ordinary blocks, try/catch
>          blocks, and the like, may have a DW_AT_entry_pc attribute to
>          indicate the first executable instruction within that range of
>          addresses. The value of the DW_AT_entry_pc attribute is a
>          relocated address. If no DW_AT_entry_pc attribute is present,
>          then the entry address is assumed to be the same as the value of
>          the DW_AT_low_pc attribute, if present; otherwise, the entry
>          address is unknown.
>
> Which seems to describe the situation exactly. And would make elfutils
> libdw dwarf_entrypc () do the right thing automagically (it will fall
> back to low_pc as described) without needing any extra lookups through
> symbol tables.
>
>> 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.
> The function_entrypc () part looks good (see some small nitpicks below).
> But I think the priority/fixup needs to be done the other way around
> (see also below).
>
>> Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
>> ---
>>   tapsets.cxx |   34 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> 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.
>
>> @@ -8154,6 +8166,26 @@ 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.
>> + */
>> +#if defined(_CALL_ELF) && _CALL_ELF == 2
> This won't work correctly when stap is ran remotely on a different
> architecture. That is not a very common situation, but I believe we do
> want to support that. So you will just have to always inspect the Elf
> ehdr.
>
>> +      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);
>> +
>> +      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);
> Here you also want to have a check for EF_PPC64_ABI in e_flags then.
>
> Note that older elf.h files won't have those defines, so you should
> either check they are defined or explicitly define them here to make
> sure things will keep building on older systems.
>
>> +#endif
>> +
>>         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
>

Sent a v2 for this patch.

-- 
Thanks,
Hemant Kumar

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

[Mark Wielaard]

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+).