[PATCH v4 1/3] systemtap/tapsets.cxx: Fix dwarfless probes on multiple static functions

[PATCH v4 1/3] systemtap/tapsets.cxx: Fix dwarfless probes on multiple static functions

[Hemant Kumar]

With multiple static functions with same names in an ELF and in absence
of dwarf, if we probe on one of the functions, then systemtap places
probe only on one static function ignoring the rest. This is because the
mapping between the symbol names and their func_info is a simple map
which doesn't allow insertion of another symbol with the same name.

This patch fixes this issue by changing this map to a multimap which
allows duplicate entries for the same symbol name. lookup_symbol code
will return a set of func_info * instead of a single descriptor for a
function name.

We also need to fix other areas in the code where lookup_symbol() and
lookup_symbol_address() are being called so as to look for a set of
func_info's and a list of Dwarf_Addr's respectively, instead of a single
descriptor.

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

diff --git a/tapsets.cxx b/tapsets.cxx
index 443fb2e..c96c542 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -395,7 +395,7 @@ struct
 symbol_table
 {
   module_info *mod_info;	// associated module
-  map<string, func_info*> map_by_name;
+  multimap<string, func_info*> map_by_name;
   multimap<Dwarf_Addr, func_info*> map_by_addr;
   map<string, Dwarf_Addr> globals;
   map<string, Dwarf_Addr> locals;
@@ -410,8 +410,8 @@ symbol_table
   void prepare_section_rejection(Dwfl_Module *mod);
   bool reject_section(GElf_Word section);
   void purge_syscall_stubs();
-  func_info *lookup_symbol(const string& name);
-  Dwarf_Addr lookup_symbol_address(const string& name);
+  set <func_info*> *lookup_symbol(const string& name);
+  list <Dwarf_Addr> *lookup_symbol_address(const string& name);
   func_info *get_func_containing_address(Dwarf_Addr addr);
   func_info *get_first_func();
 
@@ -1113,9 +1113,15 @@ dwarf_query::query_module_symtab()
         }
       else
         {
-          fi = sym_table->lookup_symbol(function_str_val);
-          if (fi && !fi->descriptor && null_die(&fi->die))
-	     query_symtab_func_info(*fi, this);
+          set<func_info*> *fis = sym_table->lookup_symbol(function_str_val);
+          if (!fis || fis->empty())
+            return;
+          for (set<func_info*>::iterator it=fis->begin(); it!=fis->end(); ++it)
+            {
+              fi = *it;
+              if (fi && !fi->descriptor && null_die(&fi->die))
+                query_symtab_func_info(*fi, this);
+            }
         }
     }
 }
@@ -7484,8 +7490,8 @@ suggest_dwarf_functions(systemtap_session& sess,
       // add all function symbols in cache
       if (module->symtab_status != info_present || module->sym_table == NULL)
         continue;
-      map<string, func_info*>& modfuncs = module->sym_table->map_by_name;
-      for (map<string, func_info*>::const_iterator itfuncs = modfuncs.begin();
+      multimap<string, func_info*>& modfuncs = module->sym_table->map_by_name;
+      for (multimap<string, func_info*>::const_iterator itfuncs = modfuncs.begin();
            itfuncs != modfuncs.end(); ++itfuncs)
         funcs.insert(itfuncs->first);
     }
@@ -8072,9 +8078,8 @@ symbol_table::add_symbol(const char *name, bool weak, bool descriptor,
   fi->name = name;
   fi->weak = weak;
   fi->descriptor = descriptor;
-  map_by_name[fi->name] = fi;
-  // TODO: Use a multimap in case there are multiple static
-  // functions with the same name?
+
+  map_by_name.insert(make_pair(fi->name, fi));
   map_by_addr.insert(make_pair(addr, fi));
 }
 
@@ -8194,22 +8199,32 @@ symbol_table::get_first_func()
   return (iter)->second;
 }
 
-func_info *
+set <func_info*> *
 symbol_table::lookup_symbol(const string& name)
 {
-  map<string, func_info*>::iterator i = map_by_name.find(name);
-  if (i == map_by_name.end())
-    return NULL;
-  return i->second;
+  set<func_info*> *fis = new set<func_info*>;
+  pair <multimap<string, func_info*>::iterator, multimap<string, func_info*>::iterator> ret;
+  ret = map_by_name.equal_range(name);
+
+  for (multimap<string, func_info*>::iterator it = ret.first; it != ret.second; ++it)
+    fis->insert(it->second);
+
+  return fis;
 }
 
-Dwarf_Addr
+list <Dwarf_Addr> *
 symbol_table::lookup_symbol_address(const string& name)
 {
-  func_info *fi = lookup_symbol(name);
-  if (fi)
-    return fi->addr;
-  return 0;
+  list <Dwarf_Addr> *addrs = new list<Dwarf_Addr>;
+  set <func_info*> *fis = lookup_symbol(name);
+
+  if (!fis || fis->empty())
+    return NULL;
+
+  for (set<func_info*>::iterator it=fis->begin(); it!=fis->end(); ++it)
+    addrs->push_back((*it)->addr);
+
+  return addrs;
 }
 
 // This is the kernel symbol table.  The kernel macro cond_syscall creates
@@ -8223,9 +8238,12 @@ symbol_table::lookup_symbol_address(const string& name)
 void
 symbol_table::purge_syscall_stubs()
 {
-  Dwarf_Addr stub_addr = lookup_symbol_address("sys_ni_syscall");
-  if (stub_addr == 0)
+  list<Dwarf_Addr> *addrs = lookup_symbol_address("sys_ni_syscall");
+  if (!addrs || addrs->empty())
     return;
+  /* Highly unlikely that multiple symbols named "sys_ni_syscall" may exist */
+  Dwarf_Addr stub_addr = addrs->front();
+
   range_t purge_range = map_by_addr.equal_range(stub_addr);
   for (iterator_t iter = purge_range.first;
        iter != purge_range.second;
@@ -8299,21 +8317,25 @@ module_info::update_symtab(cu_function_cache_t *funcs)
       // missing, so we may also need to try matching by address.  See also the
       // notes about _Z in dwflpp::iterate_over_functions().
 
-      func_info *fi = sym_table->lookup_symbol(func->first);
-      if (!fi)
+      set<func_info*> *fis = sym_table->lookup_symbol(func->first);
+      if (!fis || fis->empty())
         continue;
 
-      // iterate over all functions at the same address
-      symbol_table::range_t er = sym_table->map_by_addr.equal_range(fi->addr);
-      for (symbol_table::iterator_t it = er.first; it != er.second; ++it)
+      for (set<func_info*>::iterator fi = fis->begin(); fi!=fis->end(); ++fi)
         {
-          // update this function with the dwarf die
-          it->second->die = func->second;
+          // iterate over all functions at the same address
+          symbol_table::range_t er = sym_table->map_by_addr.equal_range((*fi)->addr);
+
+          for (symbol_table::iterator_t it = er.first; it != er.second; ++it)
+            {
+              // update this function with the dwarf die
+              it->second->die = func->second;
 
-          // if this function is a new alias, then
-          // save it to merge into the function cache
-          if (it->second != fi)
-            new_funcs.insert(make_pair(it->second->name, it->second->die));
+              // if this function is a new alias, then
+              // save it to merge into the function cache
+              if (it->second != *fi)
+                new_funcs.insert(make_pair(it->second->name, it->second->die));
+            }
         }
     }
 
-- 
1.9.3

[RFC PATCH 2/3] Test dwarfless probes on multiple static functions

[Hemant Kumar]

This patch checks how many symbols were resolved instead of probing on
them which won't require us to go till pass 5. It runs the .stp script
till pass 2. This test can be run with:
make check RUNTESTFLAGS=multisym.exp

Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
---
 testsuite/systemtap.base/multisym.exp    | 46 ++++++++++++++++++++++++++++++++
 testsuite/systemtap.base/multisym.stp    |  1 +
 testsuite/systemtap.base/multisym_baz.c  | 11 ++++++++
 testsuite/systemtap.base/multisym_main.c | 10 +++++++
 4 files changed, 68 insertions(+)
 create mode 100755 testsuite/systemtap.base/multisym.exp
 create mode 100644 testsuite/systemtap.base/multisym.stp
 create mode 100644 testsuite/systemtap.base/multisym_baz.c
 create mode 100644 testsuite/systemtap.base/multisym_main.c

diff --git a/testsuite/systemtap.base/multisym.exp b/testsuite/systemtap.base/multisym.exp
new file mode 100755
index 0000000..b8c96ce
--- /dev/null
+++ b/testsuite/systemtap.base/multisym.exp
@@ -0,0 +1,46 @@
+#!/usr/bin/expect
+
+set test "multisym"
+set testpath "$srcdir/$subdir"
+set script "$testpath/multisym.stp"
+
+# Test that two functions with the same name in the symbol table are
+# both found even when no DWARF information is available
+# As the resolution of the probes occur during pass 2, we don't need to go
+# all the way to pass 5
+# We need the number of resolutions during pass 2.
+
+set cmd [concat stap {-p 2 } $script]
+
+set res [target_compile ${testpath}/${test}_baz.c ${test}_baz.o object ""]
+if { $res != "" } {
+    verbose "target_compile failed: $res" 2
+    fail "unable to compile ${test}_baz.c"
+}
+
+set res [target_compile ${testpath}/${test}_main.c ${test}_main.o object ""]
+if { $res != "" } {
+    verbose "target_compile failed: $res" 2
+    fail "unable to compile ${test}_main.c"
+}
+
+set res [target_compile "${test}_baz.o ${test}_main.o" ${test} executable ""]
+if { $res != "" } {
+    verbose "target_compile failed: $res" 2
+    fail "unable to compile ${test}"
+}
+
+eval spawn $cmd
+set results 0
+expect {
+    -re {process\([a-z\/\"]+\)*} { incr results 1; exp_continue}
+}
+set res [wait -i $spawn_id]
+catch close
+set res [lindex $res 3]
+
+if {$res == 0 && $results == 4} {
+	pass "$test succeeded"
+    } else {
+	fail "$test failed"
+    }
diff --git a/testsuite/systemtap.base/multisym.stp b/testsuite/systemtap.base/multisym.stp
new file mode 100644
index 0000000..be30a15
--- /dev/null
+++ b/testsuite/systemtap.base/multisym.stp
@@ -0,0 +1 @@
+probe process("./multisym").function("foo") { printf ("hit") }
\ No newline at end of file
diff --git a/testsuite/systemtap.base/multisym_baz.c b/testsuite/systemtap.base/multisym_baz.c
new file mode 100644
index 0000000..297514a
--- /dev/null
+++ b/testsuite/systemtap.base/multisym_baz.c
@@ -0,0 +1,11 @@
+static int
+foo (int v)
+{
+	return v + 1;
+}
+
+int
+bar (int i)
+{
+	return foo (i - 1);
+}
diff --git a/testsuite/systemtap.base/multisym_main.c b/testsuite/systemtap.base/multisym_main.c
new file mode 100644
index 0000000..e857385
--- /dev/null
+++ b/testsuite/systemtap.base/multisym_main.c
@@ -0,0 +1,10 @@
+int foo (int v)
+{
+	return bar (v - 1);
+}
+
+int
+main (int argc, char **argv)
+{
+	return foo (argc);
+}
-- 
1.9.3

[PATCH v4 3/3] Fix: Priotirize symbol table lookup for ppc64le

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

Macro definition PPC64_LOCAL_ENTRY_OFFSET has been picked up from glibc.
It won't be defined if we are building systemtap on a machine having
older elf.h and hence, won't recognize PPC64_LOCAL_ENTRY_OFFSET.

Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
---
 tapsets.cxx | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/tapsets.cxx b/tapsets.cxx
index c96c542..ef11f8a 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -66,7 +66,17 @@ extern "C" {
 using namespace std;
 using namespace __gnu_cxx;
 
-
+// for elf.h where PPC64_LOCAL_ENTRY_OFFSET isn't defined
+#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
+// for elf.h where EF_PPC64_ABI isn't defined
+#ifndef EF_PPC64_ABI
+#define EF_PPC64_ABI 3
+#endif
 
 // ------------------------------------------------------------------------
 
@@ -2050,6 +2060,18 @@ query_dwarf_inline_instance (Dwarf_Die * die, dwarf_query * q)
     }
 }
 
+static bool
+is_filtered_func_exists (func_info_map_t filtered, func_info *fi)
+{
+  for (unsigned i = 0; i < filtered.size(); i++)
+    {
+      if ((filtered[i].entrypc == fi->entrypc) && (filtered[i].name == fi->name))
+        return true;
+    }
+
+  return false;
+}
+
 static int
 query_dwarf_func (Dwarf_Die * func, dwarf_query * q)
 {
@@ -2100,7 +2122,35 @@ 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;
+          Dwarf_Addr bias;
+          Dwfl_Module *mod = q->dw.module;
+          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 == NULL) throw SEMANTIC_ERROR (_("Couldn't get elf header"));
+
+          /* Giving priority to sym_table for ppc64*/
+          if ((em->e_machine == EM_PPC64) && ((em->e_flags & EF_PPC64_ABI) == 2)
+              && (q->dw.mod_info->sym_table))
+            {
+              set<func_info *> *fis;
+              fis = q->dw.mod_info->sym_table->lookup_symbol(func.name);
+              if (fis && !fis->empty())
+                {
+                  for (set<func_info*>::iterator it=fis->begin(); it!=fis->end() ; ++it)
+                    {
+                      func.entrypc = (*it)->addr;
+                      if (is_filtered_func_exists(q->filtered_functions, &func))
+                        continue;
+                      q->filtered_functions.push_back(func);
+                    }
+                }
+            }
+          else if (!func.entrypc && q->dw.function_entrypc (&entrypc))
             {
               func.entrypc = entrypc;
               q->filtered_functions.push_back (func);
@@ -8140,6 +8190,14 @@ symbol_table::get_from_elf()
   int syments = dwfl_module_getsymtab(mod);
   assert(syments);
   prepare_section_rejection(mod);
+  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 == NULL) throw SEMANTIC_ERROR (_("Couldn't get elf header"));
+
   for (int i = 1; i < syments; ++i)
     {
       GElf_Sym sym;
@@ -8168,6 +8226,16 @@ symbol_table::get_from_elf()
       addr = sym.st_value;
       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.
+      *
+      * st_other field is currently only used with ABIv2 on ppc64
+      */
+      if ((em->e_machine == EM_PPC64) && ((em->e_flags & EF_PPC64_ABI) == 2)
+          && (GELF_ST_TYPE(sym.st_info) == STT_FUNC) && sym.st_other)
+        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),
-- 
1.9.3

Re: [RFC PATCH 2/3] Test dwarfless probes on multiple static functions

[Hemant Kumar]

This test is based on a patch suggested by Mark :
https://sourceware.org/ml/systemtap/2015-q2/msg00011.html

On 04/20/2015 03:59 PM, Hemant Kumar wrote:
> This patch checks how many symbols were resolved instead of probing on
> them which won't require us to go till pass 5. It runs the .stp script
> till pass 2. This test can be run with:
> make check RUNTESTFLAGS=multisym.exp
>
> Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
> ---
>   testsuite/systemtap.base/multisym.exp    | 46 ++++++++++++++++++++++++++++++++
>   testsuite/systemtap.base/multisym.stp    |  1 +
>   testsuite/systemtap.base/multisym_baz.c  | 11 ++++++++
>   testsuite/systemtap.base/multisym_main.c | 10 +++++++
>   4 files changed, 68 insertions(+)
>   create mode 100755 testsuite/systemtap.base/multisym.exp
>   create mode 100644 testsuite/systemtap.base/multisym.stp
>   create mode 100644 testsuite/systemtap.base/multisym_baz.c
>   create mode 100644 testsuite/systemtap.base/multisym_main.c
>
> diff --git a/testsuite/systemtap.base/multisym.exp b/testsuite/systemtap.base/multisym.exp
> new file mode 100755
> index 0000000..b8c96ce
> --- /dev/null
> +++ b/testsuite/systemtap.base/multisym.exp
> @@ -0,0 +1,46 @@
> +#!/usr/bin/expect
> +
> +set test "multisym"
> +set testpath "$srcdir/$subdir"
> +set script "$testpath/multisym.stp"
> +
> +# Test that two functions with the same name in the symbol table are
> +# both found even when no DWARF information is available
> +# As the resolution of the probes occur during pass 2, we don't need to go
> +# all the way to pass 5
> +# We need the number of resolutions during pass 2.
> +
> +set cmd [concat stap {-p 2 } $script]
> +
> +set res [target_compile ${testpath}/${test}_baz.c ${test}_baz.o object ""]
> +if { $res != "" } {
> +    verbose "target_compile failed: $res" 2
> +    fail "unable to compile ${test}_baz.c"
> +}
> +
> +set res [target_compile ${testpath}/${test}_main.c ${test}_main.o object ""]
> +if { $res != "" } {
> +    verbose "target_compile failed: $res" 2
> +    fail "unable to compile ${test}_main.c"
> +}
> +
> +set res [target_compile "${test}_baz.o ${test}_main.o" ${test} executable ""]
> +if { $res != "" } {
> +    verbose "target_compile failed: $res" 2
> +    fail "unable to compile ${test}"
> +}
> +
> +eval spawn $cmd
> +set results 0
> +expect {
> +    -re {process\([a-z\/\"]+\)*} { incr results 1; exp_continue}
> +}
> +set res [wait -i $spawn_id]
> +catch close
> +set res [lindex $res 3]
> +
> +if {$res == 0 && $results == 4} {
> +	pass "$test succeeded"
> +    } else {
> +	fail "$test failed"
> +    }
> diff --git a/testsuite/systemtap.base/multisym.stp b/testsuite/systemtap.base/multisym.stp
> new file mode 100644
> index 0000000..be30a15
> --- /dev/null
> +++ b/testsuite/systemtap.base/multisym.stp
> @@ -0,0 +1 @@
> +probe process("./multisym").function("foo") { printf ("hit") }
> \ No newline at end of file
> diff --git a/testsuite/systemtap.base/multisym_baz.c b/testsuite/systemtap.base/multisym_baz.c
> new file mode 100644
> index 0000000..297514a
> --- /dev/null
> +++ b/testsuite/systemtap.base/multisym_baz.c
> @@ -0,0 +1,11 @@
> +static int
> +foo (int v)
> +{
> +	return v + 1;
> +}
> +
> +int
> +bar (int i)
> +{
> +	return foo (i - 1);
> +}
> diff --git a/testsuite/systemtap.base/multisym_main.c b/testsuite/systemtap.base/multisym_main.c
> new file mode 100644
> index 0000000..e857385
> --- /dev/null
> +++ b/testsuite/systemtap.base/multisym_main.c
> @@ -0,0 +1,10 @@
> +int foo (int v)
> +{
> +	return bar (v - 1);
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +	return foo (argc);
> +}

-- 
Thanks,
Hemant Kumar

Re: [PATCH v4 1/3] systemtap/tapsets.cxx: Fix dwarfless probes on multiple static functions

[Mark Wielaard]

Hi Hemant,

On Mon, 2015-04-20 at 15:59 +0530, Hemant Kumar wrote:
> With multiple static functions with same names in an ELF and in absence
> of dwarf, if we probe on one of the functions, then systemtap places
> probe only on one static function ignoring the rest. This is because the
> mapping between the symbol names and their func_info is a simple map
> which doesn't allow insertion of another symbol with the same name.
> 
> This patch fixes this issue by changing this map to a multimap which
> allows duplicate entries for the same symbol name. lookup_symbol code
> will return a set of func_info * instead of a single descriptor for a
> function name.
> 
> We also need to fix other areas in the code where lookup_symbol() and
> lookup_symbol_address() are being called so as to look for a set of
> func_info's and a list of Dwarf_Addr's respectively, instead of a single
> descriptor.

Looks good. I pushed this with one tiny change:

@@ -8242,6 +8242,8 @@ symbol_table::purge_syscall_stubs()
   if (!addrs || addrs->empty())
     return;
   /* Highly unlikely that multiple symbols named "sys_ni_syscall" may exist */
+  if (addrs->size() > 1)
+    cerr << _("Multiple 'sys_ni_syscall' symbols found.");
   Dwarf_Addr stub_addr = addrs->front();

Just so that if this highly unlikely scenario does occur we get a
warning something is fishy.

Thanks,

Mark

Re: [PATCH v4 3/3] Fix: Priotirize symbol table lookup for ppc64le

[Mark Wielaard]

On Mon, 2015-04-20 at 15:59 +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. 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.
> 
> Macro definition PPC64_LOCAL_ENTRY_OFFSET has been picked up from glibc.
> It won't be defined if we are building systemtap on a machine having
> older elf.h and hence, won't recognize PPC64_LOCAL_ENTRY_OFFSET.

Looks good. Pushed with one tiny change (in the commit message):
s/Priotirize/Prioritize/

Thanks,

Mark

Re: [PATCH v4 3/3] Fix: Priotirize symbol table lookup for ppc64le

[Hemant Kumar]

On 04/22/2015 07:18 PM, Mark Wielaard wrote:
> On Mon, 2015-04-20 at 15:59 +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. 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.
>>
>> Macro definition PPC64_LOCAL_ENTRY_OFFSET has been picked up from glibc.
>> It won't be defined if we are building systemtap on a machine having
>> older elf.h and hence, won't recognize PPC64_LOCAL_ENTRY_OFFSET.
> Looks good. Pushed with one tiny change (in the commit message):
> s/Priotirize/Prioritize/

Thanks a lot Mark.

-- 
Thanks,
Hemant Kumar

Re: [PATCH v4 1/3] systemtap/tapsets.cxx: Fix dwarfless probes on multiple static functions

[Hemant Kumar]

On 04/22/2015 07:10 PM, Mark Wielaard wrote:
> Hi Hemant,
>
> On Mon, 2015-04-20 at 15:59 +0530, Hemant Kumar wrote:
>> With multiple static functions with same names in an ELF and in absence
>> of dwarf, if we probe on one of the functions, then systemtap places
>> probe only on one static function ignoring the rest. This is because the
>> mapping between the symbol names and their func_info is a simple map
>> which doesn't allow insertion of another symbol with the same name.
>>
>> This patch fixes this issue by changing this map to a multimap which
>> allows duplicate entries for the same symbol name. lookup_symbol code
>> will return a set of func_info * instead of a single descriptor for a
>> function name.
>>
>> We also need to fix other areas in the code where lookup_symbol() and
>> lookup_symbol_address() are being called so as to look for a set of
>> func_info's and a list of Dwarf_Addr's respectively, instead of a single
>> descriptor.
> Looks good. I pushed this with one tiny change:

Thanks again.

> @@ -8242,6 +8242,8 @@ symbol_table::purge_syscall_stubs()
>     if (!addrs || addrs->empty())
>       return;
>     /* Highly unlikely that multiple symbols named "sys_ni_syscall" may exist */
> +  if (addrs->size() > 1)
> +    cerr << _("Multiple 'sys_ni_syscall' symbols found.");
>     Dwarf_Addr stub_addr = addrs->front();
>
> Just so that if this highly unlikely scenario does occur we get a
> warning something is fishy.
>

Right! looks good.

-- 
Thanks,
Hemant Kumar

Re: [RFC PATCH 2/3] Test dwarfless probes on multiple static functions

[Mark Wielaard]

On Mon, 2015-04-20 at 15:59 +0530, Hemant Kumar wrote:
> This patch checks how many symbols were resolved instead of probing on
> them which won't require us to go till pass 5. It runs the .stp script
> till pass 2. This test can be run with:
> make check RUNTESTFLAGS=multisym.exp

Nice. Much better if it can be checked in pass 2.
Pushed to master.

Thanks,

Mark

Re: [PATCH v4 1/3] systemtap/tapsets.cxx: Fix dwarfless probes on multiple static functions

[Mark Wielaard]

On Wed, 2015-04-22 at 20:05 +0530, Hemant Kumar wrote:
> On 04/22/2015 07:10 PM, Mark Wielaard wrote:
> > @@ -8242,6 +8242,8 @@ symbol_table::purge_syscall_stubs()
> >     if (!addrs || addrs->empty())
> >       return;
> >     /* Highly unlikely that multiple symbols named "sys_ni_syscall" may exist */
> > +  if (addrs->size() > 1)
> > +    cerr << _("Multiple 'sys_ni_syscall' symbols found.");
> >     Dwarf_Addr stub_addr = addrs->front();
> >
> > Just so that if this highly unlikely scenario does occur we get a
> > warning something is fishy.
> >
> Right! looks good.

And I am glad we did add that warning.
Martin found it triggered on ppc64be (ELFv1 ABI).

It was caused by ppc64be using function descriptors and stap using both
the actual function entry symbol .sys_ni_syscall and the function
descriptor symbol sys_ni_syscall. Both resolved to the same address. And
we mangle the name of the function entry symbol to remove the leading
dot. So they also have the same name.

This was mostly harmless. But it showed some inefficiencies. Frank
solved the immediate issue by using address sets instead of lists, so
duplicate addresses are just not returned:

commit fb5b48419b8d74e6cb82e90ba0aa9e188db07043
Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Wed Apr 22 16:53:51 2015 -0400

  tapsets.cxx: fix symbol/address lookup returned-data to sets passed
  by value
    
  The symbol_table lookup_symbol[_address] functions are safer if they
  return their result-sets by value rather than by pointer.  The latter
  in specific should be a set rather than a list, to properly eliminate
  duplicates.

Then I removed the hardcoded #ifdef __powerpc__ constructs in
tapsets.cxx and replaced them with a check of whether the target is
ppc64 ELFv1 abi. That way cross-stapping (is that a word?) should work
across arches too (but I haven't tested that, just that ppc64be and
ppc64le both work correctly). This also removes the actual duplicates,
so the maps aren't filled with extra func_infos (there could be lots of
duplicates in the kernel when using function descriptors).

commit 064a90a93b8702a9f2649b5d46494e6218c8a145
Author: Mark Wielaard <mjw@redhat.com>
Date:   Thu Apr 23 15:59:49 2015 +0200

  ppc64le doesn't have function descriptors. Remove __powerpc__ in
  tapsets.cxx
    
  Only process the opd section and do function descriptor mangling when
  the target is ppc64 ELFv1 ABI. Also filter out any duplicate
  func_infos.
  When seeing a symbol with a name starting with '.' we assume it is a
  regular function pointer and not a pointer to a function descriptor
  and mangle its name. That might create duplicates if there is also a
  function descriptor with that name (the address will already have been
  resolved to the same address).

Cheers,

Mark

Re: [PATCH v4 1/3] systemtap/tapsets.cxx: Fix dwarfless probes on multiple static functions

[Mark Wielaard]

On Thu, 2015-04-23 at 16:21 +0200, Mark Wielaard wrote:
> On Wed, 2015-04-22 at 20:05 +0530, Hemant Kumar wrote:
> > On 04/22/2015 07:10 PM, Mark Wielaard wrote:
> > > @@ -8242,6 +8242,8 @@ symbol_table::purge_syscall_stubs()
> > >     if (!addrs || addrs->empty())
> > >       return;
> > >     /* Highly unlikely that multiple symbols named "sys_ni_syscall" may exist */
> > > +  if (addrs->size() > 1)
> > > +    cerr << _("Multiple 'sys_ni_syscall' symbols found.");
> > >     Dwarf_Addr stub_addr = addrs->front();
> > >
> > > Just so that if this highly unlikely scenario does occur we get a
> > > warning something is fishy.
> > >
> > Right! looks good.
> 
> And I am glad we did add that warning.
> Martin found it triggered on ppc64be (ELFv1 ABI).

And then Martin found another issue on ppc64be (ELFv1 ABI) when
systemtap was using an older elfutils (< 0.158). Again it was mostly
harmless, but somewhat inefficient. Fixed as follows:

commit f80d9cdb30789aecac4727ae87b22c373900e6ca
Author: Mark Wielaard <mjw@redhat.com>
Date:   Fri Apr 24 19:59:32 2015 +0200

  Filter out descriptor/SHN_UNDEF symbols in
  symbol_table::lookup_symbol.
    
  With newer elfutils (>= 0.158) function descriptor symbols get
  resolved to their actual function entry address. With older elfutils
  we mark such symbols as descriptor through reject_section (because
  their address will match the .opd). Filter these symbols out in
  symbol_table::lookup_symbol and symbol_table::lookup_symbol_address.
  None of the callers want these SHN_UNDEF/descriptor symbol
  (addresses).
    
  This solves another instance of Multiple 'sys_ni_syscall' symbols
  found warning on ppc64[be] with older elfutils.

Cheers,

Mark