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

[Mark Wielaard <mjw at redhat dot com>]

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

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