[PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Hemant Kumar]

There can be multiple static functions in an ELF (although in different
compilation units). But the existing lookup_symbol() code doesn't take
care of this. This patch changes the already existing map between
a function name to its descriptor to a map between a function name
to a list of descriptors(func_info), so that multiple static functions
can be accomodated in this map.

So, now whenever lookup_symbol will be called, a list of func_info *
will be sent instead of a single descriptor corresponding to the
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 list
instead of a single value.

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

diff --git a/tapsets.cxx b/tapsets.cxx
index 443fb2e..326ba6a 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;
+  map<string, list <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);
+  list <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,16 @@ 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);
+          list<func_info*> *fis = new list<func_info*>;
+          fis = sym_table->lookup_symbol(function_str_val);
+          if (!fis || fis->empty())
+            return;
+          for (list<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 +7491,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();
+      map<string, list<func_info*> >& modfuncs = module->sym_table->map_by_name;
+      for (map<string, list <func_info*> >::const_iterator itfuncs = modfuncs.begin();
            itfuncs != modfuncs.end(); ++itfuncs)
         funcs.insert(itfuncs->first);
     }
@@ -8072,7 +8079,21 @@ 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;
+
+  if (map_by_name[fi->name].empty())
+    map_by_name[fi->name].push_back(fi);
+  else /* Check if its a duplicate entry ? */
+    {
+      list<func_info*>::iterator it=map_by_name[fi->name].begin();
+      for (; it!=map_by_name[fi->name].end(); ++it)
+        {
+          if (fi->addr == (*it)->addr)
+            break;
+        }
+      if (it == map_by_name[fi->name].end())
+        map_by_name[fi->name].push_back(fi);
+    }
+
   // TODO: Use a multimap in case there are multiple static
   // functions with the same name?
   map_by_addr.insert(make_pair(addr, fi));
@@ -8194,22 +8215,29 @@ symbol_table::get_first_func()
   return (iter)->second;
 }
 
-func_info *
+list <func_info*> *
 symbol_table::lookup_symbol(const string& name)
 {
-  map<string, func_info*>::iterator i = map_by_name.find(name);
+  map<string, list <func_info*> >::iterator i = map_by_name.find(name);
   if (i == map_by_name.end())
     return NULL;
-  return i->second;
+  return &i->second;
 }
 
-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<func_info*> *fis = new list<func_info*>;
+  list<Dwarf_Addr> *addrs = new list<Dwarf_Addr>;
+
+  fis = lookup_symbol(name);
+  if (!fis || fis->empty())
+    return NULL;
+  for (list<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 +8251,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 functions 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 +8330,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)
+      list<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 (list<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);
 
-          // 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));
+          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));
+            }
         }
     }
 
-- 
1.9.3

[PATCH v3 2/2] Fix: ppc64le support

[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 | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/tapsets.cxx b/tapsets.cxx
index 326ba6a..d8d4222 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -66,7 +66,13 @@ 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
 
 // ------------------------------------------------------------------------
 
@@ -2051,6 +2057,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)
 {
@@ -2101,7 +2119,34 @@ 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) && (q->dw.mod_info->sym_table))
+             {
+               list<func_info *> *fis;
+               fis = q->dw.mod_info->sym_table->lookup_symbol(func.name);
+               if (fis && !fis->empty())
+                 {
+                   for (list<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);
@@ -8156,6 +8201,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;
@@ -8184,6 +8237,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) && (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: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 3522 bytes --]

Hi Hemant,

On Fri, 2015-03-27 at 19:12 +0530, Hemant Kumar wrote:
> There can be multiple static functions in an ELF (although in different
> compilation units). But the existing lookup_symbol() code doesn't take
> care of this. This patch changes the already existing map between
> a function name to its descriptor to a map between a function name
> to a list of descriptors(func_info), so that multiple static functions
> can be accomodated in this map.

Thanks. For some reason I hadn't realized that the example I gave with
the same function symbol name in a symbol table wasn't ppc64le specific
at all. When we don't have DWARF debuginfo it is a generic issue we only
pick up one function symbol.

It would be nice to add a testcase for this. It can be as simple as what
I posted, but with -g removed, so we'll have to use the symbol table:

gcc -c baz.c
gcc -c main.c
gcc -o prog baz.o main.o

stap -e 'probe process.function("foo") { printf ("%s: %x\n", pp(), uaddr()) }' -c ./prog

Without your patch it gives:

process("/tmp/prog").function("foo"): 40051c

But with your patch all foo functions are correctly hit:

process("/tmp/prog").function("foo"): 40051c
process("/tmp/prog").function("foo"): 4004f0

And we could just add a { log ("hit") } as probe body, and check we get
two hits as testcase. Something like the attached testcase fails for me
without your patch, but passes with it when doing make installcheck
RUNTESTFLAGS=multisym.exp. But maybe there is a simpler way to test it
that doesn't need installcheck?

> So, now whenever lookup_symbol will be called, a list of func_info *
> will be sent instead of a single descriptor corresponding to the
> 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 list
> instead of a single value.

The patch does look OK to me. But my C++ container knowledge is a little
shaky. So some questions. First there is still a comment in the code
saying:

>    // TODO: Use a multimap in case there are multiple static
>    // functions with the same name?
>    map_by_addr.insert(make_pair(addr, fi));

But map_by_addr is already a multimap as introduced in commit 1c6b77
PR10327: resolve symbol aliases to dwarf functions by Josh. Which seems
to solve a somewhat similar issue in the case we do have DWARF
information. Josh, do you remember why that comment was kept?

Since map_by_addr is using a multimap I was wondering if map_by_name
should also be a multimap instead of a map to a list? Do you happen to
know the advantages/disadvantages of the two datastructures?

> @@ -1113,9 +1113,16 @@ 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);
> +          list<func_info*> *fis = new list<func_info*>;
> +          fis = sym_table->lookup_symbol(function_str_val);
> +          if (!fis || fis->empty())
> +            return;
> +          for (list<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);
> +            }
>          }
>      }
>  }

Don't we need to delete the fis somewhere?

Thanks,

Mark

[-- Attachment #2: 0001-Add-multisym-test.patch --]
[-- Type: text/x-patch, Size: 2876 bytes --]

From 1c574d34ee64dc47b83feb99bcb1e2492518749b Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 8 Apr 2015 15:13:44 +0200
Subject: [PATCH] Add multisym test

---
 testsuite/systemtap.base/multisym.exp    | 30 ++++++++++++++++++++++++++++++
 testsuite/systemtap.base/multisym.stp    |  2 ++
 testsuite/systemtap.base/multisym_baz.c  | 11 +++++++++++
 testsuite/systemtap.base/multisym_main.c | 10 ++++++++++
 4 files changed, 53 insertions(+)
 create mode 100644 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 100644
index 0000000..dbcd6ca
--- /dev/null
+++ b/testsuite/systemtap.base/multisym.exp
@@ -0,0 +1,30 @@
+set test "multisym"
+set testpath "$srcdir/$subdir"
+
+# Test that two functions with the same name in the symbol table are
+# both found even when no DWARF information is available.
+set ::result_string {hit
+hit}
+
+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}"
+}
+
+stap_run3 $test $srcdir/$subdir/$test.stp -c ./${test}
+
+# Cleanup
+if { $verbose == 0 } { catch { exec rm -f $test } }
diff --git a/testsuite/systemtap.base/multisym.stp b/testsuite/systemtap.base/multisym.stp
new file mode 100644
index 0000000..85dac9a
--- /dev/null
+++ b/testsuite/systemtap.base/multisym.stp
@@ -0,0 +1,2 @@
+# Test that must hit both foo functions.
+probe process.function("foo") { log ("hit") }
diff --git a/testsuite/systemtap.base/multisym_baz.c b/testsuite/systemtap.base/multisym_baz.c
new file mode 100644
index 0000000..37eab89
--- /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..3e51215
--- /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.8.3.1

Re: [PATCH v3 2/2] Fix: ppc64le support

[Mark Wielaard]

Hi Hemant,

This looks good to me. One comment.

On Fri, 2015-03-27 at 19:12 +0530, Hemant Kumar wrote:
> +           /* Giving priority to sym_table for ppc64*/
> +           if ((em->e_machine == EM_PPC64) && (q->dw.mod_info->sym_table))
> [...]
> +     /*
> +      * 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) && (GELF_ST_TYPE(sym.st_info) == STT_FUNC)
> +           && sym.st_other)
> +        addr += PPC64_LOCAL_ENTRY_OFFSET(sym.st_other);

In both cases I believe the check should be:
	em->e_machine == EM_PPC64 && (em->e_flags & EF_PPC64_ABI) == 2

Like the other PPC64 macros you probably need to #define EF_PPC64_ABI 3
if not yet defined in elf.h.

Cheers,

Mark

Re: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Frank Ch. Eigler]

mjw wrote:

> [...]
>>    // TODO: Use a multimap in case there are multiple static
>>    // functions with the same name?
>>    map_by_addr.insert(make_pair(addr, fi));
>
> But map_by_addr is already a multimap as introduced in commit 1c6b77
> PR10327: resolve symbol aliases to dwarf functions by Josh. [...]
> Since map_by_addr is using a multimap I was wondering if map_by_name
> should also be a multimap instead of a map to a list? Do you happen to
> know the advantages/disadvantages of the two datastructures?

Indeed, the new structure should be a multimap too.  That means it'd
be apprx. a binary tree of (key,value) pairs (with duplication
allowed.  It would be a bit more canonical, and has advantages in
cases where the list needs to be modified or traversed without
copying.


>> +          list<func_info*> *fis = new list<func_info*>;
>> +          fis = sym_table->lookup_symbol(function_str_val);
>> [...]
> Don't we need to delete the fis somewhere?

lookup_symbol should just return the list by value and let C++ handle
the memory management.  (The objects pointed to by the embedded
func_info* pointers are a separate matter.)

- FChE

Re: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Josh Stone]

On 04/08/2015 06:43 AM, Mark Wielaard wrote:
> The patch does look OK to me. But my C++ container knowledge is a little
> shaky. So some questions. First there is still a comment in the code
> saying:
> 
>>    // TODO: Use a multimap in case there are multiple static
>>    // functions with the same name?
>>    map_by_addr.insert(make_pair(addr, fi));
> 
> But map_by_addr is already a multimap as introduced in commit 1c6b77
> PR10327: resolve symbol aliases to dwarf functions by Josh. Which seems
> to solve a somewhat similar issue in the case we do have DWARF
> information. Josh, do you remember why that comment was kept?

Since it's talking about "same name", I believe that comment is actually
referring to the line *before* it, which sets map_by_name.

Re: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Hemant Kumar]

Hi Mark,

Thanks for the review.

On 04/08/2015 07:13 PM, Mark Wielaard wrote:
> Hi Hemant,
>
> On Fri, 2015-03-27 at 19:12 +0530, Hemant Kumar wrote:
>> There can be multiple static functions in an ELF (although in different
>> compilation units). But the existing lookup_symbol() code doesn't take
>> care of this. This patch changes the already existing map between
>> a function name to its descriptor to a map between a function name
>> to a list of descriptors(func_info), so that multiple static functions
>> can be accomodated in this map.
> Thanks. For some reason I hadn't realized that the example I gave with
> the same function symbol name in a symbol table wasn't ppc64le specific
> at all. When we don't have DWARF debuginfo it is a generic issue we only
> pick up one function symbol.
>
> It would be nice to add a testcase for this. It can be as simple as what

Sure, will add a testcase for this.

> I posted, but with -g removed, so we'll have to use the symbol table:
>
> gcc -c baz.c
> gcc -c main.c
> gcc -o prog baz.o main.o
>
> stap -e 'probe process.function("foo") { printf ("%s: %x\n", pp(), uaddr()) }' -c ./prog
>
> Without your patch it gives:
>
> process("/tmp/prog").function("foo"): 40051c
>
> But with your patch all foo functions are correctly hit:
>
> process("/tmp/prog").function("foo"): 40051c
> process("/tmp/prog").function("foo"): 4004f0
>
> And we could just add a { log ("hit") } as probe body, and check we get
> two hits as testcase. Something like the attached testcase fails for me
> without your patch, but passes with it when doing make installcheck
> RUNTESTFLAGS=multisym.exp. But maybe there is a simpler way to test it
> that doesn't need installcheck?

Will try that.

>> So, now whenever lookup_symbol will be called, a list of func_info *
>> will be sent instead of a single descriptor corresponding to the
>> 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 list
>> instead of a single value.
> The patch does look OK to me. But my C++ container knowledge is a little
> shaky. So some questions. First there is still a comment in the code
> saying:
>
>>     // TODO: Use a multimap in case there are multiple static
>>     // functions with the same name?
>>     map_by_addr.insert(make_pair(addr, fi));
> But map_by_addr is already a multimap as introduced in commit 1c6b77
> PR10327: resolve symbol aliases to dwarf functions by Josh. Which seems
> to solve a somewhat similar issue in the case we do have DWARF
> information. Josh, do you remember why that comment was kept?
>
> Since map_by_addr is using a multimap I was wondering if map_by_name
> should also be a multimap instead of a map to a list? Do you happen to
> know the advantages/disadvantages of the two datastructures?
>> @@ -1113,9 +1113,16 @@ 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);
>> +          list<func_info*> *fis = new list<func_info*>;
>> +          fis = sym_table->lookup_symbol(function_str_val);
>> +          if (!fis || fis->empty())
>> +            return;
>> +          for (list<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);
>> +            }
>>           }
>>       }
>>   }
> Don't we need to delete the fis somewhere?
>
> Thanks,
>
> Mark

-- 
Thanks,
Hemant Kumar

Re: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Hemant Kumar]

On 04/08/2015 09:19 PM, Frank Ch. Eigler wrote:
> mjw wrote:
>
>> [...]
>>>     // TODO: Use a multimap in case there are multiple static
>>>     // functions with the same name?
>>>     map_by_addr.insert(make_pair(addr, fi));
>> But map_by_addr is already a multimap as introduced in commit 1c6b77
>> PR10327: resolve symbol aliases to dwarf functions by Josh. [...]
>> Since map_by_addr is using a multimap I was wondering if map_by_name
>> should also be a multimap instead of a map to a list? Do you happen to
>> know the advantages/disadvantages of the two datastructures?
> Indeed, the new structure should be a multimap too.  That means it'd
> be apprx. a binary tree of (key,value) pairs (with duplication
> allowed.  It would be a bit more canonical, and has advantages in
> cases where the list needs to be modified or traversed without
> copying.
>

Right.
But, when I tried that this with multimap, the problem I faced is we can 
insert two or more entries with same key and value, for e.g., if a 
multimap already has <a,b> as an entry, we can insert multiple <a,b> 
entries after that because they don't have duplication check which is 
not the case with simple maps.
But yeah, this can be done with multimap with a duplication check before 
insertion.

>>> +          list<func_info*> *fis = new list<func_info*>;
>>> +          fis = sym_table->lookup_symbol(function_str_val);
>>> [...]
>> Don't we need to delete the fis somewhere?
> lookup_symbol should just return the list by value and let C++ handle
> the memory management.  (The objects pointed to by the embedded
> func_info* pointers are a separate matter.)
>
> - FChE
>

-- 
Thanks,
Hemant Kumar

Re: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Hemant Kumar]

On 04/08/2015 09:41 PM, Josh Stone wrote:
> On 04/08/2015 06:43 AM, Mark Wielaard wrote:
>> The patch does look OK to me. But my C++ container knowledge is a little
>> shaky. So some questions. First there is still a comment in the code
>> saying:
>>
>>>     // TODO: Use a multimap in case there are multiple static
>>>     // functions with the same name?
>>>     map_by_addr.insert(make_pair(addr, fi));
>> But map_by_addr is already a multimap as introduced in commit 1c6b77
>> PR10327: resolve symbol aliases to dwarf functions by Josh. Which seems
>> to solve a somewhat similar issue in the case we do have DWARF
>> information. Josh, do you remember why that comment was kept?
> Since it's talking about "same name", I believe that comment is actually
> referring to the line *before* it, which sets map_by_name.
>

Right, will remove the above TODO in the next version.

-- 
Thanks,
Hemant Kumar

Re: [PATCH v3 2/2] Fix: ppc64le support

[Hemant Kumar]

On 04/08/2015 08:20 PM, Mark Wielaard wrote:
> Hi Hemant,
>
> This looks good to me. One comment.
>
> On Fri, 2015-03-27 at 19:12 +0530, Hemant Kumar wrote:
>> +           /* Giving priority to sym_table for ppc64*/
>> +           if ((em->e_machine == EM_PPC64) && (q->dw.mod_info->sym_table))
>> [...]
>> +     /*
>> +      * 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) && (GELF_ST_TYPE(sym.st_info) == STT_FUNC)
>> +           && sym.st_other)
>> +        addr += PPC64_LOCAL_ENTRY_OFFSET(sym.st_other);
> In both cases I believe the check should be:
> 	em->e_machine == EM_PPC64 && (em->e_flags & EF_PPC64_ABI) == 2

Nice idea, thanks.
Will add this check in the next version.

> Like the other PPC64 macros you probably need to #define EF_PPC64_ABI 3
> if not yet defined in elf.h.

Sure, will define EF_PPC64_ABI if not defined.

> Cheers,
>
> Mark
>

-- 
Thanks,
Hemant Kumar

Re: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Frank Ch. Eigler]

Hi -

On Thu, Apr 09, 2015 at 09:49:18PM +0530, Hemant Kumar wrote:
> [...]
> But, when I tried that this with multimap, the problem I faced is we can 
> insert two or more entries with same key and value, [...]
> But yeah, this can be done with multimap with a duplication check before 
> insertion.

... or have the lookup function compute a set<> from the multimap range,
thereby doing duplicate elimination implicitly.

- FChE

Re: [PATCH v3 1/2] systemtap/tapsets.cxx: Adjusted for multiple static functions

[Hemant Kumar]

On 04/10/2015 01:00 AM, Frank Ch. Eigler wrote:
> Hi -
>
> On Thu, Apr 09, 2015 at 09:49:18PM +0530, Hemant Kumar wrote:
>> [...]
>> But, when I tried that this with multimap, the problem I faced is we can
>> insert two or more entries with same key and value, [...]
>> But yeah, this can be done with multimap with a duplication check before
>> insertion.
> ... or have the lookup function compute a set<> from the multimap range,
> thereby doing duplicate elimination implicitly.

Great idea! Thanks.

> - FChE
>

-- 
Thanks,
Hemant Kumar