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

[Hemant Kumar <hemant at linux dot vnet dot ibm dot com>]

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