This is the mail archive of the mailing list for the systemtap project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

    // 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()
-          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?



Hemant Kumar

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]