Re: [PATCH] Make symbol_set_names a member function

[Simon Marchi <simark@simark.ca>]

On 2019-12-26 8:09 p.m., Christian Biesinger wrote:
> On Thu, Dec 26, 2019 at 6:50 PM Simon Marchi <simark@simark.ca> wrote:
>>
>> On 2019-12-26 2:52 a.m., cbiesinger@chromium.org wrote:
>>> From: Christian Biesinger <cbiesinger@google.com>
>>>
>>> This also renames it to make it clearer that this is not a cheap
>>> functin (to compute_and_set_names).  Also renames name to m_name
>>
>> "functin"
> 
> Thanks, fixed locally.
> 
>>> to make the implementation of the renamed function more readable.
>>>
>>> Most of the places that access sym->m_name directly were also changed
>>> to call linkage_name () instead, to make it clearer which name they
>>> are accessing.
>>
>> I think that all makes sense.
>>
>>> @@ -2110,7 +2109,8 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
>>>           if (!sym)
>>>             {
>>>               printf_filtered ("Static symbol `");
>>> -             puts_filtered ((*psym)->ginfo.name);
>>> +             /* TODO: Should this be print_name ()?  */
>>> +             puts_filtered ((*psym)->ginfo.linkage_name ());
>>>               printf_filtered ("' only found in ");
>>>               puts_filtered (ps->filename);
>>>               printf_filtered (" psymtab\n");
>>> @@ -2128,7 +2128,8 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
>>>           if (!sym)
>>>             {
>>>               printf_filtered ("Global symbol `");
>>> -             puts_filtered ((*psym)->ginfo.name);
>>> +             /* TODO: Should this be print_name ()?  */
>>> +             puts_filtered ((*psym)->ginfo.linkage_name ());
>>>               printf_filtered ("' only found in ");
>>>               puts_filtered (ps->filename);
>>>               printf_filtered (" psymtab\n");
>>
>> For this patch, I wouldn't change the behavior (which means using linkage_name), but we could
>> consider a separate patch to change it.
> 
> I removed the TODO locally. OK to push with withose two changes?

Yes.

>> I hacked the code to always enter these ifs and print both the linkage_name
>> and the natural_name.  With a C++ test program containing this function:
>>
>>   int hello(int);
>>
>> I get:
>>
>>   linkage_name: hello
>>   natural_name: hello
>>
>> I would have expected linkage_name to be _Z5helloi and the natural_name to be
>> hello(int).  Do you know if it's expected for the partial symbol to contain
>> just "hello" for both?
> 
> Huh..
> 
> I added a printf in compute_and_set_names and found that there's a
> symbol with the mangled name *and* a symbol with the plain name. I
> guess that's why? But I don't know what that means...

Ok, I see what happens.  The first call to compute_and_set_names
is while parsing minimal symbols, by minimal_symbol_reader::install.
This one has a mangled linkage name (_Z5helloi).

The second call is while creating partial symbols, by add_psymbol_to_bcache.
This is the one with the linkage name "hello".  I suppose that's expected,
I had never really paid attention to this.

There's also a third call, while creating the full blown symbol, with a
linkage name of "hello(int)".

So the term "linkage_name" in general_symbol_info is perhaps a bit misleading,
it makes sense for minimal symbols, but not really for partial and full symbols.

Simon