[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