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

[Mark Wielaard]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-multisym-test.patch
Type: text/x-patch
Size: 2876 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/systemtap/attachments/20150408/1836e657/attachment.bin>