Re: [PATCH] Make symbol_set_names a member function

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

On Fri, Dec 27, 2019 at 4:02 AM Simon Marchi <simark@simark.ca> wrote:
>
> 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.

Thanks, pushed:
To ssh://sourceware.org/git/binutils-gdb.git
   b0d674e2b4..4d4eaa3005  HEAD -> master


> >> 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.

Hm.. maybe I'll look into this some more to understand this better. It
certainly seems surprising that minsyms behave differently from other
symbols in this respect?

Christian