Re: [PATCH PR gdb/18071] TLS variables can't be resolved on aarch64-linux-gnu

[Weimin Pan <weimin dot pan at oracle dot com>]

On 3/17/2018 9:52 AM, Simon Marchi wrote:
> On 2017-11-02 08:38 PM, Weimin Pan wrote:
> > Running the test case with upstream gdb shows two failures:
> >
> > (1) Receiving different error messages when printing TLS variable before
> >      program runs - because the ARM compiler does not emit dwarf attribute
> >      DW_AT_location for TLS, the result is expected and the baseline may
> >      need to be changed for aarch64.
> >
> > (2) Using "info address" command on C++ static TLS object resulted in
> >      "symbol unresolved" error - below is a snippet from the test case:
> >
> > class K {
> >   public:
> >    static __thread int another_thread_local;
> > };
> >
> > __thread int K::another_thread_local;
> >
> > (gdb) info address K::another_thread_local
> > Symbol "K::another_thread_local" is unresolved.
> >
> > This patch contains fix for (2).
> >
> > Function info_address_command() handles the "info address" command and
> > calls lookup_minimal_symbol_and_objfile() to find sym's symbol entry in
> > mininal symbol table if SYMBOL_COMPUTED_OPS (sym) is false. Problem is
> > that function lookup_minimal_symbol_and_objfile() only looked up an
> > objfile's minsym ordinary hash table, not its demangled hash table, which
> > was the reason why the C++ name was not found.
> >
> > The fix is to call lookup_minimal_symbol(), which already looks up entries
> > in both minsym's hash tables, to find names when traversing the object file
> > list in lookup_minimal_symbol_and_objfile().
> >
> > Tested in both aarch64-linux-gnu and amd64-linux-gnu. No regressions.
> Hi Weimin,
>
> I would be ok with making lookup_minimal_symbol_and_objfile look through demangled
> names as well, I don't see a need for it to only search through linkage names.
> I don't think it should break any user of that function, since it would mean that
> there is a clash between a mangled name and a demangled name, I don't think this is
> likely.

Hi Simon,

Yes, you're right that a name clash is unlikely to happen. I think you have
a better suggestion, at the end of your email, to leave this function alone.
But it'd be good to update the doc for lookup_minimal_symbol_and_objfile(),
where the term "linkage name" should be replaced with either "ordinary" or
"demangled" because it seems to me that "linkage name" == "mangled name".

> The doc of lookup_minimal_symbol_and_objfile in minsyms.h should be updated.
>
> So this now works:
>
> (gdb) info address K::another_thread_local2
> Symbol "K::another_thread_local2" is a thread-local variable at offset 0x4 in the thread-local storage for `/home/simark/build/binutils-gdb/gdb/test'.
>
> But printing the variable doesn't:
>
> (gdb) p K::another_thread_local2
> Cannot find thread-local storage for process 8959, executable file /home/simark/build/binutils-gdb/gdb/test:
> Cannot find thread-local variables on this target
>
> Is this also what you see?  If the scope of this patch is to only fix "info address",
> could you change the title of the patch to reflect it?  Otherwise it sounds like
> it fixes actually accessing/printing the variable as well.

Yes, printing of a TLS fails on all platforms, not just on aarch64.
How about changing the title to:

   [PATCH PR gdb/18071] aarch64: "info" command can't resolve TLS variables

> >   gdb/ChangeLog |    5 +++++
> >   gdb/minsyms.c |   17 +++--------------
> >   2 files changed, 8 insertions(+), 14 deletions(-)
> >
> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> > index 4b292e0..2f630bc 100644
> > --- a/gdb/ChangeLog
> > +++ b/gdb/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2017-11-01  Weimin Pan  <weimin.pan@oracle.com>
> > +
> > +	* minsyms.c (lookup_minimal_symbol_and_objfile): Use
> > +	lookup_minimal_symbol() to find symbol entry.
> > +
> >   2017-10-27  Keith Seitz  <keiths@redhat.com>
> >   
> >   	* breakpoint.c (print_breakpoint_location): Use the symbol saved
> > diff --git a/gdb/minsyms.c b/gdb/minsyms.c
> > index 37edbd8..4edd8b1 100644
> > --- a/gdb/minsyms.c
> > +++ b/gdb/minsyms.c
> > @@ -881,23 +881,12 @@ lookup_minimal_symbol_and_objfile (const char *name)
> >   {
> >     struct bound_minimal_symbol result;
> >     struct objfile *objfile;
> > -  unsigned int hash = msymbol_hash (name) % MINIMAL_SYMBOL_HASH_SIZE;
> >   
> >     ALL_OBJFILES (objfile)
> >       {
> > -      struct minimal_symbol *msym;
> > -
> > -      for (msym = objfile->per_bfd->msymbol_hash[hash];
> > -	   msym != NULL;
> > -	   msym = msym->hash_next)
> > -	{
> > -	  if (strcmp (MSYMBOL_LINKAGE_NAME (msym), name) == 0)
> > -	    {
> > -	      result.minsym = msym;
> > -	      result.objfile = objfile;
> > -	      return result;
> > -	    }
> > -	}
> > +      result = lookup_minimal_symbol (name, NULL, objfile);
> > +      if (result.minsym != NULL)
> > +        return result;
> >       }
> Is this code equivalent to calling lookup_minimal_symbol (name, NULL, NULL) ?  If
> so, there's already lookup_bound_minimal_symbol that does the same, so maybe we
> can just drop lookup_minimal_symbol_and_objfile and use lookup_bound_minimal_symbol.

Yes, it turns out lookup_minimal_symbol_and_objfile(name) is equivalent to
calling lookup_minimal_symbol (name, NULL, NULL). We can replace the call in
info_address_command() with either lookup_minimal_symbol (name, NULL, NULL)
or lookup_bound_minimal_symbol(name). Calling either one looks like a 
better fix.

> We could also just have lookup_minimal_symbol with parameters that default to nullptr.
> It is not clear at all to have lookup_bound_minimal_symbol and lookup_minimal_symbol
> that both return a bound_minimal_symbol, that's quite misleading.

I don't know the rational behind having these two functions which get 
called in
quite a few places. Yes, having default arguments for 
lookup_minimal_symbol()
will be another option.

Thanks very much for your comments.

Weimin

> Simon